Re: [RFC V3 PATCH 18/26] net/netpolicy: set tx queues according to policy

[Tom Herbert]

On Mon, Sep 12, 2016 at 7:55 AM,  <kan.liang@xxxxxxxxx> wrote:
> From: Kan Liang <kan.liang@xxxxxxxxx>
>
> When the device tries to transmit a packet, netdev_pick_tx is called to
> find the available tx queues. If the net policy is applied, it picks up
> the assigned tx queue from net policy subsystem, and redirect the
> traffic to the assigned queue.
>
> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxx>
> ---
>  include/net/sock.h |  9 +++++++++
>  net/core/dev.c     | 20 ++++++++++++++++++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index e1e9e3d..ca97f35 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2280,4 +2280,13 @@ extern int sysctl_optmem_max;
>  extern __u32 sysctl_wmem_default;
>  extern __u32 sysctl_rmem_default;
>
> +/* Return netpolicy instance information from socket. */
> +static inline struct netpolicy_instance *netpolicy_find_instance(struct sock *sk)
> +{
> +#ifdef CONFIG_NETPOLICY
> +       if (is_net_policy_valid(sk->sk_netpolicy.policy))
> +               return &sk->sk_netpolicy;
> +#endif
> +       return NULL;
> +}
>  #endif /* _SOCK_H */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 34b5322..b9a8044 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3266,6 +3266,7 @@ struct netdev_queue *netdev_pick_tx(struct net_device *dev,
>                                     struct sk_buff *skb,
>                                     void *accel_priv)
>  {
> +       struct sock *sk = skb->sk;
>         int queue_index = 0;
>
>  #ifdef CONFIG_XPS
> @@ -3280,8 +3281,23 @@ struct netdev_queue *netdev_pick_tx(struct net_device *dev,
>                 if (ops->ndo_select_queue)
>                         queue_index = ops->ndo_select_queue(dev, skb, accel_priv,
>                                                             __netdev_pick_tx);
> -               else
> -                       queue_index = __netdev_pick_tx(dev, skb);
> +               else {
> +#ifdef CONFIG_NETPOLICY
> +                       struct netpolicy_instance *instance;
> +
> +                       queue_index = -1;
> +                       if (dev->netpolicy && sk) {
> +                               instance = netpolicy_find_instance(sk);
> +                               if (instance) {
> +                                       if (!instance->dev)
> +                                               instance->dev = dev;
> +                                       queue_index = netpolicy_pick_queue(instance, false);
> +                               }
> +                       }
> +                       if (queue_index < 0)
> +#endif

I doubt this produces the intended effect. Several drivers use
ndo_select_queue (such as mlx4) where there might do something special
for a few packets but end up called the default handler which
__netdev_pick_tx for most packets. So in such cases the netpolicy path
would be routinely bypassed. Maybe this code should be in
__netdev_pick_tx.

Tom

> +                               queue_index = __netdev_pick_tx(dev, skb);
> +               }
>
>                 if (!accel_priv)
>                         queue_index = netdev_cap_txqueue(dev, queue_index);
> --
> 2.5.5
>