[Devel] Re: [PATCH 2/2] Add support for the per-task sem_undo list (v2)

[Dan Smith]

>> +static void put_undo_list(struct sem_undo_list *ulp);

OL> nit: to me it makes more sense to move
OL> grab/drop/checkpoint/restart code, as well sem_init(), to the end
OL> of the file (and avoid those statics...).

Okay, was trying to avoid chopping the file up too much.

OL> Can get rid of ..._users() since we don't collect them.

Oops :)

OL> semadj is short, so:
OL> s/__u16/__s16/

Gah, yeah, I always want everything to be unsigned for some reason :D

>> -		sma = sem_lock_check(tsk->nsproxy->ipc_ns, un->semid);
>> +		sma = sem_lock_check(ulp->ipc_ns, un->semid);

OL> This hunk belongs to previous patch, no ?

It could, but it seems more relevant when converting the function from
using a task to using just the undo_list itself...

>> +	atomic_inc(&ulp->refcnt);

OL> I suspect there is a leak here: atomic_inc is needed only for
OL> tasks other than the first task; The first task already gets the
OL> refcount deep inside find_alloc_undo(), and the hash-table gets
OL> its ref via the obj->grab method.

But then it drops that ref right after it inserts it, thus we need
another one, right?.  I think the confusion may come from the fact
that the hash assumes the first refcount is its own and the caller of
restore_obj() will grab another (for the task, in this case).  Since
we don't do that, we need to grab both early on.  I hit this in
another place in the network stuff, IIRC, which generated a similar
review comment ;)

-- 
Dan Smith
IBM Linux Technology Center
email: danms at us.ibm.com
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers