Messages in this thread | | | Date | Fri, 25 Oct 2002 14:18:29 +1000 | From | Rusty Russell <> | Subject | Re: [PATCH]updated ipc lock patch |
| |
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/
| |