Re: [PATCH]updated ipc lock patch

From: Hugh Dickins (hugh@veritas.com)
Date: Thu Oct 24 2002 - 18:59:03 EST


On Thu, 24 Oct 2002, Andrew Morton 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.

Ah, we all like to criticize the lack of comments in others' code.

> What about this code?
>
> 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);
> }
>
> Are we sure that it's never called under locks?

Yes.

> And it seems that if the kmalloc fails, we decide to leak some
> memory, yes?

Yes, but why would it fail?
and what do you think should be the alternative?

> If so it would be better to use GFP_ATOMIC there. Avoids any
> locking problems and also increases the chance of the allocation
> succeeding. (With an explanatory comment, naturally :)).

There are no locking doubts here.
GFP_ATOMIC would _reduce_ the chance of the allocation succeeding:
GFP_KERNEL does include the __GFP_WAIT flag, GFP_ATOMIC does not.

> Even better: is it possible to embed the rcu_ipc_free inside the
> object-to-be-freed? Perhaps not?

It would certainly be possible (I did suggest it as a maybe),
but it's unclear whether it's worthwhile wasting the extra memory
longterm like that. Mingming chose not to embed, I see no reason
to overrule.

> Stylistically, it is best to not typecast the return value
> from kmalloc, btw. You should never typecast the return
> value of anything which returns a void *, because it weakens
> your compile-time checking. Example:
>
> foo *bar = (foo *)zot();
>
> The compiler will swallow that, regardless of what zot() returns.
> Someone could go and change zot() to return a reiserfs_inode *
> and you would never know about it. Whereas:
>
> foo *bar = zot();
>
> Says to the compiler "zot() must return a bar * or a void *",
> which is much tighter checking, yes?

You have too much time on your hands, Andrew :-)

> There is an insane amount of inlining in the ipc code. I
> couldn't keep my paws off it.

I agree tempting: I thought you might like that in a subsequent patch,
yes? Mingming was splitting locks, not doing a cleanup of inlines.

Hugh

-
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:25 EST