Re: AW: [PATCH RT RFC v4 1/8] add generalized priority-inheritanceinterface

[Gregory Haskins]

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