All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v2] net_sched: restore "mpu xxx" handling
@ 2022-01-12 17:02 Kevin Bracey
  2022-01-12 17:08 ` Eric Dumazet
  0 siblings, 1 reply; 4+ messages in thread
From: Kevin Bracey @ 2022-01-12 17:02 UTC (permalink / raw)
  To: netdev; +Cc: kuba, Kevin Bracey, Eric Dumazet, Jiri Pirko, Vimalkumar

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.

iproute2 tc has always passed mpu into the kernel via a tc_ratespec
structure, but the kernel never directly acted on it, merely stored it
so that it could be read back by `tc class show`.

Rather, tc would generate length-to-time tables that included the mpu
(and linklayer) in their construction, and the kernel used those tables.

Since v3.7, the tables were no longer used. Along with "mpu", this also
broke "overhead" and "linklayer" which were fixed in 01cb71d2d47b
("net_sched: restore "overhead xxx" handling", v3.10) and 8a8e3d84b171
("net_sched: restore "linklayer atm" handling", v3.11).

"overhead" was fixed by simply restoring use of tc_ratespec::overhead -
this had originally been used by the kernel but was initially omitted
from the new non-table-based calculations.

"linklayer" had been handled in the table like "mpu", but the mode was
not originally passed in tc_ratespec. The new implementation was made to
handle it by getting new versions of tc to pass the mode in an extended
tc_ratespec, and for older versions of tc the table contents were analysed
at load time to deduce linklayer.

As "mpu" has always been given to the kernel in tc_ratespec,
accompanying the mpu-based table, we can restore system functionality
with no userspace change by making the kernel act on the tc_ratespec
value.

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net-next v2] net_sched: restore "mpu xxx" handling
  2022-01-12 17:02 [PATCH net-next v2] net_sched: restore "mpu xxx" handling Kevin Bracey
@ 2022-01-12 17:08 ` Eric Dumazet
  2022-01-12 17:31   ` Kevin Bracey
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2022-01-12 17:08 UTC (permalink / raw)
  To: Kevin Bracey; +Cc: netdev, Jakub Kicinski, Jiri Pirko, Vimalkumar

On Wed, Jan 12, 2022 at 9:02 AM Kevin Bracey <kevin@bracey.fi> 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.
>
> iproute2 tc has always passed mpu into the kernel via a tc_ratespec
> structure, but the kernel never directly acted on it, merely stored it
> so that it could be read back by `tc class show`.
>
> Rather, tc would generate length-to-time tables that included the mpu
> (and linklayer) in their construction, and the kernel used those tables.
>
> Since v3.7, the tables were no longer used. Along with "mpu", this also
> broke "overhead" and "linklayer" which were fixed in 01cb71d2d47b
> ("net_sched: restore "overhead xxx" handling", v3.10) and 8a8e3d84b171
> ("net_sched: restore "linklayer atm" handling", v3.11).
>
> "overhead" was fixed by simply restoring use of tc_ratespec::overhead -
> this had originally been used by the kernel but was initially omitted
> from the new non-table-based calculations.
>
> "linklayer" had been handled in the table like "mpu", but the mode was
> not originally passed in tc_ratespec. The new implementation was made to
> handle it by getting new versions of tc to pass the mode in an extended
> tc_ratespec, and for older versions of tc the table contents were analysed
> at load time to deduce linklayer.
>
> As "mpu" has always been given to the kernel in tc_ratespec,
> accompanying the mpu-based table, we can restore system functionality
> with no userspace change by making the kernel act on the tc_ratespec
> value.
>
> 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>

Thanks for the nice changelog.

I do have a question related to HTB offload.

Is this mpu attribute considered there ?

Thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net-next v2] net_sched: restore "mpu xxx" handling
  2022-01-12 17:08 ` Eric Dumazet
@ 2022-01-12 17:31   ` Kevin Bracey
  2022-01-13 19:08     ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: Kevin Bracey @ 2022-01-12 17:31 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Jakub Kicinski, Jiri Pirko, Vimalkumar

On 12/01/2022 19:08, Eric Dumazet wrote:
> On Wed, Jan 12, 2022 at 9:02 AM Kevin Bracey <kevin@bracey.fi> wrote:
>> commit 56b765b79e9a ("htb: improved accuracy at high rates") broke
>> "overhead X", "linklayer atm" and "mpu X" attributes.
> Thanks for the nice changelog.
>
> I do have a question related to HTB offload.
>
> Is this mpu attribute considered there ?

I had not considered that. None of these 3 adjustment parameters are 
passed to its setup - tc_htb_qopt_offload merely contains u64 rate and ceil.

Dealing with that looks like it might be a bit fiddly. htb_change_class 
would need reordering. It appears the software case permits parameter 
changing and it is ordered on that basis, but the offload locks in 
parameters on creation.

But in principle, tc_htb_qopt_offload could contain a pair of 
psched_ratecfg rather than a pair of u64s, and then offload would have 
all the adjustment information.



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net-next v2] net_sched: restore "mpu xxx" handling
  2022-01-12 17:31   ` Kevin Bracey
@ 2022-01-13 19:08     ` Jakub Kicinski
  0 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2022-01-13 19:08 UTC (permalink / raw)
  To: Kevin Bracey; +Cc: Eric Dumazet, netdev, Jiri Pirko, Vimalkumar, maximmi

On Wed, 12 Jan 2022 19:31:54 +0200 Kevin Bracey wrote:
> On 12/01/2022 19:08, Eric Dumazet wrote:
> > On Wed, Jan 12, 2022 at 9:02 AM Kevin Bracey <kevin@bracey.fi> wrote:  
> >> commit 56b765b79e9a ("htb: improved accuracy at high rates") broke
> >> "overhead X", "linklayer atm" and "mpu X" attributes.  

Applied, thanks!

> > Thanks for the nice changelog.
> >
> > I do have a question related to HTB offload.
> >
> > Is this mpu attribute considered there ?  
> 
> I had not considered that. None of these 3 adjustment parameters are 
> passed to its setup - tc_htb_qopt_offload merely contains u64 rate and ceil.
> 
> Dealing with that looks like it might be a bit fiddly. htb_change_class 
> would need reordering. It appears the software case permits parameter 
> changing and it is ordered on that basis, but the offload locks in 
> parameters on creation.
> 
> But in principle, tc_htb_qopt_offload could contain a pair of 
> psched_ratecfg rather than a pair of u64s, and then offload would have 
> all the adjustment information.

CC Maxim, offload should at least be rejected if user request
unsupported params.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-01-13 19:08 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12 17:02 [PATCH net-next v2] net_sched: restore "mpu xxx" handling Kevin Bracey
2022-01-12 17:08 ` Eric Dumazet
2022-01-12 17:31   ` Kevin Bracey
2022-01-13 19:08     ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.