From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f72.google.com (mail-pg0-f72.google.com [74.125.83.72]) by kanga.kvack.org (Postfix) with ESMTP id 71EA36B0033 for ; Fri, 3 Nov 2017 17:49:35 -0400 (EDT) Received: by mail-pg0-f72.google.com with SMTP id b192so4785989pga.14 for ; Fri, 03 Nov 2017 14:49:35 -0700 (PDT) Received: from hqemgate14.nvidia.com (hqemgate14.nvidia.com. [216.228.121.143]) by mx.google.com with ESMTPS id g3si5335832plp.292.2017.11.03.14.49.32 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 03 Nov 2017 14:49:32 -0700 (PDT) Subject: Re: [PATCH v3] printk: Add console owner and waiter logic to load balance console writes References: <20171102134515.6eef16de@gandalf.local.home> <82a3df5e-c8ad-dc41-8739-247e5034de29@suse.cz> <9f3bbbab-ef58-a2a6-d4c5-89e62ade34f8@nvidia.com> <20171103072121.3c2fd5ab@vmware.local.home> <20171103075404.14f9058a@vmware.local.home> From: John Hubbard Message-ID: Date: Fri, 3 Nov 2017 14:46:56 -0700 MIME-Version: 1.0 In-Reply-To: <20171103075404.14f9058a@vmware.local.home> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Steven Rostedt Cc: Vlastimil Babka , LKML , Peter Zijlstra , akpm@linux-foundation.org, linux-mm@kvack.org, Cong Wang , Dave Hansen , Johannes Weiner , Mel Gorman , Michal Hocko , Petr Mladek , Sergey Senozhatsky , "yuwang.yuwang" , Linus Torvalds , Jan Kara , Mathieu Desnoyers , Tetsuo Handa On 11/03/2017 04:54 AM, Steven Rostedt wrote: > On Fri, 3 Nov 2017 07:21:21 -0400 > Steven Rostedt wrote: > >> On Thu, 2 Nov 2017 21:09:32 -0700 >> John Hubbard wrote: >> > > > >>> 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 >> >> But console_waiter only gets set to 1 if console_waiter is 0 *and* >> console_owner is not NULL and is not current. console_owner is only >> updated under a spin lock and console_waiter is only set under a spin >> lock when console_owner is not NULL. >> >> This means this scenario can not happen. >> >> >>> >>> 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. >> >> As stated above, the console owner check will prevent this issue. >> > > 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 > > -- 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: email@kvack.org > -- 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: email@kvack.org