Date:   Fri, 14 Jun 2019 23:56:41 +0100
From:   John Hurley <john.hurley@...ronome.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        Davide Caratti <dcaratti@...hat.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        oss-drivers@...ronome.com
Subject: Re: [PATCH net-next v2 1/3] net: sched: add mpls manipulation actions
 to TC

On Fri, Jun 14, 2019 at 5:59 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> On Thu, Jun 13, 2019 at 10:44 AM John Hurley <john.hurley@...ronome.com> wrote:
> > +static inline void tcf_mpls_set_eth_type(struct sk_buff *skb, __be16 ethertype)
> > +{
> > +       struct ethhdr *hdr = eth_hdr(skb);
> > +
> > +       skb_postpull_rcsum(skb, &hdr->h_proto, ETH_TLEN);
> > +       hdr->h_proto = ethertype;
> > +       skb_postpush_rcsum(skb, &hdr->h_proto, ETH_TLEN);
>
> So you just want to adjust the checksum with the new ->h_proto
> value. please use a right csum API, rather than skb_post*_rcsum().
>

Hi Cong,
Yes, I'm trying to maintain the checksum value if checksum complete
has been set.
The function above pulls the old eth type out of the checksum value
(if it is checksum complete), updates the eth type, and pushes the new
eth type into the checksum.
This passes my tests on the checksum.
I couldn't see an appropriate function to do this other than
recalculating the whole thing.
Maybe I missed something?

>
> > +}
> > +
> > +static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a,
> > +                       struct tcf_result *res)
> > +{
> > +       struct tcf_mpls *m = to_mpls(a);
> > +       struct mpls_shim_hdr *lse;
> > +       struct tcf_mpls_params *p;
> > +       u32 temp_lse;
> > +       int ret;
> > +       u8 ttl;
> > +
> > +       tcf_lastuse_update(&m->tcf_tm);
> > +       bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
> > +
> > +       /* Ensure 'data' points at mac_header prior calling mpls manipulating
> > +        * functions.
> > +        */
> > +       if (skb_at_tc_ingress(skb))
> > +               skb_push_rcsum(skb, skb->mac_len);
> > +
> > +       ret = READ_ONCE(m->tcf_action);
> > +
> > +       p = rcu_dereference_bh(m->mpls_p);
> > +
> > +       switch (p->tcfm_action) {
> > +       case TCA_MPLS_ACT_POP:
> > +               if (unlikely(!eth_p_mpls(skb->protocol)))
> > +                       goto out;
> > +
> > +               if (unlikely(skb_ensure_writable(skb, ETH_HLEN + MPLS_HLEN)))
> > +                       goto drop;
> > +
> > +               skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
> > +               memmove(skb->data + MPLS_HLEN, skb->data, ETH_HLEN);
> > +
> > +               __skb_pull(skb, MPLS_HLEN);
> > +               skb_reset_mac_header(skb);
> > +               skb_set_network_header(skb, ETH_HLEN);
> > +
> > +               tcf_mpls_set_eth_type(skb, p->tcfm_proto);
> > +               skb->protocol = p->tcfm_proto;
> > +               break;
> > +       case TCA_MPLS_ACT_PUSH:
> > +               if (unlikely(skb_cow_head(skb, MPLS_HLEN)))
> > +                       goto drop;
> > +
> > +               skb_push(skb, MPLS_HLEN);
> > +               memmove(skb->data, skb->data + MPLS_HLEN, ETH_HLEN);
> > +               skb_reset_mac_header(skb);
> > +               skb_set_network_header(skb, ETH_HLEN);
> > +
> > +               lse = mpls_hdr(skb);
> > +               lse->label_stack_entry = 0;
> > +               tcf_mpls_mod_lse(lse, p, !eth_p_mpls(skb->protocol));
> > +               skb_postpush_rcsum(skb, lse, MPLS_HLEN);
> > +
> > +               tcf_mpls_set_eth_type(skb, p->tcfm_proto);
> > +               skb->protocol = p->tcfm_proto;
> > +               break;
>
> Is it possible to refactor and reuse the similar code in
> net/openvswitch/actions.c::pop_mpls()/push_mpls()?
>

This is something I would need to look into

>
>
> > +       case TCA_MPLS_ACT_MODIFY:
> > +               if (unlikely(!eth_p_mpls(skb->protocol)))
> > +                       goto out;
> > +
> > +               if (unlikely(skb_ensure_writable(skb, ETH_HLEN + MPLS_HLEN)))
> > +                       goto drop;
> > +
> > +               lse = mpls_hdr(skb);
> > +               skb_postpull_rcsum(skb, lse, MPLS_HLEN);
> > +               tcf_mpls_mod_lse(lse, p, false);
> > +               skb_postpush_rcsum(skb, lse, MPLS_HLEN);
> > +               break;
> > +       case TCA_MPLS_ACT_DEC_TTL:
> > +               if (unlikely(!eth_p_mpls(skb->protocol)))
> > +                       goto out;
> > +
> > +               if (unlikely(skb_ensure_writable(skb, ETH_HLEN + MPLS_HLEN)))
> > +                       goto drop;
> > +
> > +               lse = mpls_hdr(skb);
> > +               temp_lse = be32_to_cpu(lse->label_stack_entry);
> > +               ttl = (temp_lse & MPLS_LS_TTL_MASK) >> MPLS_LS_TTL_SHIFT;
> > +               if (!--ttl)
> > +                       goto drop;
> > +
> > +               temp_lse &= ~MPLS_LS_TTL_MASK;
> > +               temp_lse |= ttl << MPLS_LS_TTL_SHIFT;
> > +               skb_postpull_rcsum(skb, lse, MPLS_HLEN);
> > +               lse->label_stack_entry = cpu_to_be32(temp_lse);
> > +               skb_postpush_rcsum(skb, lse, MPLS_HLEN);
> > +               break;
> > +       default:
> > +               WARN_ONCE(1, "Invalid MPLS action\n");
>
>
> This warning is not necessary, it must be validated in ->init().
>

ack.
Thanks for comments

>
> > +       }
> > +
> > +out:
> > +       if (skb_at_tc_ingress(skb))
> > +               skb_pull_rcsum(skb, skb->mac_len);
> > +
> > +       return ret;
> > +
> > +drop:
> > +       qstats_drop_inc(this_cpu_ptr(m->common.cpu_qstats));
> > +       return TC_ACT_SHOT;
> > +}
>
>
> Thanks.

Powered by blists - more mailing lists