Re: [PATCH next v2 2/3] printk: change @clear_seq to atomic64_t

[John Ogness]

On 2020-12-04, Petr Mladek <pmladek@xxxxxxxx> 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