From: Ben Pfaff
> Sent: 22 August 2017 18:35
...
> We solved the alignment problem in OVS userspace a different way, by
> defining our versions of the network protocol headers so that they only
> need 16-bit alignment. In turn, we did that by defining a
> ovs_16aligned_be32 type as a pair of be16s
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
On Tue, Aug 22, 2017 at 08:32:49AM +, Jan Scheurich wrote:
> > > Or why else does OVS user space code take so great pain to model
> > > possible misalignment and provide/use safe access functions?
> >
> > I don't know how the ovs user space deals with packet allocation. In
> > the kernel, the
On Mon, Aug 21, 2017 at 05:47:13PM +0800, Jiri Benc wrote:
>
> > 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:
> > If I understand correctly, this is a default definition that can be
> > overridden by drivers so that we still cannot rely on the Ethernet
> > payload always being 32-bit-aligned.
>
> Not by drivers, by architectures. Only architectures that can efficiently
> handle unaligned access (or on whi
On Mon, 21 Aug 2017 10:10:38 +, Jan Scheurich wrote:
> If I understand correctly, this is a default definition that can be
> overridden by drivers so that we still cannot rely on the Ethernet
> payload always being 32-bit-aligned.
Not by drivers, by architectures. Only architectures that can
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:
> > 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_hd
> > NSH can be carried over Ethernet with a 14 byte header. In that case
> > the total NSH header would typically be 16-bit aligned, so that all
> > 32-bit members would be misaligned.
>
> See NET_IP_ALIGN in include/linux/skbuff.h.
If I understand correctly, this is a default definition that can
On Mon, 21 Aug 2017 09:42:27 +, Jan Scheurich wrote:
> I understand your concern. But not declaring md2 as array is wrong as
> well, as there might not be an MD2 TLV context header. An MD2 NSH
> header is perfectly valid without any TLV. So in any case the user of
> the struct needs to be aware
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
> > 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[];
>
> I'm not that sure about this. With each member of md2 having a dif
On Mon, 21 Aug 2017 09:04:30 +, Jan Scheurich wrote:
> 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 {
>
>
> 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_md
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:
> > 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 co
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
> 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
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 nev
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
On Fri, Aug 18, 2017 at 09:31:03PM +0800, Jiri Benc wrote:
> On Fri, 18 Aug 2017 15:26:01 +0200, Jiri Benc wrote:
> > Out of time for today, will continue the review next week. Again, feel
> > free to send a new version meanwhile or wait for the rest of the
> > review, whatever works better for you
On Sat, Aug 19, 2017 at 03:09:47AM +0800, Eric Garver wrote:
> On Fri, Aug 18, 2017 at 03:24:31PM +0800, Yi Yang wrote:
> > v3->v4
> > - Add new NSH match field ttl
> > - Update NSH header to the latest format
> >which will be final format and won't change
> >per its author's confirmation
On Fri, Aug 18, 2017 at 09:26:01PM +0800, Jiri Benc wrote:
> On Fri, 18 Aug 2017 15:24:31 +0800, Yi Yang wrote:
> > +struct nsh_md2_tlv {
> > + __be16 md_class;
> > + u8 type;
> > + u8 length;
> > + /* Followed by variable-length data. */
> > +};
>
> What was wrong with the u8[] field that
On Fri, Aug 18, 2017 at 03:24:31PM +0800, Yi Yang wrote:
> v3->v4
> - Add new NSH match field ttl
> - Update NSH header to the latest format
>which will be final format and won't change
>per its author's confirmation.
> - Fix comments for v3.
Hi Yi,
Only a few comments below since Jiri
On Fri, 18 Aug 2017 15:26:01 +0200, Jiri Benc wrote:
> Out of time for today, will continue the review next week. Again, feel
> free to send a new version meanwhile or wait for the rest of the
> review, whatever works better for you.
One more thing: before you send a new version, be sure and doubl
On Fri, 18 Aug 2017 15:24:31 +0800, Yi Yang wrote:
> +struct nsh_md2_tlv {
> + __be16 md_class;
> + u8 type;
> + u8 length;
> + /* Followed by variable-length data. */
> +};
What was wrong with the u8[] field that was present at the end of the
struct in the previous version of the
v3->v4
- Add new NSH match field ttl
- Update NSH header to the latest format
which will be final format and won't change
per its author's confirmation.
- Fix comments for v3.
v2->v3
- Change OVS_KEY_ATTR_NSH to nested key to handle
length-fixed attributes and length-variable
attri
25 matches
Mail list logo
