On Mon, 21 Aug 2017 16:39:01 +0800, Yang, Yi wrote:
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'm sorry, I don't get this. What's wrong with having __u8[] as the
last member of the struct? That's C99. It's 18 years old standard.
We're using that throughout our uAPI. Why that should be a problem for
any user space program?
quoted
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.
Without GSO, I don't see any use for inner_protocol.
However, don't you need to software segment the packet if it's GSO
before pushing the NSH header?
And wouldn't it be better to implement GSO for NSH, anyway?
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.
I don't really care that much about the names if it's clear what they
mean. I was merely commenting on the inconsistency which looked weird.
Whether it's md_type or mdtype, I don't have a preference (does not
mean others won't, though :-)). Just pick one and stick to it, as far
as I'm concerned.
Jiri
On Mon, Aug 21, 2017 at 11:18:54AM +0200, Jiri Benc wrote:
On Mon, 21 Aug 2017 16:39:01 +0800, Yang, Yi wrote:
quoted
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'm sorry, I don't get this. What's wrong with having __u8[] as the
last member of the struct? That's C99. It's 18 years old standard.
We're using that throughout our uAPI. Why that should be a problem for
any user space program?
The issue is it is used union in
struct nsh_hdr {
ovs_be16 ver_flags_ttl_len;
uint8_t md_type;
uint8_t next_proto;
ovs_16aligned_be32 path_hdr;
union {
struct nsh_md1_ctx md1;
struct nsh_md2_tlv md2;
};
};
in Linux kernel build, it complained it, I changed it to
struct nsh_hdr {
ovs_be16 ver_flags_ttl_len;
uint8_t md_type;
uint8_t next_proto;
ovs_16aligned_be32 path_hdr;
union {
struct nsh_md1_ctx md1;
struct nsh_md2_tlv md2[0];
};
};
It is ok, but for Microsoft compiler, it isn't allowed there is struct
nsh_md2_tlv md2[0] in a union, that is Ben Pfaff's hack :-)
quoted
quoted
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.
Without GSO, I don't see any use for inner_protocol.
However, don't you need to software segment the packet if it's GSO
before pushing the NSH header?
And wouldn't it be better to implement GSO for NSH, anyway?
I don't know how we can support this, is it a must-have thing?
quoted
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.
I don't really care that much about the names if it's clear what they
mean. I was merely commenting on the inconsistency which looked weird.
Whether it's md_type or mdtype, I don't have a preference (does not
mean others won't, though :-)). Just pick one and stick to it, as far
as I'm concerned.
But struct nsh_hdr had different struct from struct ovs_key_nsh, we
have no way to make them completely same, do you mean we should use the
same name if they are same fields and represent the same thing?
On Mon, 21 Aug 2017 17:15:42 +0800, Yang, Yi wrote:
The issue is it is used union in
struct nsh_hdr {
ovs_be16 ver_flags_ttl_len;
uint8_t md_type;
uint8_t next_proto;
ovs_16aligned_be32 path_hdr;
union {
struct nsh_md1_ctx md1;
struct nsh_md2_tlv md2;
};
};
This should work (modulo the non-kernel type names, of course). Did you
mean to put [] after md2?
in Linux kernel build, it complained it, I changed it to
I wouldn't use this. First, zero length array is a GCC extension. It
would indeed be better not to use that in uAPI. Second, I wouldn't even
use a flexible array member here, see my reply to Jan for the reasons.
Note that I commented on struct nsh_md2_tlv having __u8[] as the last
member which IMHO makes good sense. I'm not entirely sure what C99 says
about flexible array member being part of a struct inside union inside
a struct, though. GCC seems to cope with that just fine but AFAIK it
has some extension over the C standard wrt. flexible array members.
I don't know how we can support this, is it a must-have thing?
What would happen if you get a GSO packet? Ports of an ovs bridge claim
GSO support, thus they may get a GSO packet. You have to handle it one
way or the other: either software segment the packet before pushing the
header, or implement proper GSO support for NSH.
But struct nsh_hdr had different struct from struct ovs_key_nsh, we
have no way to make them completely same, do you mean we should use the
same name if they are same fields and represent the same thing?
On Mon, Aug 21, 2017 at 05:47:13PM +0800, Jiri Benc wrote:
On Mon, 21 Aug 2017 17:15:42 +0800, Yang, Yi wrote:
quoted
The issue is it is used union in
struct nsh_hdr {
ovs_be16 ver_flags_ttl_len;
uint8_t md_type;
uint8_t next_proto;
ovs_16aligned_be32 path_hdr;
union {
struct nsh_md1_ctx md1;
struct nsh_md2_tlv md2;
};
};
This should work (modulo the non-kernel type names, of course). Did you
mean to put [] after md2?
Yes, the original version has [] after md2.
quoted
in Linux kernel build, it complained it, I changed it to
What was the error message?
./include/net/nsh.h:213:25: error: flexible array member in union
struct nsh_md2_tlv md2[];
^
I wouldn't use this. First, zero length array is a GCC extension. It
would indeed be better not to use that in uAPI. Second, I wouldn't even
use a flexible array member here, see my reply to Jan for the reasons.
Note that I commented on struct nsh_md2_tlv having __u8[] as the last
member which IMHO makes good sense. I'm not entirely sure what C99 says
about flexible array member being part of a struct inside union inside
a struct, though. GCC seems to cope with that just fine but AFAIK it
has some extension over the C standard wrt. flexible array members.
Yes, if struct nsh_md2_tlv has __u8[] as last field,
struct nsh_md2_tlv {
__be16 md_class;
u8 type;
u8 length;
u8 md_value[];
};
struct nsh_hdr {
__be16 ver_flags_ttl_len;
u8 md_type;
u8 next_proto;
__be32 path_hdr;
union {
struct nsh_md1_ctx md1;
struct nsh_md2_tlv md2;
};
};
it is ok, so let us use this one.
quoted
I don't know how we can support this, is it a must-have thing?
What would happen if you get a GSO packet? Ports of an ovs bridge claim
GSO support, thus they may get a GSO packet. You have to handle it one
way or the other: either software segment the packet before pushing the
header, or implement proper GSO support for NSH.
This is an issue, I'll investigate it and find a way to handle this.
quoted
But struct nsh_hdr had different struct from struct ovs_key_nsh, we
have no way to make them completely same, do you mean we should use the
same name if they are same fields and represent the same thing?
On Mon, Aug 21, 2017 at 05:47:13PM +0800, Jiri Benc wrote:
quoted
I don't know how we can support this, is it a must-have thing?
What would happen if you get a GSO packet? Ports of an ovs bridge claim
GSO support, thus they may get a GSO packet. You have to handle it one
way or the other: either software segment the packet before pushing the
header, or implement proper GSO support for NSH.
I checked GSO support code, we have two cases to handle, one case is to
output NSH packet to VxLAN-gpe port, the other case is to output NSH packet
to Ethernet port (tap, veth, vhost, physical eth, etc.), I don't think
it is a good way to do GSO support in OVS module, because we can't know
the final output port at this point, if the final output port is
VxLAN-gpe port, it will still face GSO issue, but I didn't see vxlan
module handles this, maybe udp tunnel did this. I think a NSH packet can
be split into two packets, one has NSH header, another one doesn't, so I'm not sure how vxlan module can avoid this situation.
For the second case, we can add a nsh GSO module in net/nsh/nsh_gso.c (as MPLS did in net/mpls/mpls_gso.c), that is the best way to handle this, what do you
think about it?
On Tue, 22 Aug 2017 17:38:26 +0800, Yang, Yi wrote:
I checked GSO support code, we have two cases to handle, one case is to
output NSH packet to VxLAN-gpe port, the other case is to output NSH packet
to Ethernet port (tap, veth, vhost, physical eth, etc.), I don't think
it is a good way to do GSO support in OVS module, because we can't know
the final output port at this point, if the final output port is
VxLAN-gpe port, it will still face GSO issue, but I didn't see vxlan
module handles this, maybe udp tunnel did this. I think a NSH packet can
be split into two packets, one has NSH header, another one doesn't,
so I'm not sure how vxlan module can avoid this situation.
As I said before, by either implementing GSO support for NSH or by
segmenting in software before pushing the NSH header. In the first
case, the packet will be software segmented before being pushed to the
hardware, in the second case, it will be software segmented before
pushing the NSH header.
For the second case, we can add a nsh GSO module in net/nsh/nsh_gso.c
(as MPLS did in net/mpls/mpls_gso.c), that is the best way to handle
this, what do you think about it?
Yes, that's one of the two options that we've been talking about. It's
the better and the more efficient one.
Jiri