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.
quoted
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?
quoted
quoted
+ 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.
quoted
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.
Jiri
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:
quoted
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.
quoted
quoted
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.
quoted
quoted
quoted
+ 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.
quoted
quoted
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.
quoted
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
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;
};
};
The second member of the union should be a variable length array [] of struct nsh_md2_tlv
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[];
};
};
That was the original design before Ben removed it due to missing support in Microsoft compiler.
For the Kernel datapath we should go back to that.
I wonder about the possible 16-bit alignment of the 32-bit fields, though. How is that handled in the kernel? Also struct nsh_md1_ctx has 32-bit members, which might not be 32-bit aligned in the packet.
BR, Jan