lkml.org 
[lkml]   [2002]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2005-03-22 13:30    [W:0.078 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site