Re: [PATCH] Process Aggregates (PAGG) support for the 2.6 kernel

[Chris Wright <chrisw@osdl.org>]

* Erik Jacobson (erikj@subway.americas.sgi.com) wrote:
> I didn't choose to change the macros at this time - however, I'm not against
> changing them either - I just haven't done it yet.

OK.  I still think it's a good idea for readability and type safety.

> On Mon, 26 Apr 2004, Chris Wright wrote:
> > This looks like it's just the infrastructure, i.e. nothing is using it.
> > It seems like PAGG could be done on top of CKRM (albeit, with more
> > code).  But if the goal is to do some basic accounting, scheduling, etc.
> > on a resource group, wouldn't CKRM be more generic?
> 
> Right.  A couple examples of things we have that use it are CSA
> (oss.sgi.com/csa) and job.  job provides inescapable job containers that
> are also used by csa.
> 
> But what I presented here was just the infrastructure as you said.
> 
> Patches for inescapable job containers ('job') are available on the pagg web
> site as well (oss.sgi.com/pagg).

OK, thanks, I'll take a look.

> > > +       char		*name;	/* Name Key - restricted to 32 characters */
> >
> > why the restriction?
> 
> I'm open to suggestions.  Right now, this is usually set to something like
> "job" or similar.  The max length is enforced by the module that makes use
> of pagg.  For example, with the job package:

Right, if it's a pointer, and you guarantee it's NULL terminated, then I
don't see the point.  Otherwise, strncmp() or something?

> I fixed the tasklist issue you were concerned about.  Again, I didn't address
> the macro issue at this moment.

Alright, see below.

> > This looks like it leaks the just alloc'd to_pagg.
> 
> I agree that it looks suspect but I think it's OK.
> 
> You're talking about the case where the pagg was allocated, but couldn't
> attach I assume.
> 
> The alloc_pagg function adds that allocated pagg to the pagg list.  In
> error_return, detach_pagg_list is called so this pagg should be freed then.

Yes, I see it now, thanks.  BTW, I see a common idiom here of:

 if (!list_empty) {
	list_for_each() {
	}

 }

Seems like mostly an empty optimization, since list_for_each essentially
does that list_empty() check, no?

> +unregister_pagg_hook(struct pagg_hook *pagg_hook_old)
> +{
<snip>
> +	down_write(&pagg_hook_list_sem);
<snip>
> +		read_lock(&tasklist_lock);
> +		for_each_process(task) {
> +			struct pagg *pagg = NULL;
> +
> +			get_task_struct(task); /* So the task doesn't vanish on us */
> +			read_unlock(&tasklist_lock);

dropped tasklist_lock, task could exit, and unlink and potentially drop the 
only other ref.

> +			read_lock_pagg_list(task);
> +			pagg = get_pagg(task, pagg_hook_old->name);
> +			put_task_struct(task);

and this could have totally freed the memory to task.

still looks unsafe to me.

> +			/* 
> +			 * We won't be accessing pagg's memory, just need
> +			 * to see if one existed - so we can release the task
> +			 * lock now.
> +			 */
> +			read_unlock_pagg_list(task);
> +			if (pagg) {
> +				up_write(&pagg_hook_list_sem);
> +				return -EBUSY;
> +			}
> +
> +			/* lock the task list again so we get a valid task in the loop */
> +			read_lock(&tasklist_lock);
> +		}
> +		read_unlock(&tasklist_lock);
> +		list_del_init(&pagg_hook->entry);
> +		up_write(&pagg_hook_list_sem);

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net