On Wed. 10 Mar 2021 at 00:35, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
>
> On 09.03.2021 22:10:08, Vincent MAILHOL wrote:
> > Sounds good to me. I will prepare a patch to explain the issue
> > and try to introduce the dql_set_min_limit() function.
> >
> > Meanwhile, I would be thankful if you could continue the review :)
>
> Thanks for the mail, looks good.
>
> One note for the patch, though:
>
> > diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h
> > index 407c2f281b64..32437f168a35 100644
> > --- a/include/linux/dynamic_queue_limits.h
> > +++ b/include/linux/dynamic_queue_limits.h
> > @@ -103,6 +103,9 @@ void dql_reset(struct dql *dql);
> > /* Initialize dql state */
> > void dql_init(struct dql *dql, unsigned int hold_time);
> >
> > +/* Set the dql minimum limit */
> #ifdef CONFIG_DQL
> > +void dql_set_min_limit(struct dql *dql, unsigned int min_limit);
> #else
> static inline void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
> {
> }
> #endif
> > +
> > #endif /* _KERNEL_ */
> >
> > #endif /* _LINUX_DQL_H */
> > diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
> > index fde0aa244148..8b6ad1e0a2e3 100644
> > --- a/lib/dynamic_queue_limits.c
> > +++ b/lib/dynamic_queue_limits.c
>
> This file is only compiled if CONFIG_DQL is set, see lib/Makefile:
>
> | obj-$(CONFIG_DQL) += dynamic_queue_limits.o
Got it.
> > @@ -136,3 +136,11 @@ void dql_init(struct dql *dql, unsigned int hold_time)
> > dql_reset(dql);
> > }
> > EXPORT_SYMBOL(dql_init);
> > +
> > +void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
> > +{
> > +#ifdef CONFIG_BQL
>
> remove this ifdef
>
> > + dql->min_limit = min_limit;
> > +#endif
> > +}
> > +EXPORT_SYMBOL(dql_set_min_limit);
Actually, after doing a few more tests, this is a bit more complicated
than anticipated.
The dql member of struct netdev_queue is also guarded by a #ifdef CONFIG_BQL:
https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L629
This means that under the current idea, we would also need to guard
the call to dql_set_min_limit():
#ifdef CONFIG_BQL
dql_set_min_limit(&netdev_get_tx_queue(netdev, 0)->dql,
es58x_dev->param->dql_limit_min);
#ifdef CONFIG_BQL
This kills the initial intent of not using the #ifdef CONFIG_BQL to
set the value.
This leads to the need to do:
void netdev_queue_set_dql_min_limit(struct netdev_queue *q, unsigned
int min_limit)
{
#ifdef CONFIG_BQL
q->dql.min_limit = min_limit;
#endif
}
which would probably be inside /include/linux/netdevice.h.
Does it make sense?
Yours sincerely,
Vincent
