Re: [PATCH v1 net-next 14/14] net/sched: Make etf report drops on error_queue

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Wed, Jun 27, 2018 at 6:07 PM Jesus Sanchez-Palencia
<jesus.sanchez-palencia@intel.com> wrote:
>
> Use the socket error queue for reporting dropped packets if the
> socket has enabled that feature through the SO_TXTIME API.
>
> Packets are dropped either on enqueue() if they aren't accepted by the
> qdisc or on dequeue() if the system misses their deadline. Those are
> reported as different errors so applications can react accordingly.
>
> Userspace can retrieve the errors through the socket error queue and the
> corresponding cmsg interfaces. A struct sock_extended_err* is used for
> returning the error data, and the packet's timestamp can be retrieved by
> adding both ee_data and ee_info fields as e.g.:
>
>     ((__u64) serr->ee_data << 32) + serr->ee_info
>
> This feature is disabled by default and must be explicitly enabled by
> applications. Enabling it can bring some overhead for the Tx cycles
> of the application.
>
> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
> ---

>  struct sock_txtime {
>         clockid_t       clockid;        /* reference clockid */
> -       u16             flags;          /* bit 0: txtime in deadline_mode */
> +       u16             flags;          /* bit 0: txtime in deadline_mode
> +                                        * bit 1: report drops on sk err queue
> +                                        */
>  };

If this is shared with userspace, should be defined in an uapi header.
Same on the flag bits below. Self documenting code is preferable over
comments.

>  /*
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 73f4404e49e4..e681a45cfe7e 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -473,6 +473,7 @@ struct sock {
>         u16                     sk_clockid;
>         u16                     sk_txtime_flags;
>  #define SK_TXTIME_DEADLINE_MASK        BIT(0)
> +#define SK_TXTIME_RECV_ERR_MASK        BIT(1)

Integer bitfields are (arguably) more readable. There is no requirement
that the user interface be the same as the in-kernel implementation. Indeed
if you can save bits in struct sock, that is preferable (but not so for the ABI,
which cannot easily be extended).

>
>         struct socket           *sk_socket;
>         void                    *sk_user_data;
> diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h
> index dc64cfaf13da..66fd5e443c94 100644
> --- a/include/uapi/linux/errqueue.h
> +++ b/include/uapi/linux/errqueue.h
> @@ -25,6 +25,8 @@ struct sock_extended_err {
>  #define SO_EE_OFFENDER(ee)     ((struct sockaddr*)((ee)+1))
>
>  #define SO_EE_CODE_ZEROCOPY_COPIED     1
> +#define SO_EE_CODE_TXTIME_INVALID_PARAM        2
> +#define SO_EE_CODE_TXTIME_MISSED       3
>
>  /**
>   *     struct scm_timestamping - timestamps exposed through cmsg
> diff --git a/net/sched/sch_etf.c b/net/sched/sch_etf.c
> index 5514a8aa3bd5..166f4b72875b 100644
> --- a/net/sched/sch_etf.c
> +++ b/net/sched/sch_etf.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/string.h>
>  #include <linux/errno.h>
> +#include <linux/errqueue.h>
>  #include <linux/rbtree.h>
>  #include <linux/skbuff.h>
>  #include <linux/posix-timers.h>
> @@ -124,6 +125,35 @@ static void reset_watchdog(struct Qdisc *sch)
>         qdisc_watchdog_schedule_ns(&q->watchdog, ktime_to_ns(next));
>  }
>
> +static void report_sock_error(struct sk_buff *skb, u32 err, u8 code)
> +{
> +       struct sock_exterr_skb *serr;
> +       ktime_t txtime = skb->tstamp;
> +
> +       if (!skb->sk || !(skb->sk->sk_txtime_flags & SK_TXTIME_RECV_ERR_MASK))
> +               return;
> +
> +       skb = skb_clone_sk(skb);
> +       if (!skb)
> +               return;
> +
> +       sock_hold(skb->sk);

Why take an extra reference? The skb holds a ref on the sk.

> +
> +       serr = SKB_EXT_ERR(skb);
> +       serr->ee.ee_errno = err;
> +       serr->ee.ee_origin = SO_EE_ORIGIN_LOCAL;

I suggest adding a new SO_EE_ORIGIN_TXTIME as opposed to overloading
the existing
local origin. Then the EE_CODE can start at 1, as ee_code can be
demultiplexed by origin.

> +       serr->ee.ee_type = 0;
> +       serr->ee.ee_code = code;
> +       serr->ee.ee_pad = 0;
> +       serr->ee.ee_data = (txtime >> 32); /* high part of tstamp */
> +       serr->ee.ee_info = txtime; /* low part of tstamp */
> +
> +       if (sock_queue_err_skb(skb->sk, skb))
> +               kfree_skb(skb);
> +
> +       sock_put(skb->sk);
> +}