Re: [PATCH v3] printk: Add console owner and waiter logic to load balance console writes

[John Hubbard <jhubbard@nvidia.com>]

On 11/03/2017 02:46 PM, John Hubbard wrote:
> On 11/03/2017 04:54 AM, Steven Rostedt wrote:
>> On Fri, 3 Nov 2017 07:21:21 -0400
>> Steven Rostedt <rostedt@goodmis.org> wrote:
[...]
>>
>> I'll condense the patch to show what I mean:
>>
>> To become a waiter, a task must do the following:
>>
>> +			printk_safe_enter_irqsave(flags);
>> +
>> +			raw_spin_lock(&console_owner_lock);
>> +			owner = READ_ONCE(console_owner);
>> +			waiter = READ_ONCE(console_waiter);
>> +			if (!waiter && owner && owner != current) {
>> +				WRITE_ONCE(console_waiter, true);
>> +				spin = true;
>> +			}
>> +			raw_spin_unlock(&console_owner_lock);
>>
>>
>> The new waiter gets set only if there isn't already a waiter *and*
>> there is an owner that is not current (and with the printk_safe_enter I
>> don't think that is even needed).
>>
>> +				while (!READ_ONCE(console_waiter))
>> +					cpu_relax();
>>
>> The spin is outside the spin lock. But only the owner can clear it.
>>
>> Now the owner is doing a loop of this (with interrupts disabled)
>>
>> +		raw_spin_lock(&console_owner_lock);
>> +		console_owner = current;
>> +		raw_spin_unlock(&console_owner_lock);
>>
>> Write to consoles.
>>
>> +		raw_spin_lock(&console_owner_lock);
>> +		waiter = READ_ONCE(console_waiter);
>> +		console_owner = NULL;
>> +		raw_spin_unlock(&console_owner_lock);
>>
>> +		if (waiter)
>> +			break;
>>
>> At this moment console_owner is NULL, and no new waiters can happen.
>> The next owner will be the waiter that is spinning.
>>
>> +	if (waiter) {
>> +		WRITE_ONCE(console_waiter, false);
>>
>> There is no possibility of another task sneaking in and becoming a
>> waiter at this moment. The console_owner was cleared under spin lock,
>> and a waiter is only set under the same spin lock if owner is set.
>> There will be no new owner sneaking in because to become the owner, you
>> must have the console lock. Since it is never released between the time
>> the owner clears console_waiter and the waiter takes the console lock,
>> there is no race.
> 
> Yes, you are right of course. That does close the window. Sorry about
> missing that point.
> 
> I'll try to quickly put together a small patch on top of this, that
> shows a simplification, to just use an atomic compare and swap between a
> global atomic value, and a local (on the stack) flag value, just in
> case that is of interest.
> 
> thanks
> john h

Just a follow-up: I was unable to simplify this; the atomic compare-and-swap
approach merely made it different, rather than smaller or simpler. 

So, after spending a fair amount of time with the patch, it looks good to me,
for whatever that's worth. :) Thanks again for explaining the locking details.

thanks
john h

> 
>>
>> -- Steve
>>

>>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>