Re: [PATCH] remoteproc: core: Invoke subdev callbacks in list order

On Tue 25 May 18:54 CDT 2021, Saravana Kannan wrote:

On XXXXX, Siddharth Gupta wrote:

quoted

On 5/24/2021 8:03 PM, Bjorn Andersson wrote:

quoted

On Mon 17 May 18:08 CDT 2021, Siddharth Gupta wrote:

quoted

Subdevices at the beginning of the subdev list should have
higher priority than those at the end of the list. Reverse
traversal of the list causes priority inversion, which can
impact the performance of the device.

The subdev lists layers of the communication onion, we bring them up
inside out and we take them down outside in.

This stems from the primary idea that we want to be able to shut things
down cleanly (in the case of a stop) and we pass the "crashed" flag to
indicate to each recipient during "stop" that it may not rely on the
response of a lower layer.

As such, I don't think it's right to say that we have a priority
inversion.

My understanding of the topic was that each subdevice should be
independent of the other. In our case unfortunately the sysmon
subdevice depends on the glink endpoint.

In that case, the glink has to be prepared/started before sysmon, right?

Correct, we prepare glink, then prepare sysmon, start glink then start
sysmon - and reverse for stop and unprepare.

quoted

However the priority inversion doesn't happen in these
subdevices, it happens due to the SSR notifications that we send
to kernel clients. In this case kernel clients also can have QMI
sockets that in turn depend on the glink endpoint, which means
when they go to release the QMI socket a broadcast will be sent
out to all connected clients about the closure of the connection
which in this case happens to be the remoteproc which died. So
if we peel the onion, we will be unnecessarily be waiting for a
dead remoteproc.

So why can't the QMI layer be smart about this and check that the
remoteproc hasn't crashed before you try to communicate with it?

I guess we could do that, if we really have to. But I find it quite
ugly and would like to avoid it.

Or if the
glink is torn down before QMI gets to broadcast, then it's a pretty clear
indication of failure and just notify all the kernel side QMI clients?

No, the system is designed to deal with this; as the remoteproc goes
down glink will be torn down, which will team down the qrtr link to
whatever qrtr nodes exist on (or beyond) that remote processor.

So if it's down the qrtr will naturally fail because there's no path to
that qrtr node.

quoted

quoted

quoted

For example a device adds the glink, sysmon and ssr subdevs
to its list. During a crash the ssr notification would go
before the glink and sysmon notifications. This can cause a
degraded response when a client driver waits for a response
from the crashed rproc.

In general the design is such that components are not expected to
communicate with the crashed remote when "crashed" is set, this avoids
the single-remote crash.

Here the glink device on the rpmsg bus won't know about the
crashed remoteproc till we send glink notification first, right?

Why not just query the current state of the remote proc before trying to
talk to it? It should be a quick check.

We notify subdevices (and thereby indirectly other drivers) that the
remoteproc is going down, either cleanly or that it's dead.

The problem seen here is that when remoteproc tell some component that
the particular remote processor is dead (crashed/not going to respond)
they react by attempting to communicate with the dying remote processor
- which will naturally time out.

In the general case the solution is simply to stop communicate with the
remote when you're told it's dead. The question is what kind of implicit
operations we're seeing here.

quoted

Since we send out sysmon and SSR notifications first, the glink
device will still be "alive" on the rpmsg bus.

quoted

The case where this isn't holding up is when two remote processors
crashes simultaneously, in which case e.g. sysmon has been seen hitting
its timeout waiting for an ack from a dead remoteproc - but I was under
the impression that this window shrunk dramatically as a side effect of
us fixing the notification ordering.

You are right, the window would become smaller in the case of two
remoteprocs, but this issue can come up with even a single
remoteproc unless prioritize certain subdevices.

I think the main problem you have here is rproc sub devices that depend on
other rproc sub devices. But there's no dependency tracking here. Your
change just happens to work for your specific case because the order of the
sub devices in the list happens to work for your inter-subdevice
dependencies. But this is definitely not going to work for all users of
subdevices.

Right, in the particular case I'm talking about here we saw two remote
processors dying concurrently and ended up in sysmon with each one
trying to notify the other about the change in status. But as I said, to
a large degree this has been avoided by making sure that sysmon checks
the status of the remoteproc before attempting to send. It is however
still possible that you get past this check before the recipient of your
notification dies, in which case you would end up having to wait out the
timeout.

It might be possible to complete the process waiting for a response in
this case, but I don't have any data indicating if it's worth it.

And more importantly, this is not the problem that Siddharth is
reporting.

If keeping track of dependency is too much complexity (I haven't read
enough rproc code to comment on that), at the least, it looks like you need
another ops instead of changing the order of stop() callbacks. Or at a
minimum pick the ordering based on the "crashed" flag. A blanket, I'll just
switch the ordering of stop() for everyone for all cases is wrong.

I unfortunately don't see which problem you're trying to solve, above
looks to me like an extreme micro-optimization and has nothing to do
with dependencies.

In fact, in the normal/clean shutdown case, I'd think you'll want to stop
the subdevices in reverse initialization order so that you can cleanly stop
QMI/sysmon first before shutting down glink.

Yes.

Regards,
Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel