Re: [PATCH net-next v4] openvswitch: enable NSH support

["Yang, Yi" <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Mon, Aug 21, 2017 at 10:19:24AM +0200, Jiri Benc wrote:
> On Mon, 21 Aug 2017 14:11:10 +0800, Yang, Yi wrote:
> > In OVS code, it has been removed because of Microsoft compiler issue.
> 
> We absolutely, completely and utterly do not care in the kernel. Please
> never make such arguments and never make the code look worse because of
> a compiler for other operating systems.

Anyway, we need to keep the code in userspace consistent with the one in
kernel as possible as, otherwise it will be a burden for developer, I
know userspace has different coding standard from kernel, this will make
developer painful if we have two sets of code although they have same
functionality.

> 
> > > I was wondering about this during the reviews of the previous versions.
> > > Now I've given this more thought but I still don't see it - why is the
> > > inner_protocol set here?
> > 
> > I saw push_mpls has it, so also set it.
> 
> MPLS supports GSO and needs this for segmentation. I don't see anything
> GSO related in this patch.
> 
> How do you plan to address GSO, anyway?

No plan to do that, I'm not an expert on this, we can remove it if
you're very sure it is necessary.

> 
> > > > +	err = check_header(skb, length);
> > > > +	if (unlikely(err))
> > > > +		return err;
> > > > +
> > > > +	key->nsh.flags = nsh_get_flags(nsh);
> > > 
> > > Again, need to reload nsh.
> > 
> > I used skb->len in v5, so we can't avoid such issue.
> 
> And how do you ensure that the skb has enough headroom, then? That is
> wrong. All I said is that you have to reload the pointers to the header
> which is what you have to do.

Ok, got it, will add it.

> 
> > > Wouldn't it be nicer if the fields of struct ovs_nsh_key_base and of
> > > struct nsh_hdr had the same names?
> > 
> > Such change also will impact on OVS code, so I prefer not to change
> > them.
> 
> We do not care.
> 
> The order in which you send patches to different projects is your
> choice. The only standard by which we measure and evaluate kernel
> patches is the technical suitability of the patches. Whether or not
> some other projects have dependencies on some kind of previous versions
> of out of tree kernel patches have zero relevance here.
> 
> If you designed other code to depend on your notion on how a kernel API
> will look like before getting the actual patches accepted to the
> kernel, that's your problem and you'll have to deal with that.
> 
> > For struct nsh_hdr, we need more self-descriptive fields, but for struct
> > ovs_nsh_key_base, because we need to use keys in OVS OpenFlow, so np is
> > obviously better than next_proto, we also try our best to make sure the
> > old NSH implementation has same match fields as the new one does.
> 
> Not relevant.

Ok, I can follow your standard :-)

To make sure we make agreement, please confirm if this one is ok?

struct nsh_hdr {
    ovs_be16 ver_flags_ttl_len;
    uint8_t mdtype;
    uint8_t np;
    ovs_16aligned_be32 path_hdr;
    union {
        struct nsh_md1_ctx md1;
        struct nsh_md2_tlv md2;
    };
};

Or it will be better if you can provide your preferred version here.

> 
>  Jiri