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/