Re: [PATCH] [RFC] seqcount: Add lockdep functionality to seqcount/seqlock structures

[John Stultz <john.stultz@linaro.org>]

On 09/10/2013 01:43 AM, Peter Zijlstra wrote:
> On Mon, Sep 09, 2013 at 09:42:46PM -0700, John Stultz wrote:
>> Currently seqlocks and seqcounts don't support lockdep.
>>
>> After running across a seqcount related deadlock in the timekeeping
>> code, I used a less-refined and more focused varient of this patch
>> to narrow down the cause of the issue.
>>
>> This is a first-pass attempt to properly enable lockdep functionality
>> on seqlocks and seqcounts.
>>
>> Due to seqlocks/seqcounts having slightly different possible semantics
>> then standard locks (ie: reader->reader and reader->writer recursion is
>> fine, but writer->reader is not), this implementation is probably not
>> as exact as I'd like (currently using a hack by only spot checking
>> readers), and may be overly strict in some cases.
>>
>> I've handled one cases where there were nested seqlock writers, and
>> there may be more edge cases, as while I've gotten it to run cleanly,
>> depending on config its reporting issues that I'm not sure if they are
>> flaws in the implementation or actual bugs. But I wanted to send this
>> out for some initial thoughts as until today I hadn't looked at much
>> of the lockdep infrastructure. So I'm sure there are improvements
>> that could be made.
>>
>> Comments and feedback would be appreciated!
>> --- a/arch/x86/vdso/vclock_gettime.c
>> +++ b/arch/x86/vdso/vclock_gettime.c
>> @@ -178,13 +178,15 @@ notrace static int __always_inline do_realtime(struct timespec *ts)
>>  
>>  	ts->tv_nsec = 0;
>>  	do {
>> -		seq = read_seqcount_begin(&gtod->seq);
>> +		seq = __read_seqcount_begin(&gtod->seq);
>> +		smp_rmb();
>>  		mode = gtod->clock.vclock_mode;
>>  		ts->tv_sec = gtod->wall_time_sec;
>>  		ns = gtod->wall_time_snsec;
>>  		ns += vgetsns(&mode);
>>  		ns >>= gtod->clock.shift;
>> -	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
>> +		smp_rmb();
>> +	} while (unlikely(__read_seqcount_retry(&gtod->seq, seq)));
>>  
>>  	timespec_add_ns(ts, ns);
>>  	return mode;
>
> You didn't mention the VDSO 'fun' in you Changelog!

No, although its not any net change. I'll clean it up w/ a helper
function so that we're not doing the memory barriers in the logic here.
That should simplify the diff a bit.

thanks
-john