[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <48AA859D.4030005@novell.com>
Date: Tue, 19 Aug 2008 04:34:37 -0400
From: Gregory Haskins <ghaskins@...ell.com>
To: Matthias Behr <linux@...ehr.de>
CC: linux-kernel@...r.kernel.org, linux-rt-users@...r.kernel.org,
gregory.haskins@...il.com, mingo@...e.hu, David.Holmes@....com,
jkacur@...il.com, paulmck@...ux.vnet.ibm.com, peterz@...radead.org,
tglx@...utronix.de, rostedt@...dmis.org
Subject: Re: AW: [PATCH RT RFC v4 1/8] add generalized priority-inheritance
interface
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
Download attachment "signature.asc" of type "application/pgp-signature" (258 bytes)
Powered by blists - more mailing lists