On Fri, Aug 11, 2017 at 10:24:18AM +0200, Jiri Benc wrote:
> On Thu, 10 Aug 2017 21:21:15 +0800, Yi Yang wrote:
> > OVS master and 2.8 branch has merged NSH userspace
> > patch series, this patch is to enable NSH support
> > in kernel data path in order that OVS can support
> > NSH in 2.8 release in compat mode by porting this.
>
> Please include changelog when posting a new version of a patch.
>
> > +static inline u16
> > +nsh_hdr_len(const struct nsh_hdr *nsh)
>
> Single line, please. And all other instances of this in nsh.h, too.
Thanks Jiri, I'll change these in next version.
>
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -333,6 +333,7 @@ enum ovs_key_attr {
> > OVS_KEY_ATTR_CT_LABELS, /* 16-octet connection tracking label */
> > OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4, /* struct ovs_key_ct_tuple_ipv4 */
> > OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, /* struct ovs_key_ct_tuple_ipv6 */
> > + OVS_KEY_ATTR_NSH, /* struct ovs_key_nsh */
> >
> > #ifdef __KERNEL__
> > OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info */
> > @@ -491,6 +492,15 @@ struct ovs_key_ct_tuple_ipv6 {
> > __u8 ipv6_proto;
> > };
> >
> > +struct ovs_key_nsh {
> > + __u8 flags;
> > + __u8 mdtype;
> > + __u8 np;
> > + __u8 pad;
> > + __be32 path_hdr;
> > + __be32 c[4];
>
> I still don't like the "c" name. Please change it to something
> descriptive. This is uAPI and can't be changed later.
is "__be32 context[4]" ok?
>
> And I still don't see my comment about this not being extensible for
> MD type 2 addressed. Please understand this is uAPI and it is set in
> stone once it is merged into the kernel. It's very important we get
> this right since the beginning.
I understood it, but I don't know how I can do this per your comments,
Per Ben's comments,
PUSH_NSH message looks like
PUSH_NSH begin
nsh base header
MD type 1
PUSH_NSH end
or
PUSH_NSH begin
nsh base header
MD type 2
PUSH_NSH end
If so, I think we don't need struct ovs_action_push_nsh at all, just as
_CT action did, treat it as a variable data buffer.
So define three new netlink attributes
OVS_ACTION_ATTR_NSH_BASE_HEADER
OVS_ACTION_ATTR_NSH_MD1_DATA
OVS_ACTION_ATTR_NSH_MD2_DATA
OVS_ACTION_ATTR_PUSH_NSH is nested netlink attribute, it will nest
OVS_ACTION_ATTR_NSH_BASE_HEADER and OVS_ACTION_ATTR_NSH_MD1_DATA for MD
type 1, it will nest OVS_ACTION_ATTR_NSH_BASE_HEADER and
OVS_ACTION_ATTR_NSH_MD2_DATA for MD type 2. I'll compeletely remove struct
ovs_action_push_nsh, is it ok?
>
> > +struct ovs_action_push_nsh {
> > + __u8 flags;
> > + __u8 mdtype;
> > + __u8 mdlen;
> > + __u8 np;
> > + __be32 path_hdr;
> > + __u8 metadata[];
> > +};
>
> This is not how netlink attributes work. Please reread Ben Pfaff's
> explanation on how this needs to be structured (Message-ID:
> <20170809180912.gu6...@ovn.org>) and rework the patch. I 100% agree
> with what he wrote, his proposal is very clean and matches how netlink
> is designed.
>
> > @@ -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_PUSH_NSH, /* struct ovs_action_push_nsh. */
> > + OVS_ACTION_ATTR_POP_NSH, /* No argument. */
>
> Thank you for changing this to push/pop, it looks much cleaner now.
>
> > +static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> > +{
> > + struct nsh_hdr *nsh = (struct nsh_hdr *)skb_network_header(skb);
> > + u16 ver_flags_len;
> > + u8 version, length;
> > + u32 path_hdr;
> > + int i;
> > +
> > + memset(&key->nsh, 0, sizeof(struct ovs_key_nsh));
> > + ver_flags_len = ntohs(nsh->ver_flags_len);
> > + version = (ver_flags_len & NSH_VER_MASK) >> NSH_VER_SHIFT;
> > + length = (ver_flags_len & NSH_LEN_MASK) >> NSH_LEN_SHIFT;
>
> A nit: the operation to get/set version, length and flags from the NSH
> header seems to be repeated enough to warrant helper functions in
> include/net/nsh.h. Something like:
>
> static inline u8 nsh_get_version(const struct nsh_hdr *nsh)
> {
> return (ntohs(nsh->ver_flags_len) & NSH_VER_MASK) >> NSH_VER_SHIFT;
> }
>
> etc.
>
> Not a blocker, though, it may be done later if needed.
Ok, will change it.
>
> > @@ -76,9 +77,11 @@ static bool actions_may_change_flow(const struct nlattr
> > *actions)
> >
> > case OVS_ACTION_ATTR_CT:
> > case OVS_ACTION_ATTR_HASH:
> > + case OVS_ACTION_ATTR_POP_NSH:
> > case OVS_ACTION_ATTR_POP_ETH:
> > case OVS_ACTION_ATTR_POP_MPLS:
> > case OVS_ACTION_ATTR_POP_VLAN:
> > + case OVS_ACTION_ATTR_PUSH_NSH:
> > case OVS_ACTION_ATTR_PUSH_ETH:
> > case OVS_ACTION_ATTR_PUSH_MPLS:
> > case OVS_ACTION_ATTR_PUSH_VLAN:
>
> Alphabetical order, please.
Ok, will change.
>
> Thanks,
>
> Jiri
