lkml.org 
[lkml]   [2017]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update
    On Tue, Oct 10, 2017 at 7:33 PM, Manish Kurup <kurup.manish@gmail.com> 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?

    \
     
     \ /
      Last update: 2017-10-11 18:28    [W:3.878 / U:0.712 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site