Hi Matthias, Matthias Behr wrote:
Hi Greg, I got a few review comments/questions. Pls see below. Best Regards, Matthias P.S. I'm a kernel newbie so don't hesitate to tell me if I'm wrong ;-)+/** + * pi_sink_init - initialize a pi_sink before use + * @sink: a sink context + * @ops: pointer to an pi_sink_ops structure + */ +static inline void +pi_sink_init(struct pi_sink *sink, struct pi_sink_ops *ops) +{ + atomic_set(&sink->refs, 0); + sink->ops = ops; +}Shouldn't ops be tested for 0 here? (ASSERT/BUG_ON/...) (get's dereferenced later quite often in the form "if (sink->ops->...)".
This is a good idea. I will add this.
+/** + * pi_sink_put - down the reference count, freeing the sink if 0 + * @node: the node context + * @flags: optional flags to modify behavior. Reserved, must be 0. + * + * Returns: none + */ +static inline void +pi_sink_put(struct pi_sink *sink, unsigned int flags) +{ + if (atomic_dec_and_test(&sink->refs)) { + if (sink->ops->free) + sink->ops->free(sink, flags); + } +}Shouldn't the atomic/locked part cover the ...->free(...) as well?
Actually, it already does. The free can only be called by the last reference dropping the ref-count.
A pi_get right after the atomic_dec_and_test but before the free() could lead to a free() with refs>0?
A pi_get() after the ref could have already dropped to zero is broken at a higher layer. E.g. the caller of pi_get() has to ensure that there are no races against the reference dropping to begin with. This is the same as any reference-counted object (for instance, see get_task_struct()).
Thanks for the review, Matthias! Regards, -Greg
Attachment:
signature.asc
Description: OpenPGP digital signature
