[PATCH net-next] net_sched: restore "mpu xxx" handling

[PATCH net-next] net_sched: restore "mpu xxx" handling

[Kevin Bracey]

commit 56b765b79e9a ("htb: improved accuracy at high rates") broke
"overhead X", "linklayer atm" and "mpu X" attributes.

"overhead X" and "linklayer atm" have already been fixed. This restores
the "mpu X" handling, as might be used by DOCSIS or Ethernet shaping:

    tc class add ... htb rate X overhead 4 mpu 64

The code being fixed is used by htb, tbf and act_police. Cake has its
own mpu handling. qdisc_calculate_pkt_len still uses the size table
containing values adjusted for mpu by user space.

Fixes: 56b765b79e9a ("htb: improved accuracy at high rates")
Signed-off-by: Kevin Bracey <kevin@bracey.fi>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Vimalkumar <j.vimal@gmail.com>
---
 include/net/sch_generic.h | 5 +++++
 net/sched/sch_generic.c   | 1 +
 2 files changed, 6 insertions(+)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c70e6d2b2fdd..fddca0aa73ef 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1244,6 +1244,7 @@ struct psched_ratecfg {
 	u64	rate_bytes_ps; /* bytes per second */
 	u32	mult;
 	u16	overhead;
+	u16	mpu;
 	u8	linklayer;
 	u8	shift;
 };
@@ -1253,6 +1254,9 @@ static inline u64 psched_l2t_ns(const struct psched_ratecfg *r,
 {
 	len += r->overhead;
 
+	if (len < r->mpu)
+		len = r->mpu;
+
 	if (unlikely(r->linklayer == TC_LINKLAYER_ATM))
 		return ((u64)(DIV_ROUND_UP(len,48)*53) * r->mult) >> r->shift;
 
@@ -1275,6 +1279,7 @@ static inline void psched_ratecfg_getrate(struct tc_ratespec *res,
 	res->rate = min_t(u64, r->rate_bytes_ps, ~0U);
 
 	res->overhead = r->overhead;
+	res->mpu = r->mpu;
 	res->linklayer = (r->linklayer & TC_LINKLAYER_MASK);
 }
 
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 3b0f62095803..5d391fe3137d 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1474,6 +1474,7 @@ void psched_ratecfg_precompute(struct psched_ratecfg *r,
 {
 	memset(r, 0, sizeof(*r));
 	r->overhead = conf->overhead;
+	r->mpu = conf->mpu;
 	r->rate_bytes_ps = max_t(u64, conf->rate, rate64);
 	r->linklayer = (conf->linklayer & TC_LINKLAYER_MASK);
 	psched_ratecfg_precompute__(r->rate_bytes_ps, &r->mult, &r->shift);
-- 
2.25.1

Re: [PATCH net-next] net_sched: restore "mpu xxx" handling

[Jakub Kicinski]

On Fri, 7 Jan 2022 22:22:50 +0200 Kevin Bracey wrote:
> commit 56b765b79e9a ("htb: improved accuracy at high rates") broke
> "overhead X", "linklayer atm" and "mpu X" attributes.
> 
> "overhead X" and "linklayer atm" have already been fixed. This restores
> the "mpu X" handling, as might be used by DOCSIS or Ethernet shaping:
> 
>     tc class add ... htb rate X overhead 4 mpu 64
> 
> The code being fixed is used by htb, tbf and act_police. Cake has its
> own mpu handling. qdisc_calculate_pkt_len still uses the size table
> containing values adjusted for mpu by user space.
> 
> Fixes: 56b765b79e9a ("htb: improved accuracy at high rates")

Are you sure this worked and got broken? I can't seem to grep out any
uses of mpu in this code. commit 175f9c1bba9b ("net_sched: Add size
table for qdiscs") adds it as part of the struct but I can't find a
single use of it.

Re: [PATCH net-next] net_sched: restore "mpu xxx" handling

[Kevin Bracey]

On 12/01/2022 07:06, Jakub Kicinski wrote:
> On Fri, 7 Jan 2022 22:22:50 +0200 Kevin Bracey wrote:
>> commit 56b765b79e9a ("htb: improved accuracy at high rates") broke
>> "overhead X", "linklayer atm" and "mpu X" attributes.
>>
>> "overhead X" and "linklayer atm" have already been fixed. This restores
>> the "mpu X" handling, as might be used by DOCSIS or Ethernet shaping:
>>
>>      tc class add ... htb rate X overhead 4 mpu 64
>>
>> The code being fixed is used by htb, tbf and act_police. Cake has its
>> own mpu handling. qdisc_calculate_pkt_len still uses the size table
>> containing values adjusted for mpu by user space.
>>
>> Fixes: 56b765b79e9a ("htb: improved accuracy at high rates")
> Are you sure this worked and got broken? I can't seem to grep out any
> uses of mpu in this code. commit 175f9c1bba9b ("net_sched: Add size
> table for qdiscs") adds it as part of the struct but I can't find a
> single use of it.

Indeed, There has never been any kernel handling of tc_ratespec::mpu - 
the kernel merely stored the value.

But the system functionality worked because the length-to-time that tc 
passed to TCA_HTB_RTAB was based on the mpu value.

Since 56b765b79e9a ("htb: improved accuracy at high rates"), the table 
has been ignored, and the kernel has done its own immediate calculations.

So this would be the first time the kernel itself has acted on the 
tc_ratespec::mpu value. But it echoes the changes made in 01cb71d2d47b 
("net_sched: restore "overhead xxx" handling") and 8a8e3d84b171 
("net_sched: restore "linklayer atm" handling")

The overhead had been similarly passed to the kernel but not originally 
acted on. Linklayer had to be added to tc_ratespec.

I noticed this because I'd been messing with the htbs on a 2.6.26-based 
router, and when I migrated to a 4.1-based one it was obvious the mpu 
values set in tc weren't sticking. Inspection showed that not only was 
the kernel not storing them, it wasn't any longer using the table based 
on them, so they were ineffective.

Kevin

Re: [PATCH net-next] net_sched: restore "mpu xxx" handling

[Kevin Bracey]

On 12/01/2022 08:36, Kevin Bracey wrote:
>
> Indeed, There has never been any kernel handling of tc_ratespec::mpu - 
> the kernel merely stored the value.
>
> The overhead had been similarly passed to the kernel but not 
> originally acted on. Linklayer had to be added to tc_ratespec. 

Ah, I need to correct myself there. The overhead was originally acted on 
in qdisc_l2t. htb_l2t forgot to incorporate it.

So:

  * overhead - always passed via tc_ratespec, handled by kernel. HTB
    temporarily ignored it.
  * linklayer - not originally passed via tc_ratespec, but incorporated
    in table. HTB temporarily lost functionality when it stopped using
    table. Later passed via ratespec, or inferred from table analysis
    for old iproute2.
  * mpu - always passed via tc_ratespec, but ignored by kernel.
    Incorporated in table. HTB lost functionality when it stopped using
    table.

("always" meaning "since iproute2 first had the parameter").

So this is a tad different from the other two - those were making the 
kernel act on something it previously acted on. This makes it act on 
something it's always been given, but never acted on. But it restores 
iproute2+kernel system functionality with no userspace change.

Kevin

Re: [PATCH net-next] net_sched: restore "mpu xxx" handling

[Jakub Kicinski]

On Wed, 12 Jan 2022 09:02:59 +0200 Kevin Bracey wrote:
> On 12/01/2022 08:36, Kevin Bracey wrote:
> >
> > Indeed, There has never been any kernel handling of tc_ratespec::mpu - 
> > the kernel merely stored the value.
> >
> > The overhead had been similarly passed to the kernel but not 
> > originally acted on. Linklayer had to be added to tc_ratespec.   
> 
> Ah, I need to correct myself there. The overhead was originally acted on 
> in qdisc_l2t. htb_l2t forgot to incorporate it.
> 
> So:
> 
>   * overhead - always passed via tc_ratespec, handled by kernel. HTB
>     temporarily ignored it.
>   * linklayer - not originally passed via tc_ratespec, but incorporated
>     in table. HTB temporarily lost functionality when it stopped using
>     table. Later passed via ratespec, or inferred from table analysis
>     for old iproute2.
>   * mpu - always passed via tc_ratespec, but ignored by kernel.
>     Incorporated in table. HTB lost functionality when it stopped using
>     table.
> 
> ("always" meaning "since iproute2 first had the parameter").
> 
> So this is a tad different from the other two - those were making the 
> kernel act on something it previously acted on. This makes it act on 
> something it's always been given, but never acted on. But it restores 
> iproute2+kernel system functionality with no userspace change.

I see, thanks for the explanation! I think it's worth adding this
analysis to the commit message, please repost.