On 2020-12-04, Petr Mladek <[email protected]> wrote:
> On Tue 2020-12-01 21:59:40, John Ogness wrote:
>> Currently @clear_seq access is protected by @logbuf_lock. Once
>> @logbuf_lock is removed some other form of synchronization will be
>> required. Change the type of @clear_seq to atomic64_t to provide the
>> synchronization.
>>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index fc5e3a7d6d89..e9018c4e1b66 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -3412,7 +3418,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer);
>> */
>> void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper)
>> {
>> - dumper->cur_seq = clear_seq;
>> + dumper->cur_seq = atomic64_read(&clear_seq);
>
> Sigh, atomic64_read() uses a spin lock in the generic implementation
> that is used on some architectures.
>
> Hmm, this seems to be the only location where the lock must not be
> used.
Yes, and it is read-only access. Perhaps atomic64_t is the wrong thing
to use here. We could use a seqcount_latch and a shadow variable so that
if a writer has been preempted, we can use the previous value. (Only
kmsg_dump would need to use the lockless variant to read the value.)
void clear_seq_set(u64 val)
{
spin_lock_irq(&clear_lock);
raw_write_seqcount_latch(&clear_latch);
clear_seq[0] = val;
raw_write_seqcount_latch(&clear_latch);
clear_seq[1] = val;
spin_unlock_irq(&clear_lock);
}
u64 clear_seq_get_nolock(void)
{
unsigned int seq, idx;
u64 val;
do {
seq = raw_read_seqcount_latch(&clear_latch);
idx = seq & 0x1;
val = clear_seq[idx];
} while (read_seqcount_latch_retry(&clear_latch, seq));
return val;
}
u64 clear_seq_get(void)
{
u64 val;
spin_lock_irq(&clear_lock);
val = clear_seq[0];
spin_unlock_irq(&clear_lock);
return val;
}
> Alternative solution would to always fallback to the first_seq on
> these architectures. Few people would complain when they see more
> messages. We could always improve it when it causes problems.
I am also OK with this solution.
John Ogness
