On 2019-04-24, Scott Wood <swood@xxxxxxxxxx> 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
