On 11/02/2017 03:16 PM, Vlastimil Babka wrote:
> On 11/02/2017 06:45 PM, Steven Rostedt wrote:
> ...> __DEVKMSG_LOG_BIT_ON = 0,
>> __DEVKMSG_LOG_BIT_OFF,
>> @@ -1753,8 +1760,56 @@ asmlinkage int vprintk_emit(int facility
>> * semaphore. The release will print out buffers and wake up
>> * /dev/kmsg and syslog() users.
>> */
>> - if (console_trylock())
>> + if (console_trylock()) {
>> console_unlock();
>> + } else {
>> + struct task_struct *owner = NULL;
>> + bool waiter;
>> + bool spin = false;
>> +
>> + 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);
>> +
>> + /*
>> + * If there is an active printk() writing to the
>> + * consoles, instead of having it write our data too,
>> + * see if we can offload that load from the active
>> + * printer, and do some printing ourselves.
>> + * Go into a spin only if there isn't already a waiter
>> + * spinning, and there is an active printer, and
>> + * that active printer isn't us (recursive printk?).
>> + */
>> + if (spin) {
>> + /* We spin waiting for the owner to release us */
>> + spin_acquire(&console_owner_dep_map, 0, 0, _THIS_IP_);
>> + /* Owner will clear console_waiter on hand off */
>> + while (!READ_ONCE(console_waiter))
>
> This should not be negated, right? We should spin while it's true, not
> false.
>
Vlastimil's right about the polarity problem above, but while I was trying
to verify that, I noticed another problem: the "handoff" of the console lock
is broken.
For example, if there are 3 or more threads, you can do the following:
thread A: holds the console lock, is printing, then moves into the console_unlock
phase
thread B: goes into the waiter spin loop above, and (once the polarity is corrected)
waits for console_waiter to become 0
thread A: finishing up, sets console_waiter --> 0
thread C: before thread B notices, thread C goes into the "else" section, sees that
console_waiter == 0, and sets console_waiter --> 1. So thread C now
becomes the waiter
thread B: gets *very* unlucky and never sees the 1 --> 0 --> 1 transition of
console_waiter, so it continues waiting. And now we have both B
and C in the same spin loop, and this is now broken.
At the root, this is really due to the absence of a pre-existing "hand-off this lock"
mechanism. And this one here is not quite correct.
Solution ideas: for a true hand-off, there needs to be a bit more information
exchanged. Conceptually, a (lock-protected) list of waiters (which would
only ever have zero or one entries) is a good way to start thinking about it.
I talked it over with Mark Hairgrove here, he suggested a more sophisticated
way of doing that sort of hand-off, using compare-and-exchange. I can turn that
into a patch if you like (I'm not as fast as some folks, so I didn't attempt to
do that right away), although I'm sure you have lots of ideas on how to do it.
thanks,
John Hubbard
>> + cpu_relax();
>> +
>> + spin_release(&console_owner_dep_map, 1, _THIS_IP_);
>> + printk_safe_exit_irqrestore(flags);
>> +
>> + /*
>> + * The owner passed the console lock to us.
>> + * Since we did not spin on console lock, annotate
>> + * this as a trylock. Otherwise lockdep will
>> + * complain.
>> + */
>> + mutex_acquire(&console_lock_dep_map, 0, 1, _THIS_IP_);
>> + console_unlock();
>> + printk_safe_enter_irqsave(flags);
>> + }
>> + printk_safe_exit_irqrestore(flags);
>> +
>> + }
>> }
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
