Re: [PATCH v2] cxl/mbox: Add background cmd handling machinery

[Dan Williams <dan.j.williams@intel.com>]

Davidlohr Bueso wrote:
> On Mon, 22 May 2023, Dan Williams wrote:
> 
> >Davidlohr Bueso wrote:
> >> It at least removes one point of potential disambiguity. This out of sync
> >> issue with timeouts was one of the reasons I originally proposed dealing
> >> with all bg commands asynchronously (which doesn't prevent users from still
> >> time-slicing). This approach is supposed to make the code simpler, but leaves
> >> this mess. Currently we state:
> >>
> >>	 * In the event of timeout, the mailbox state is
> >>	 * indeterminate until the next successful command submission and the
> >>	 * driver can get back in sync with the hardware state.
> >
> >Right, but my struggle is how to explain to userspace what to do next.
> >Does userspace keep submitting until it gets a successful acceptance?
> 
> Yes. It would be the users' responsibility to deal with timeouts.
> 
> >How does userspace know that its failure is induced by background
> >command collision?
> 
> Knowing about the apriori timeout, would the user care?

It's not just timeouts its also SIGTERM. If the user is allowed to say
"I do not care about the previous completion" then the driver should have
some way to get back in sync. Either the next submission needs to wait,
or there needs to be a facility to flush previous commands.

> >That's why I think it is ok to block and wait on the previous background
> >command completion when the next command is submitted. That seems to satisfy
> >least surprise. The submitter asked for the last command to end in
> >failure and is asking for the next one to queue up and complete.
> >
> >I think this means that cxl_mbox_background_complete() needs to be called
> >before every submission and needs to check for "background operation"
> >(mailbox status bit0) being cleared to manage potential collisions.
> 
> All this really defeats the "simpler" angle. Without accepting that it's
> the user who needs to resubmit until in sync, it seems doing synchronously
> is sw shooting itself in the foot.

Make it as simple as possible, but no simpler. The driver saying "you
are on your own after first cancellation or timeout" is too simple.
Especially when the driver designed with an eye towards not allowing
long running background commands be submitted (outside of the special
sanitization consideration).

So the choices are either allow the background command wait to be
interruptible and then do an implied sync on the next submission, or
make it uninterruptible where timeouts are either a real device problem
or a driver issue that needs to reduce the size of the background
operation to fit the timeout.

This middle ground of allowing SIGTERM and then userspace is on its own
to repair the connection violates least surprise.