From: Jiri Benc <jbenc@redhat•com>
To: Yi Yang <yi.y.yang@intel•com>
Cc: netdev@vger•kernel.org, dev@openvswitch•org, davem@davemloft•net
Subject: Re: [PATCH net-next] openvswitch: add NSH support
Date: Tue, 8 Aug 2017 16:28:10 +0200 [thread overview]
Message-ID: <20170808162810.1bab3812@griffin> (raw)
In-Reply-To: <1502168380-106785-1-git-send-email-yi.y.yang@intel.com>
On Tue, 8 Aug 2017 12:59:40 +0800, Yi Yang wrote:
> +struct ovs_key_nsh {
> + __u8 flags;
> + __u8 mdtype;
> + __u8 np;
> + __u8 pad;
> + __be32 path_hdr;
> + __be32 c[4];
"c" is a very poor name. Please rename it to something that expresses
what this field contains.
Also, this looks like MD type 1 only. How are those fields going to work
with MD type 2? I don't think MD type 2 implementation is necessary in
this patch but I'd like to know how this is going to work - it's uAPI
and thus set in stone once this is merged. The uAPI needs to be
designed with future use in mind.
> +#define OVS_ENCAP_NSH_MAX_MD_LEN 16
> +/*
> + * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
> + * @flags: NSH header flags.
> + * @mdtype: NSH metadata type.
> + * @mdlen: Length of NSH metadata in bytes.
> + * @np: NSH next_protocol: Inner packet type.
> + * @path_hdr: NSH service path id and service index.
> + * @metadata: NSH metadata for MD type 1 or 2
> + */
> +struct ovs_action_encap_nsh {
> + __u8 flags;
> + __u8 mdtype;
> + __u8 mdlen;
> + __u8 np;
> + __be32 path_hdr;
> + __u8 metadata[OVS_ENCAP_NSH_MAX_MD_LEN];
This is wrong. The metadata size is set to a fixed size by this and
cannot be ever extended, or at least not easily. Netlink has attributes
for exactly these cases and that's what needs to be used here.
> @@ -835,6 +866,8 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_TRUNC, /* u32 struct ovs_action_trunc. */
> OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
> OVS_ACTION_ATTR_POP_ETH, /* No argument. */
> + OVS_ACTION_ATTR_ENCAP_NSH, /* struct ovs_action_encap_nsh. */
> + OVS_ACTION_ATTR_DECAP_NSH, /* No argument. */
Use "push" and "pop", not "encap" and "decap" to be consistent with the
naming in the rest of the file. We use encap and decap for tunnel
operations. This code does not use lwtunnels, thus push and pop is more
appropriate.
Jiri
next prev parent reply other threads:[~2017-08-08 14:28 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-08 4:59 [PATCH net-next] openvswitch: add NSH support Yi Yang
2017-08-08 14:28 ` Jiri Benc [this message]
2017-08-09 2:05 ` Yang, Yi Y
[not found] ` <79BBBFE6CB6C9B488C1A45ACD284F51961C391CA-0J0gbvR4kTggGBtAFL8yw7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
2017-08-09 2:42 ` Ben Pfaff
[not found] ` <20170809024200.GG6175-LZ6Gd1LRuIk@public.gmane.org>
2017-08-09 8:32 ` Jan Scheurich
[not found] ` <CFF8EF42F1132E4CBE2BF0AB6C21C58D72735682-hqolJogE5njKJFWPz4pdheaU1rCVNFv4@public.gmane.org>
2017-08-09 9:41 ` Yang, Yi Y
2017-08-09 18:09 ` [ovs-dev] " Ben Pfaff
2017-08-09 20:12 ` Yang, Yi Y
[not found] ` <79BBBFE6CB6C9B488C1A45ACD284F51961C3C14A-0J0gbvR4kTggGBtAFL8yw7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
2017-08-09 20:53 ` Ben Pfaff
2017-08-09 22:53 ` [ovs-dev] " Yang, Yi Y
Reply instructions:
You may reply publicly 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=20170808162810.1bab3812@griffin \
--to=jbenc@redhat$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dev@openvswitch$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=yi.y.yang@intel$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox