On Thu, 24 Oct 2002 16:30:32 -0700
Andrew Morton <akpm@digeo.com> wrote:
> Hugh Dickins wrote:
> >
> > ...
> > Manfred and I have both reviewed the patch (or the 2.5.44 version)
> > and we both recommend it highly (well, let Manfred speak for himself).
> >
>
> OK, thanks.
>
> So I took a look. Wish I hadn't :( The locking rules in there
> are outrageously uncommented. You must be brave people.
Agreed. Here's my brief audit:
>+ int max_id = ids->max_id;
>
>- for (id = 0; id <= ids->max_id; id++) {
>+ read_barrier_depends();
>+ for (id = 0; id <= max_id; id++) {
That needs to be a rmb(), not a read_barrier_depends(). And like all
barriers, it *requires* a comment:
/* We must read max_id before reading any entries */
I can't see the following in the patch posted, but:
> void ipc_rcu_free(void* ptr, int size)
> {
> struct rcu_ipc_free* arg;
>
> arg = (struct rcu_ipc_free *) kmalloc(sizeof(*arg), GFP_KERNEL);
> if (arg == NULL)
> return;
> arg->ptr = ptr;
> arg->size = size;
> call_rcu(&arg->rcu_head, ipc_free_callback, arg);
> }
This is unacceptable crap, sorry. You *must* allocate the resources
required to free the object *at the time you allocate the object*,
since freeing must not fail.
> Even better: is it possible to embed the rcu_ipc_free inside the
> object-to-be-freed? Perhaps not?
Yes, this must be done.
Rusty.
-- there are those who do and those who hang on and you don't see too many doers quoting their contemporaries. -- Larry McVoy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Thu Oct 31 2002 - 22:00:26 EST