Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Cong Wang]

On Tue, Oct 10, 2017 at 7:33 PM, Manish Kurup <kurup.manish@xxxxxxxxx> wrote:
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index 14c262c..9bb0236 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -29,31 +29,37 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>         int action;
>         int err;
>         u16 tci;
> +       struct tcf_vlan_params *p;
>
>         tcf_lastuse_update(&v->tcf_tm);
>         bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
>
> -       spin_lock(&v->tcf_lock);
> -       action = v->tcf_action;
> -

spin_lock() is removed here, see below.


>         /* Ensure 'data' points at mac_header prior calling vlan manipulating
>          * functions.
>          */
>         if (skb_at_tc_ingress(skb))
>                 skb_push_rcsum(skb, skb->mac_len);
>
> -       switch (v->tcfv_action) {
> +       rcu_read_lock();
> +
> +       action = READ_ONCE(v->tcf_action);
> +
> +       p = rcu_dereference(v->vlan_p);
> +
> +       switch (p->tcfv_action) {
>         case TCA_VLAN_ACT_POP:
>                 err = skb_vlan_pop(skb);
>                 if (err)
>                         goto drop;
>                 break;
> +
>         case TCA_VLAN_ACT_PUSH:
> -               err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid |
> -                                   (v->tcfv_push_prio << VLAN_PRIO_SHIFT));
> +               err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid |
> +                               (p->tcfv_push_prio << VLAN_PRIO_SHIFT));
>                 if (err)
>                         goto drop;
>                 break;
> +
>         case TCA_VLAN_ACT_MODIFY:
>                 /* No-op if no vlan tag (either hw-accel or in-payload) */
>                 if (!skb_vlan_tagged(skb))
> @@ -69,15 +75,16 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>                                 goto drop;
>                 }
>                 /* replace the vid */
> -               tci = (tci & ~VLAN_VID_MASK) | v->tcfv_push_vid;
> +               tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid;
>                 /* replace prio bits, if tcfv_push_prio specified */
> -               if (v->tcfv_push_prio) {
> +               if (p->tcfv_push_prio) {
>                         tci &= ~VLAN_PRIO_MASK;
> -                       tci |= v->tcfv_push_prio << VLAN_PRIO_SHIFT;
> +                       tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT;
>                 }
>                 /* put updated tci as hwaccel tag */
> -               __vlan_hwaccel_put_tag(skb, v->tcfv_push_proto, tci);
> +               __vlan_hwaccel_put_tag(skb, p->tcfv_push_proto, tci);
>                 break;
> +
>         default:
>                 BUG();
>         }
> @@ -89,6 +96,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>         qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
>
>  unlock:
> +       rcu_read_unlock();
>         if (skb_at_tc_ingress(skb))
>                 skb_pull_rcsum(skb, skb->mac_len);
>


But here spin_unlock() is not removed... At least it doesn't show in diff
context. It's probably unbalanced spinlock.


> @@ -111,6 +119,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>         struct nlattr *tb[TCA_VLAN_MAX + 1];
>         struct tc_vlan *parm;
>         struct tcf_vlan *v;
> +       struct tcf_vlan_params *p, *p_old;
>         int action;
>         __be16 push_vid = 0;
>         __be16 push_proto = 0;
> @@ -187,16 +196,33 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>
>         v = to_vlan(*a);
>
> -       spin_lock_bh(&v->tcf_lock);
> -
> -       v->tcfv_action = action;
> -       v->tcfv_push_vid = push_vid;
> -       v->tcfv_push_prio = push_prio;
> -       v->tcfv_push_proto = push_proto;
> +       ASSERT_RTNL();
> +       p = kzalloc(sizeof(*p), GFP_KERNEL);
> +       if (unlikely(!p)) {
> +               if (ovr)
> +                       tcf_idr_release(*a, bind);
> +               return -ENOMEM;
> +       }
>
>         v->tcf_action = parm->action;
>
> -       spin_unlock_bh(&v->tcf_lock);
> +       p_old = rtnl_dereference(v->vlan_p);
> +
> +       if (ovr)
> +               spin_lock_bh(&v->tcf_lock);

Why still take spinlock when you already have RTNL lock?
What's the point?