Re: [PATCH 3/3] scsi/ncr5380: Improve interrupt latency during PIO tranfers

[Finn Thain <fthain@telegraphics.com.au>]

On Sun, 28 Aug 2016, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Sat, Aug 27, 2016 at 4:30 AM, Finn Thain <fthain@telegraphics.com.au> 
> wrote:
> > Large PIO transfers are broken up into chunks to try to avoid 
> > disabling local IRQs for long periods. But IRQs are still disabled for 
> > too long and this causes SCC FIFO overruns during serial port 
> > transfers. This patch fixes the problem by halving the PIO chunk size.
> >
> > Testing with mac_scsi shows that the extra NCR5380_main() loop 
> > iterations have negligible performance impact on SCSI transfers (about 
> > 1% slower). On a faster system (using the dmx3191d module) transfers 
> > showed no measurable change.
> >
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> >
> > ---
> >  drivers/scsi/NCR5380.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > Index: linux/drivers/scsi/NCR5380.c
> > ===================================================================
> > --- linux.orig/drivers/scsi/NCR5380.c   2016-08-27 12:29:57.000000000 +1000
> > +++ linux/drivers/scsi/NCR5380.c        2016-08-27 12:29:58.000000000 +1000
> > @@ -1847,11 +1847,11 @@ static void NCR5380_information_transfer
> >                                                 /* XXX - need to source or sink data here, as appropriate */
> >                                         }
> >                                 } else {
> > -                                       /* Break up transfer into 3 ms chunks,
> > -                                        * presuming 6 accesses per handshake.
> > +                                       /* Transfer a small chunk so that the
> > +                                        * irq mode lock is not held too long.
> >                                          */
> >                                         transfersize = min((unsigned long)cmd->SCp.this_residual,
> > -                                                          hostdata->accesses_per_ms / 2);
> > +                                                          hostdata->accesses_per_ms >> 2);
> 
> I think it's easier to read if you use "/ 4".

I think the factor, "1/4 byte milliseconds per access" is not very 
meaningful. The PIO transfersize can be understood as,

        pio_bytes_until_scc_fifo_overflow = accesses_per_ms /
                         (accesses_per_pio_byte / ms_until_fifo_overflow)

This loop seemed like a good place to avoid a DIV instruction (though I 
didn't try to confirm that) and so I used a bit shift to indicate that 
intention.

The shift amount was an empirical result that happened to work for the 
hardware I tested it on, at the baud rate I was using. Admittedly, if we 
want to avoid further tweaks to this then I'll have to do more testing and 
find a better approximation.

-- 

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>