Re: [PATCH] printk: kmsg_dump: remove mutex usage

[John Ogness <john.ogness@linutronix.de>]

On 2019-04-24, Scott Wood <swood@redhat.com> wrote:
> On Wed, 2019-04-24 at 16:36 +0200, John Ogness wrote:
>> @@ -2830,16 +2829,18 @@ void kmsg_dump(enum kmsg_dump_reason reason)
>>  		if (dumper->max_reason && reason > dumper->max_reason)
>>  			continue;
>>  
>> -		/* initialize iterator with data about the stored records */
>> -		dumper->active = true;
>> +		/*
>> +		 * use a local copy to avoid modifying the
>> +		 * iterator used by any other cpus/contexts
>> +		 */
>> +		memcpy(&dumper_local, dumper, sizeof(dumper_local));
>>  
>> -		kmsg_dump_rewind(dumper);
>> +		/* initialize iterator with data about the stored records */
>> +		dumper_local.active = true;
>> +		kmsg_dump_rewind(&dumper_local);
>>  
>>  		/* invoke dumper which will iterate over records */
>> -		dumper->dump(dumper, reason);
>> -
>> -		/* reset iterator */
>> -		dumper->active = false;
>> +		dumper_local.dump(&dumper_local, reason);
>>  	}
>
> When would a dumper (or anything else that checks it) ever see active be
> false?

A valid question. Originally I wanted to completely remove the active
flag. But really the rt patchset is not the place for these kinds of
changes. I am currently reworking printk (for mainline) and I will
evaluate the purpose/usefulness of the active flag for that work.

>>  	rcu_read_unlock();
>>  }
>> @@ -2951,9 +2952,7 @@ bool kmsg_dump_get_line(struct kmsg_dumper *dumper,
>> bool syslog,
>>  {
>>  	bool ret;
>>  
>> -	mutex_lock(&kmsg_dump_lock);
>>  	ret = kmsg_dump_get_line_nolock(dumper, syslog, line, size, len);
>> -	mutex_unlock(&kmsg_dump_lock);
>>  
>>  	return ret;
>>  }
>> @@ -3105,9 +3104,7 @@ void kmsg_dump_rewind_nolock(struct kmsg_dumper
>> *dumper)
>>   */
>>  void kmsg_dump_rewind(struct kmsg_dumper *dumper)
>>  {
>> -	mutex_lock(&kmsg_dump_lock);
>>  	kmsg_dump_rewind_nolock(dumper);
>> -	mutex_unlock(&kmsg_dump_lock);
>>  }
>>  EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>>  
>
> Any reason not to get rid of the wrappers now that the lock's gone?

I wanted my patch to be as less intrusive as possible. For my mainline
work I will look into eliminating the wrappers.

John Ogness