>>> On 30.06.10 at 18:58, George Dunlap <dunlapg@xxxxxxxxx> wrote:
> Most of the patch, other than the volatiles, looks good; thanks for doing
> this.
>
> Can you explain exactly what it is you're trying to accomplish by
> making things volatile? What kinds of failure modes are you expecting
> to protect against? Glancing through the code, I can't really see
> that making things volatile will make much of a difference.
The point is to prevent the compiler from doing the memory reads
more than once (which it is permitted to do on non-volatiles). As
said in the submission comment, this could be done via barriers too,
but one of the two has to be there.
> Further comments inline below.
>
> On Tue, Jun 29, 2010 at 4:37 PM, Jan Beulich <JBeulich@xxxxxxxxxx> wrote:
>> --- 2010-06-15.orig/xen/common/trace.c 2010-06-29 17:04:45.000000000 +0200
>> +++ 2010-06-15/xen/common/trace.c 2010-06-29 17:05:09.000000000 +0200
> [snip]
>> @@ -626,13 +649,11 @@ void __trace_var(u32 event, int cycles,
>>
>> /* Read tb_init_done /before/ t_bufs. */
>> rmb();
>> -
>> - spin_lock_irqsave(&this_cpu(t_lock), flags);
>> -
>> buf = this_cpu(t_bufs);
>> -
>> if ( unlikely(!buf) )
>> - goto unlock;
>> + return;
>> +
>> + spin_lock_irqsave(&this_cpu(t_lock), flags);
>
> This isn't any good: the whole point of this lock is to keep t_bufs
> from disappearing under our feet.
Not really - the buffer can't disappear if we make it here, it can only
disappear when tb_init_done wasn't set yet. But if that's a major
concern, I can of course put the check back into the locked region.
And wait, I think in the end there's no change needed at all -
originally I thought going to the "unlock" label here is unsafe as
the code there would de-reference buf, but started_below_highwater
still being zero would prevent this.
So I'll submit another version in any case, after we settled on
whether using volatile qualifiers is acceptable here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home