Re: [PATCH net-next v2] ipv6: ioam: Support for Queue depth data field

[Ido Schimmel <idosch@idosch.org>]

On Fri, Dec 24, 2021 at 02:50:00PM +0100, Justin Iurman wrote:
> v2:
>  - Fix sparse warning (use rcu_dereference)
> 
> This patch adds support for the queue depth in IOAM trace data fields.
> 
> The draft [1] says the following:
> 
>    The "queue depth" field is a 4-octet unsigned integer field.  This
>    field indicates the current length of the egress interface queue of
>    the interface from where the packet is forwarded out.  The queue
>    depth is expressed as the current amount of memory buffers used by
>    the queue (a packet could consume one or more memory buffers,
>    depending on its size).
> 
> An existing function (i.e., qdisc_qstats_qlen_backlog) is used to
> retrieve the current queue length without reinventing the wheel.
> 
> Note: it was tested and qlen is increasing when an artificial delay is
> added on the egress with tc.
> 
>   [1] https://datatracker.ietf.org/doc/html/draft-ietf-ippm-ioam-data#section-5.4.2.7
> 
> Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
> ---
>  net/ipv6/ioam6.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/ioam6.c b/net/ipv6/ioam6.c
> index 122a3d47424c..969a5adbaf5c 100644
> --- a/net/ipv6/ioam6.c
> +++ b/net/ipv6/ioam6.c
> @@ -13,10 +13,12 @@
>  #include <linux/ioam6.h>
>  #include <linux/ioam6_genl.h>
>  #include <linux/rhashtable.h>
> +#include <linux/netdevice.h>
>  
>  #include <net/addrconf.h>
>  #include <net/genetlink.h>
>  #include <net/ioam6.h>
> +#include <net/sch_generic.h>
>  
>  static void ioam6_ns_release(struct ioam6_namespace *ns)
>  {
> @@ -717,7 +719,19 @@ static void __ioam6_fill_trace_data(struct sk_buff *skb,
>  
>  	/* queue depth */
>  	if (trace->type.bit6) {
> -		*(__be32 *)data = cpu_to_be32(IOAM6_U32_UNAVAILABLE);
> +		struct netdev_queue *queue;
> +		struct Qdisc *qdisc;
> +		__u32 qlen, backlog;
> +
> +		if (skb_dst(skb)->dev->flags & IFF_LOOPBACK) {
> +			*(__be32 *)data = cpu_to_be32(IOAM6_U32_UNAVAILABLE);
> +		} else {
> +			queue = skb_get_tx_queue(skb_dst(skb)->dev, skb);
> +			qdisc = rcu_dereference(queue->qdisc);
> +			qdisc_qstats_qlen_backlog(qdisc, &qlen, &backlog);
> +
> +			*(__be32 *)data = cpu_to_be32(qlen);

Why 'qlen' is used and not 'backlog'? From the paragraph you quoted it
seems that queue depth needs to take into account the size of the
enqueued packets, not only their number.

Did you check what other IOAM implementations (SW/HW) report for queue
depth? I would assume that they report bytes.

> +		}
>  		data += sizeof(__be32);
>  	}
>  
> -- 
> 2.25.1
>