RE: [PATCH v2 2/5] ntb_perf: send command in response to EAGAIN

[Nath, Arindam]

> -----Original Message-----
> From: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> Sent: Wednesday, March 11, 2020 03:01
> To: Mehta, Sanju <Sanju.Mehta@xxxxxxx>; jdmason@xxxxxxxx;
> dave.jiang@xxxxxxxxx; allenbh@xxxxxxxxx; Nath, Arindam
> <Arindam.Nath@xxxxxxx>; S-k, Shyam-sundar <Shyam-sundar.S-
> k@xxxxxxx>
> Cc: linux-ntb@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/5] ntb_perf: send command in response to EAGAIN
> 
> 
> 
> On 2020-03-10 2:54 p.m., Sanjay R Mehta wrote:
> > From: Arindam Nath <arindam.nath@xxxxxxx>
> >
> > perf_spad_cmd_send() and perf_msg_cmd_send() return
> > -EAGAIN after trying to send commands for a maximum
> > of MSG_TRIES re-tries. But currently there is no
> > handling for this error. These functions are invoked
> > from perf_service_work() through function pointers,
> > so rather than simply call these functions is not
> > enough. We need to make sure to invoke them again in
> > case of -EAGAIN. Since peer status bits were cleared
> > before calling these functions, we set the same status
> > bits before queueing the work again for later invocation.
> > This way we simply won't go ahead and initialize the
> > XLAT registers wrongfully in case sending the very first
> > command itself fails.
> 
> So what happens if there's an actual non-recoverable error that causes
> perf_msg_cmd_send() to fail? Are you proposing it just requeues high
> priority work forever?

The intent of the patch is to handle -EAGAIN, since the error code is
an indication that we need to try again later. Currently there is a very
small time frame during which ntb_perf should be loaded on both sides
(primary and secondary) to have XLAT registers configured correctly.
Failing that the code will still fall through without properly initializing the
XLAT registers and there is no indication of that either until we have
actually tried to perform 'echo 0 > /sys/kernel/debug/.../run'.

With the changes proposed in this patch, we do not have to depend
on whether the drivers at both ends are loaded within a fixed time
duration. So we can simply load the driver at one side, and at a later
time load the driver on the other, and still the XLAT registers would
be set up correctly.

Looking at perf_spad_cmd_send() and perf_msg_cmd_send(), if the
concern is that ntb_peer_spad_read()/ntb_msg_read_sts() fail because
of some non-recoverable error and we still schedule a high priority
work, that is a valid concern. But isn't it still better than simply falling
through and initializing XLAT register with incorrect values?

> 
> I never really reviewed this stuff properly but it looks like it has a
> bunch of problems. Using the high priority work queue for some low
> priority setup work seems wrong, at the very least. The spad and msg
> send loops also look like they have a bunch of inter-host race condition
> problems as well. Yikes.

I am not very sure what the design considerations were when having
a high priority work queue, but perhaps we can all have a discussion
on this.

Arindam

> 
> Logan
> 
> 
> 
> > Signed-off-by: Arindam Nath <arindam.nath@xxxxxxx>
> > Signed-off-by: Sanjay R Mehta <sanju.mehta@xxxxxxx>
> > ---
> >  drivers/ntb/test/ntb_perf.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
> > index 6d16628..9068e42 100644
> > --- a/drivers/ntb/test/ntb_perf.c
> > +++ b/drivers/ntb/test/ntb_perf.c
> > @@ -625,14 +625,24 @@ static void perf_service_work(struct work_struct
> *work)
> >  {
> >  	struct perf_peer *peer = to_peer_service(work);
> >
> > -	if (test_and_clear_bit(PERF_CMD_SSIZE, &peer->sts))
> > -		perf_cmd_send(peer, PERF_CMD_SSIZE, peer-
> >outbuf_size);
> > +	if (test_and_clear_bit(PERF_CMD_SSIZE, &peer->sts)) {
> > +		if (perf_cmd_send(peer, PERF_CMD_SSIZE, peer-
> >outbuf_size)
> > +		    == -EAGAIN) {
> > +			set_bit(PERF_CMD_SSIZE, &peer->sts);
> > +			(void)queue_work(system_highpri_wq, &peer-
> >service);
> > +		}
> > +	}
> >
> >  	if (test_and_clear_bit(PERF_CMD_RSIZE, &peer->sts))
> >  		perf_setup_inbuf(peer);
> >
> > -	if (test_and_clear_bit(PERF_CMD_SXLAT, &peer->sts))
> > -		perf_cmd_send(peer, PERF_CMD_SXLAT, peer->inbuf_xlat);
> > +	if (test_and_clear_bit(PERF_CMD_SXLAT, &peer->sts)) {
> > +		if (perf_cmd_send(peer, PERF_CMD_SXLAT, peer-
> >inbuf_xlat)
> > +		    == -EAGAIN) {
> > +			set_bit(PERF_CMD_SXLAT, &peer->sts);
> > +			(void)queue_work(system_highpri_wq, &peer-
> >service);
> > +		}
> > +	}
> >
> >  	if (test_and_clear_bit(PERF_CMD_RXLAT, &peer->sts))
> >  		perf_setup_outbuf(peer);
> >