Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: John Hurley <john.hurley@netronome.com>
To: Cong Wang <xiyou.wangcong@gmail.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 23:56:41 +0100
Message-ID: <CAK+XE=mXVW84MXE5bDYyGhK5XrC_q3ECiaj5=WsXFV0FXBk+eA@mail.gmail.com> (raw)
In-Reply-To: <CAM_iQpU0jZhg60-CVEZ9H1N57ga9jPwVt5tF1fM=uP_sj4kmKQ@mail.gmail.com>

On Fri, Jun 14, 2019 at 5:59 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> 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().
>

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.

  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
2019-06-14 22:56     ` John Hurley [this message]
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='CAK+XE=mXVW84MXE5bDYyGhK5XrC_q3ECiaj5=WsXFV0FXBk+eA@mail.gmail.com' \
    --to=john.hurley@netronome.com \
    --cc=davem@davemloft.net \
    --cc=dcaratti@redhat.com \
    --cc=jakub.kicinski@netronome.com \
    --cc=jiri@mellanox.com \
    --cc=netdev@vger.kernel.org \
    --cc=oss-drivers@netronome.com \
    --cc=simon.horman@netronome.com \
    --cc=xiyou.wangcong@gmail.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