On Thu, May 24, 2018 at 05:22:37PM -0700, William Tu wrote: > On Thu, May 24, 2018 at 2:16 PM, Ben Pfaff <[email protected]> wrote: > > On Thu, May 24, 2018 at 12:08:31PM -0700, William Tu wrote: > >> On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff <[email protected]> wrote: > >> > On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote: > >> >> On Wed, May 23, 2018 at 7:13 PM, Darrell Ball <[email protected]> wrote: > >> >> > The struct erspan_metadata is updated to replace the 'version' > >> >> > placeholder with the erspan base hdr. Also, the erspan > >> >> > index is defined explicitly as ovs_16aligned_be32 to mirror > >> >> > its encoding. > >> >> > Changes to odp_util result from updating the erspan index > >> >> > type. > >> >> > > >> >> > CC: William Tu <[email protected]> > >> >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") > >> >> > Signed-off-by: Darrell Ball <[email protected]> > >> >> > --- > >> >> > lib/odp-util.c | 36 ++++++++++++++++++++---------------- > >> >> > lib/packets.h | 6 +++--- > >> >> > 2 files changed, 23 insertions(+), 19 deletions(-) > >> >> > > >> >> > diff --git a/lib/odp-util.c b/lib/odp-util.c > >> >> > index 105ac80..767281f 100644 > >> >> > --- a/lib/odp-util.c > >> >> > +++ b/lib/odp-util.c > >> >> > @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr > >> >> > *attr, bool is_mask, > >> >> > > >> >> > memcpy(&opts, nl_attr_get(a), attr_len); > >> >> > > >> >> > - tun->erspan_ver = opts.version; > >> >> > + tun->erspan_ver = opts.bh.ver; > >> >> > if (tun->erspan_ver == 1) { > >> >> > - tun->erspan_idx = ntohl(opts.u.index); > >> >> > + tun->erspan_idx = > >> >> > ntohl(get_16aligned_be32(&opts.u.index)); > >> >> > } else if (tun->erspan_ver == 2) { > >> >> > tun->erspan_dir = opts.u.md2.dir; > >> >> > tun->erspan_hwid = get_hwid(&opts.u.md2); > >> >> > @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const > >> >> > struct flow_tnl *tun_key, > >> >> > !strcmp(tnl_type, "ip6erspan")) && > >> >> > (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) { > >> >> > struct erspan_metadata opts; > >> >> > + memset(&opts, 0, sizeof opts); > >> >> > > >> >> > - opts.version = tun_key->erspan_ver; > >> >> > - if (opts.version == 1) { > >> >> > - opts.u.index = htonl(tun_key->erspan_idx); > >> >> > + opts.bh.ver = tun_key->erspan_ver; > >> >> > + if (opts.bh.ver == 1) { > >> >> > + put_16aligned_be32(&opts.u.index, > >> >> > htonl(tun_key->erspan_idx)); > >> >> > } else { > >> >> > opts.u.md2.dir = tun_key->erspan_dir; > >> >> > set_hwid(&opts.u.md2, tun_key->erspan_hwid); > >> >> > @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr > >> >> > *attr, > >> >> > opts = nl_attr_get(attr); > >> >> > mask = mask_attr ? nl_attr_get(mask_attr) : NULL; > >> >> > > >> >> > - ver = (uint8_t)opts->version; > >> >> > + ver = opts->bh.ver; > >> >> > if (mask) { > >> >> > - ver_ma = (uint8_t)mask->version; > >> >> > + ver_ma = mask->bh.ver; > >> >> > } > >> >> > > >> >> > format_u8u(ds, "ver", ver, mask ? &ver_ma : NULL, verbose); > >> >> > > >> >> > - if (opts->version == 1) { > >> >> > + if (opts->bh.ver == 1) { > >> >> > if (mask) { > >> >> > ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",", > >> >> > - ntohl(opts->u.index), > >> >> > - ntohl(mask->u.index)); > >> >> > + ntohl(get_16aligned_be32(&opts->u.index)), > >> >> > + ntohl(get_16aligned_be32(&mask->u.index))); > >> >> > } else { > >> >> > - ds_put_format(ds, "idx=%#"PRIx32",", > >> >> > ntohl(opts->u.index)); > >> >> > + ds_put_format(ds, "idx=%#"PRIx32",", > >> >> > + ntohl(get_16aligned_be32(&opts->u.index))); > >> >> > } > >> >> > - } else if (opts->version == 2) { > >> >> > + } else if (opts->bh.ver == 2) { > >> >> > dir = opts->u.md2.dir; > >> >> > hwid = opts->u.md2.hwid; > >> >> > if (mask) { > >> >> > @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s, > >> >> > > >> >> > if (!strncmp(s, ")", 1)) { > >> >> > s += 1; > >> >> > - key->version = ver; > >> >> > - key->u.index = htonl(idx); > >> >> > + memset(&key->bh, 0, sizeof key->bh); > >> >> > + key->bh.ver = ver; > >> >> > + put_16aligned_be32(&key->u.index, htonl(idx)); > >> >> > if (mask) { > >> >> > - mask->u.index = htonl(idx_mask); > >> >> > + put_16aligned_be32(&mask->u.index, htonl(idx_mask)); > >> >> > } > >> >> > } > >> >> > return s - s_base; > >> >> > @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s, > >> >> > > >> >> > if (!strncmp(s, ")", 1)) { > >> >> > s += 1; > >> >> > - key->version = ver; > >> >> > + memset(&key->bh, 0, sizeof key->bh); > >> >> > + key->bh.ver = ver; > >> >> > key->u.md2.hwid = hwid; > >> >> > key->u.md2.dir = dir; > >> >> > if (mask) { > >> >> > diff --git a/lib/packets.h b/lib/packets.h > >> >> > index 7645a9d..5c013a3 100644 > >> >> > --- a/lib/packets.h > >> >> > +++ b/lib/packets.h > >> >> > @@ -1312,10 +1312,10 @@ struct erspan_md2 { > >> >> > }; > >> >> > > >> >> > struct erspan_metadata { > >> >> > - int version; > >> >> > + struct erspan_base_hdr bh; > >> >> > union { > >> >> > - ovs_be32 index; /* Version 1 (type II)*/ > >> >> > - struct erspan_md2 md2; /* Version 2 (type III) */ > >> >> > + ovs_16aligned_be32 index; /* Version 1 (type II). */ > >> >> > + struct erspan_md2 md2; /* Version 2 (type III). */ > >> >> > } u; > >> >> > }; > >> >> > > >> >> Thanks for the patch. > >> >> > >> >> We shouldn't change this header since struct erspan_metadata is exposed > >> >> from kernel's UAPI header. (see include/uapi/linux/erspan.h). > >> >> OVS kernel module expects this binary format. > >> > > >> > We often use different data structures in userspace and the kernel. > >> > This only changes the userspace one. > >> > > >> > It's a bad idea to use "int" for wire formats because its size can vary > >> > among platforms (although the Linux kernel does rely on it being 32 > >> > bits, we don't make the same assumption in our userspace). > >> > > >> > >> the 'int version' here is not wire format. It is to determine version 1 or > >> 2 > >> so that the metadata mode tunnel device knows to whether use > >> ovs_be32 index or, > >> struct erspan_md2 md2 (which are wire format) > >> > >> > It's a bad idea to use "ovs_be32" in packet formats, in userspace, > >> > because packets aren't always aligned on 32-bit boundaries. > >> > >> I see. Then I think we should do: > >> @@ -1314,7 +1314,7 @@ struct erspan_md2 { > >> struct erspan_metadata { > >> int version; > >> union { > >> - ovs_be32 index; /* Version 1 (type II)*/ > >> + ovs_16aligned_be32 index; /* Version 1 (type II)*/ > >> struct erspan_md2 md2; /* Version 2 (type III) */ > >> } u; > >> }; > > > > OK, now I understand what's going on. This is only used to define the > > format of the OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS attribute. Since it's not > > a wire format at all, we don't need to use ovs_16aligned_be32 either, > > and an "int" is fine too. > > > > Since erspan_metadata is part of the kernel ABI, which is exposed via > > Netlink, it should normally be defined by including a kernel header > > rather than in packets.h, which normally just defines wire formats for > > things. Can we arrange for that to happen? > > > > Sure. > > > Thanks, > > > > Ben. > > > > Also, I guess that we could just use the data in-place: > > > Yes, thanks. > I will submit a separate patch to fix this.
Thanks. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
