On Wed, Aug 01, 2018 at 11:16:22PM +1000, Michael Ellerman wrote:
John Allen <jal...@linux.ibm.com> writes:
On Mon, Jul 23, 2018 at 11:41:24PM +1000, Michael Ellerman wrote:
John Allen <jal...@linux.ibm.com> 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.
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?
-John
