From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ozlabs.org (bilbo.ozlabs.org [203.11.71.1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 41lst56GrtzDqgZ for ; Wed, 8 Aug 2018 23:38:53 +1000 (AEST) From: Michael Ellerman To: John Allen , nfont@linux.vnet.ibm.com Cc: linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v2 2/2] powerpc/pseries: Wait for completion of hotplug events during PRRN handling In-Reply-To: <20180807192628.uf3nycnagabjs55o@p50.austin.ibm.com> References: <20180717194048.3057-1-jallen@linux.ibm.com> <20180717194048.3057-3-jallen@linux.ibm.com> <87k1pmhxx7.fsf@concordia.ellerman.id.au> <20180723152223.mydr5dovcv26domv@p50> <87in4ufcrd.fsf@concordia.ellerman.id.au> <20180807192628.uf3nycnagabjs55o@p50.austin.ibm.com> Date: Wed, 08 Aug 2018 23:38:52 +1000 Message-ID: <878t5hasgj.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , John Allen writes: > On Wed, Aug 01, 2018 at 11:16:22PM +1000, Michael Ellerman wrote: >>John Allen writes: >>> On Mon, Jul 23, 2018 at 11:41:24PM +1000, Michael Ellerman wrote: >>>>John Allen writes: >>>> >>>>> While handling PRRN events, the time to handle the actual hotplug events >>>>> dwarfs the time it takes to perform the device tree updates and queue the >>>>> hotplug events. In the case that PRRN events are being queued continuously, >>>>> hotplug events have been observed to be queued faster than the kernel can >>>>> actually handle them. This patch avoids the problem by waiting for a >>>>> hotplug request to complete before queueing more hotplug events. >> >>Have you tested this patch in isolation, ie. not with patch 1? > > While I was away on vacation, I believe a build was tested with just > this patch and not the first and it has been running with no problems. > However, I think they've had problems recreating the problem in general > so it may just be that the environment is not setup properly to recreate > the issue. Yes I asked Haren to test it :) >>From memory there were some warnings still about the work queue being blocked for long periods, but they weren't fatal. >>>>So do we need the hotplug work queue at all? Can we just call >>>>handle_dlpar_errorlog() directly? >>>> >>>>Or are we using the work queue to serialise things? And if so would a >>>>mutex be better? >>> >>> Right, the workqueue is meant to serialize all hotplug events and it >>> gets used for more than just PRRN events. I believe the motivation for >>> using the workqueue over a mutex is that KVM guests initiate hotplug >>> events through the hotplug interrupt and can queue fairly large requests >>> meaning that in this scenario, waiting for a lock would block interrupts >>> for a while. >> >>OK, but that just means that path needs to schedule work to run later. >> >>> Using the workqueue allows us to serialize hotplug events >>> from different sources in the same way without worrying about the >>> context in which the event is generated. >> >>A lock would be so much simpler. >> >>It looks like we have three callers of queue_hotplug_event(), the dlpar >>code, the mobility code and the ras interrupt. >> >>The dlpar code already waits synchronously: >> >> init_completion(&hotplug_done); >> queue_hotplug_event(hp_elog, &hotplug_done, &rc); >> wait_for_completion(&hotplug_done); >> >>You're changing mobility to do the same (this patch), leaving only the >>ras interrupt that actually queues work and returns. >> >> >>So it really seems like a mutex would do the trick, and the ras >>interrupt would be the only case that needs to schedule work for later. > > I think you may be right, but I would need some feedback from Nathan > Fontenot before I redesign the queue. He's been thinking about that > design for longer than I have and may know something that I don't > regarding the reason we're using a workqueue rather than a mutex. > Given that the bug this is meant to address is pretty high priority, > would you consider the wait_for_completion an acceptable stopgap while a > more substantial redesign of this code is discussed? Yeah that would be OK. cheers