From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756006AbdKCLV1 (ORCPT ); Fri, 3 Nov 2017 07:21:27 -0400 Received: from smtprelay0237.hostedemail.com ([216.40.44.237]:39761 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753003AbdKCLV0 (ORCPT ); Fri, 3 Nov 2017 07:21:26 -0400 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::::::::::::::::::::::::,RULES_HIT:41:355:379:541:599:800:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1535:1543:1593:1594:1605:1711:1730:1747:1777:1792:2194:2198:2199:2200:2393:2553:2559:2562:2693:2892:2895:2915:3138:3139:3140:3141:3142:3622:3865:3866:3867:3868:3870:3871:3872:3873:3874:4250:4411:4432:4470:4605:5007:6117:6261:6742:7875:7904:9010:10004:10400:10848:10967:11026:11232:11473:11658:11914:12043:12296:12438:12663:12740:12760:12895:13255:13439:14096:14097:14181:14659:14721:21067:21080:21324:21433:21451:21611:21627:30054:30064:30070:30075:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:1,LUA_SUMMARY:none X-HE-Tag: quiet58_c1fb4d5a9926 X-Filterd-Recvd-Size: 5514 Date: Fri, 3 Nov 2017 07:21:21 -0400 From: Steven Rostedt To: John Hubbard Cc: Vlastimil Babka , LKML , Peter Zijlstra , , , Cong Wang , Dave Hansen , Johannes Weiner , Mel Gorman , Michal Hocko , Petr Mladek , Sergey Senozhatsky , "yuwang.yuwang" , Linus Torvalds , Jan Kara , Mathieu Desnoyers , Tetsuo Handa Subject: Re: [PATCH v3] printk: Add console owner and waiter logic to load balance console writes Message-ID: <20171103072121.3c2fd5ab@vmware.local.home> In-Reply-To: <9f3bbbab-ef58-a2a6-d4c5-89e62ade34f8@nvidia.com> References: <20171102134515.6eef16de@gandalf.local.home> <82a3df5e-c8ad-dc41-8739-247e5034de29@suse.cz> <9f3bbbab-ef58-a2a6-d4c5-89e62ade34f8@nvidia.com> X-Mailer: Claws Mail 3.15.1-dirty (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2 Nov 2017 21:09:32 -0700 John Hubbard wrote: > 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 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. -- Steve > > 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. >