Re: [PATCH v12 1/1] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On 09.03.2021 21:45:58, Vincent MAILHOL wrote:
> > On 3/8/21 5:34 PM, Vincent Mailhol wrote:
> > > This driver supports the ES581.4, ES582.1 and ES584.1 interfaces from
> > > ETAS GmbH (https://www.etas.com/en/products/es58x.php).
> > >
> > > Co-developed-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> > > Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > I'm not sure if you're supposed to change dql.min_limit from the driver.
> 
> One thing for sure, I am the only one to do it.
> 
> The reason to do so is because benchmarks show me that values
> below this threshold are not good for this device (and I try to
> be very permissive on the values).
> 
> USB introduces a lot of latency and the small PDU of CAN does not
> help. The BQL is here to remediate, however, the algorithms can
> take time to adjust, especially if there are small bursts.
> Modifying the dql.min_limit was the only solution I found to make
> sure that packets can be sent in bulk even during small burst
> events.
> 
> The BQL was not designed for USB nor was it designed for CAN
> which probably explains why I am the first one to ever have
> thought of using dql.min_limit like this.

We can try to sneak it into the kernel or ask on the net-dev list for a
proper interface[1] for setting sensible defaults to the min_limit.

> Using dql.min_limit is a hack and I pledge guilty for it. However,
> because this hack brings performance improvement, I would like to keep
> it if you do not mind.

Your explanation is very good and clear, what about sending new mail
with this problem description to the netdev list? A proper solution
might be something like dql_set_min_limit() with a static inline no-op
of BQL is disabled.

Looking at 114cf5802165 ("bql: Byte queue limits"), you want to include:
- Tom Herbert <therbert@google.com>
- Eric Dumazet <eric.dumazet@gmail.com>

Marc

[1] Having ifdefs to set the value in the driver is clearly a sign of
some misuse :)

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]