Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Cong Wang <xiyou.wangcong@gmail.com>
To: John Hurley <john.hurley@netronome.com>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jiri Pirko <jiri@mellanox.com>,
	Davide Caratti <dcaratti@redhat.com>,
	Simon Horman <simon.horman@netronome.com>,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	oss-drivers@netronome.com
Subject: Re: [PATCH net-next v2 1/3] net: sched: add mpls manipulation actions to TC
Date: Fri, 14 Jun 2019 09:59:14 -0700
Message-ID: <CAM_iQpU0jZhg60-CVEZ9H1N57ga9jPwVt5tF1fM=uP_sj4kmKQ@mail.gmail.com> (raw)
In-Reply-To: <1560447839-8337-2-git-send-email-john.hurley@netronome.com>

On Thu, Jun 13, 2019 at 10:44 AM John Hurley <john.hurley@netronome.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().


> +}
> +
> +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()?



> +       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().


> +       }
> +
> +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.

  parent reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 17:43 [PATCH net-next v2 0/3] Add MPLS " John Hurley
2019-06-13 17:43 ` [PATCH net-next v2 1/3] net: sched: add mpls manipulation " John Hurley
2019-06-14  8:10   ` Jiri Pirko
2019-06-14 11:59     ` John Hurley
2019-06-14 16:59   ` Cong Wang [this message]
2019-06-14 22:56     ` John Hurley
2019-06-17 21:18       ` Cong Wang
2019-06-17 22:09         ` John Hurley
2019-06-13 17:43 ` [PATCH net-next v2 2/3] net: sched: include mpls actions in hardware intermediate representation John Hurley
2019-06-14  8:11   ` Jiri Pirko
2019-06-13 17:43 ` [PATCH net-next v2 3/3] selftests: tc-tests: actions: add MPLS tests John Hurley

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAM_iQpU0jZhg60-CVEZ9H1N57ga9jPwVt5tF1fM=uP_sj4kmKQ@mail.gmail.com' \
    --to=xiyou.wangcong@gmail.com \
    --cc=davem@davemloft.net \
    --cc=dcaratti@redhat.com \
    --cc=jakub.kicinski@netronome.com \
    --cc=jiri@mellanox.com \
    --cc=john.hurley@netronome.com \
    --cc=netdev@vger.kernel.org \
    --cc=oss-drivers@netronome.com \
    --cc=simon.horman@netronome.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox