RE: [next-queue PATCH v7 4/6] net/sched: Introduce Credit Based Shaper (CBS) qdisc

[Vinicius Costa Gomes]

Hi David,

David Laight  writes:

[...]

>> > index 099bf5528fed..41e349df4bf4 100644
>> > --- a/include/uapi/linux/pkt_sched.h
>> > +++ b/include/uapi/linux/pkt_sched.h
>> > @@ -871,4 +871,22 @@ struct tc_pie_xstats {
>> >__u32 maxq; /* maximum queue size */
>> >__u32 ecn_mark; /* packets marked with ecn*/
>> >  };
>> > +
>> > +/* CBS */
>> > +struct tc_cbs_qopt {
>> > +  __u8 offload;
>
> You probably don't want unnamed padding in a uapi structure.

Yeah, this needs to be fixed.

>
>> > +  __s32 hicredit;
>> > +  __s32 locredit;
>> > +  __s32 idleslope;
>> > +  __s32 sendslope;
>> > +};
>> > +
>> > +enum {
>> > +  TCA_CBS_UNSPEC,
>> > +  TCA_CBS_PARMS,
>> > +  __TCA_CBS_MAX,
>> > +};
>> > +
>> > +#define TCA_CBS_MAX (__TCA_CBS_MAX - 1)
>
> Why not:
>   TCA_CBS_PARMS,
>   TCA_CBS_NEXT,
>   TCA_CBS_MAX = TCA_CBS_NEXT - 1,

The way it is proposed, at least is consistent with the rest of the
file. So, if you don't have any stronger reasons, I'd like to keep it
this way.

>
> ...
>   David


Cheers,
--
Vinicius

RE: [next-queue PATCH v7 4/6] net/sched: Introduce Credit Based Shaper (CBS) qdisc

[David Laight]

From: Ivan Khoronzhuk
> Sent: 13 October 2017 20:59
> On Thu, Oct 12, 2017 at 05:40:03PM -0700, Vinicius Costa Gomes wrote:
> > This queueing discipline implements the shaper algorithm defined by
> > the 802.1Q-2014 Section 8.6.8.2 and detailed in Annex L.
> >
> > It's primary usage is to apply some bandwidth reservation to user
> > defined traffic classes, which are mapped to different queues via the
> > mqprio qdisc.
> >
> > Only a simple software implementation is added for now.
> >
> > Signed-off-by: Vinicius Costa Gomes 
> > Signed-off-by: Jesus Sanchez-Palencia 
> > ---
> >  include/uapi/linux/pkt_sched.h |  18 +++
> >  net/sched/Kconfig  |  11 ++
> >  net/sched/Makefile |   1 +
> >  net/sched/sch_cbs.c| 314 
> > +
> >  4 files changed, 344 insertions(+)
> >  create mode 100644 net/sched/sch_cbs.c
> >
> > diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> > index 099bf5528fed..41e349df4bf4 100644
> > --- a/include/uapi/linux/pkt_sched.h
> > +++ b/include/uapi/linux/pkt_sched.h
> > @@ -871,4 +871,22 @@ struct tc_pie_xstats {
> > __u32 maxq; /* maximum queue size */
> > __u32 ecn_mark; /* packets marked with ecn*/
> >  };
> > +
> > +/* CBS */
> > +struct tc_cbs_qopt {
> > +   __u8 offload;

You probably don't want unnamed padding in a uapi structure.

> > +   __s32 hicredit;
> > +   __s32 locredit;
> > +   __s32 idleslope;
> > +   __s32 sendslope;
> > +};
> > +
> > +enum {
> > +   TCA_CBS_UNSPEC,
> > +   TCA_CBS_PARMS,
> > +   __TCA_CBS_MAX,
> > +};
> > +
> > +#define TCA_CBS_MAX (__TCA_CBS_MAX - 1)

Why not:
TCA_CBS_PARMS,
TCA_CBS_NEXT,
TCA_CBS_MAX = TCA_CBS_NEXT - 1,

...
David

Re: [next-queue PATCH v7 4/6] net/sched: Introduce Credit Based Shaper (CBS) qdisc

[Vinicius Costa Gomes]

Hi,

Ivan Khoronzhuk  writes:

[...]

>> +
>> +static int cbs_enqueue_soft(struct sk_buff *skb, struct Qdisc *sch)
>> +{
>> +struct cbs_sched_data *q = qdisc_priv(sch);
>> +
>> +if (sch->q.qlen == 0 && q->credits > 0) {
>> +/* We need to stop accumulating credits when there's
>> + * no packet enqueued packets and q->credits is
> no packet -> no

Ugh. Fixed.

>
>> + * positive.
>> + */
>> +q->credits = 0;
>> +q->last = ktime_get_ns();
>> +}
>> +
>> +return qdisc_enqueue_tail(skb, sch);
>> +}
>> +

[...]

>> +static struct sk_buff *cbs_dequeue_soft(struct Qdisc *sch)
>> +{
>> +struct cbs_sched_data *q = qdisc_priv(sch);
>> +s64 now = ktime_get_ns();
>> +struct sk_buff *skb;
>> +s64 credits;
>> +int len;
>> +
>> +if (q->credits < 0) {
>> +credits = timediff_to_credits(now - q->last, q->idleslope);
> Maybe be better to add small optimization by moving some calculations from 
> data
> path, I mean, save idle_slope in bytes instead of kbit and converting it for
> every packet. Both delay_from_credits() and timediff_to_credits() is used only
> once and with idle_slope only...and both of them converting it.
>
> Same for credits_from_len() and send slope, save it in units of port_rate.
>

Done. Thanks.


Cheers,
--
Vinicius

Re: [next-queue PATCH v7 4/6] net/sched: Introduce Credit Based Shaper (CBS) qdisc

[Ivan Khoronzhuk]

On Thu, Oct 12, 2017 at 05:40:03PM -0700, Vinicius Costa Gomes wrote:
> This queueing discipline implements the shaper algorithm defined by
> the 802.1Q-2014 Section 8.6.8.2 and detailed in Annex L.
> 
> It's primary usage is to apply some bandwidth reservation to user
> defined traffic classes, which are mapped to different queues via the
> mqprio qdisc.
> 
> Only a simple software implementation is added for now.
> 
> Signed-off-by: Vinicius Costa Gomes 
> Signed-off-by: Jesus Sanchez-Palencia 
> ---
>  include/uapi/linux/pkt_sched.h |  18 +++
>  net/sched/Kconfig  |  11 ++
>  net/sched/Makefile |   1 +
>  net/sched/sch_cbs.c| 314 
> +
>  4 files changed, 344 insertions(+)
>  create mode 100644 net/sched/sch_cbs.c
> 
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 099bf5528fed..41e349df4bf4 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -871,4 +871,22 @@ struct tc_pie_xstats {
>   __u32 maxq; /* maximum queue size */
>   __u32 ecn_mark; /* packets marked with ecn*/
>  };
> +
> +/* CBS */
> +struct tc_cbs_qopt {
> + __u8 offload;
> + __s32 hicredit;
> + __s32 locredit;
> + __s32 idleslope;
> + __s32 sendslope;
> +};
> +
> +enum {
> + TCA_CBS_UNSPEC,
> + TCA_CBS_PARMS,
> + __TCA_CBS_MAX,
> +};
> +
> +#define TCA_CBS_MAX (__TCA_CBS_MAX - 1)
> +
>  #endif
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index e70ed26485a2..c03d86a7775e 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -172,6 +172,17 @@ config NET_SCH_TBF
> To compile this code as a module, choose M here: the
> module will be called sch_tbf.
>  
> +config NET_SCH_CBS
> + tristate "Credit Based Shaper (CBS)"
> + ---help---
> +   Say Y here if you want to use the Credit Based Shaper (CBS) packet
> +   scheduling algorithm.
> +
> +   See the top of  for more details.
> +
> +   To compile this code as a module, choose M here: the
> +   module will be called sch_cbs.
> +
>  config NET_SCH_GRED
>   tristate "Generic Random Early Detection (GRED)"
>   ---help---
> diff --git a/net/sched/Makefile b/net/sched/Makefile
> index 7b915d226de7..80c8f92d162d 100644
> --- a/net/sched/Makefile
> +++ b/net/sched/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_NET_SCH_FQ_CODEL)  += sch_fq_codel.o
>  obj-$(CONFIG_NET_SCH_FQ) += sch_fq.o
>  obj-$(CONFIG_NET_SCH_HHF)+= sch_hhf.o
>  obj-$(CONFIG_NET_SCH_PIE)+= sch_pie.o
> +obj-$(CONFIG_NET_SCH_CBS)+= sch_cbs.o
>  
>  obj-$(CONFIG_NET_CLS_U32)+= cls_u32.o
>  obj-$(CONFIG_NET_CLS_ROUTE4) += cls_route.o
> diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
> new file mode 100644
> index ..0643587e6dc8
> --- /dev/null
> +++ b/net/sched/sch_cbs.c
> @@ -0,0 +1,314 @@
> +/*
> + * net/sched/sch_cbs.c   Credit Based Shaper
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation; either version
> + *   2 of the License, or (at your option) any later version.
> + *
> + * Authors:  Vinicius Costa Gomes 
> + *
> + */
> +
> +/* Credit Based Shaper (CBS)
> + * =
> + *
> + * This is a simple rate-limiting shaper aimed at TSN applications on
> + * systems with known traffic workloads.
> + *
> + * Its algorithm is defined by the IEEE 802.1Q-2014 Specification,
> + * Section 8.6.8.2, and explained in more detail in the Annex L of the
> + * same specification.
> + *
> + * There are four tunables to be considered:
> + *
> + *   'idleslope': Idleslope is the rate of credits that is
> + *   accumulated (in kilobits per second) when there is at least
> + *   one packet waiting for transmission. Packets are transmitted
> + *   when the current value of credits is equal or greater than
> + *   zero. When there is no packet to be transmitted the amount of
> + *   credits is set to zero. This is the main tunable of the CBS
> + *   algorithm.
> + *
> + *   'sendslope':
> + *   Sendslope is the rate of credits that is depleted (it should be a
> + *   negative number of kilobits per second) when a transmission is
> + *   ocurring. It can be calculated as follows, (IEEE 802.1Q-2014 Section
> + *   8.6.8.2 item g):
> + *
> + *   sendslope = idleslope - port_transmit_rate
> + *
> + *   'hicredit': Hicredit defines the maximum amount of credits (in
> + *   bytes) that can be accumulated. Hicredit depends on the
> + *   characteristics of interfering traffic,
> + *   'max_interference_size' is the maximum size of any burst of
> + *   traffic that can delay the transmission of a frame that is
> + *   available for transmission for this traffic class, 

Re: [next-queue PATCH v7 4/6] net/sched: Introduce Credit Based Shaper (CBS) qdisc

[Vinicius Costa Gomes]

Hi,

Eric Dumazet  writes:

[...]

>
> Your mixing of s64 and u64 is disturbing.
>
> do_div() handles u64, not s64.
>
> div64_s64() might be needed in place of do_div()

I wasn't very comfortable about the signal juggling either. Didn't know
about div64_s64(), looks much better. Will fix, thanks.


Cheers,
--
Vinicius

Re: [next-queue PATCH v7 4/6] net/sched: Introduce Credit Based Shaper (CBS) qdisc

[Eric Dumazet]

On Thu, 2017-10-12 at 17:40 -0700, Vinicius Costa Gomes wrote:
> This queueing discipline implements the shaper algorithm defined by
> the 802.1Q-2014 Section 8.6.8.2 and detailed in Annex L.
> 
> It's primary usage is to apply some bandwidth reservation to user
> defined traffic classes, which are mapped to different queues via the
> mqprio qdisc.
> 
> Only a simple software implementation is added for now.
> 
> Signed-off-by: Vinicius Costa Gomes 
> Signed-off-by: Jesus Sanchez-Palencia 
> ---

> +/* timediff is in ns, slope is in kbps */
> +static s64 timediff_to_credits(s64 timediff, s32 slope)
> +{
> + s64 credits = timediff * slope * BYTES_PER_KBIT;
> +
> + do_div(credits, NSEC_PER_SEC);
> +
> + return credits;
> +}
> +
> +static s64 delay_from_credits(s64 credits, s32 slope)
> +{
> + s64 rate = slope * BYTES_PER_KBIT;
> + s64 delay;
> +
> + if (unlikely(rate == 0))
> + return S64_MAX;
> +
> + delay = -credits * NSEC_PER_SEC;
> + do_div(delay, rate);
> +
> + return delay;
> +}
> +
> +static s64 credits_from_len(unsigned int len, s32 slope, s64 port_rate)
> +{
> + /* As do_div() only works on unsigned quantities, convert
> +  * slope to a positive number here, and credits to a negative
> +  * number before returning.
> +  */
> + s64 rate = -slope * BYTES_PER_KBIT;
> + s64 credits;
> +
> + if (unlikely(port_rate == 0))
> + return S64_MAX;
> +
> + credits = len * rate;
> + do_div(credits, port_rate);
> +
> + return -credits;
> +}
> +


Your mixing of s64 and u64 is disturbing.

do_div() handles u64, not s64.

div64_s64() might be needed in place of do_div()