| Message ID | 1527128037-63000-2-git-send-email-dlu998@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4,1/2] netdev-native-tnl: Fix alignment for erspan index. | expand |
On Wed, May 23, 2018 at 7:13 PM, Darrell Ball <dlu998@gmail.com> 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 <u9012063@gmail.com> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") > Signed-off-by: Darrell Ball <dlu998@gmail.com> > --- > 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. Regards, William
On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote: > On Wed, May 23, 2018 at 7:13 PM, Darrell Ball <dlu998@gmail.com> 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 <u9012063@gmail.com> > > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > --- > > 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). It's a bad idea to use "ovs_be32" in packet formats, in userspace, because packets aren't always aligned on 32-bit boundaries.
On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff <blp@ovn.org> 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 <dlu998@gmail.com> 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 <u9012063@gmail.com> >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") >> > Signed-off-by: Darrell Ball <dlu998@gmail.com> >> > --- >> > 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; }; Thanks! William
On Thu, May 24, 2018 at 12:08:31PM -0700, William Tu wrote: > On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff <blp@ovn.org> 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 <dlu998@gmail.com> 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 <u9012063@gmail.com> > >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") > >> > Signed-off-by: Darrell Ball <dlu998@gmail.com> > >> > --- > >> > 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? Thanks, Ben. Also, I guess that we could just use the data in-place: diff --git a/lib/odp-util.c b/lib/odp-util.c index 105ac809073e..5e858f0f9797 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -2781,17 +2781,14 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, tun_metadata_from_geneve_nlattr(a, is_mask, tun); break; case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS: { - int attr_len = nl_attr_get_size(a); - struct erspan_metadata opts; + const struct erspan_metadata *opts = nl_attr_get(a); - memcpy(&opts, nl_attr_get(a), attr_len); - - tun->erspan_ver = opts.version; + tun->erspan_ver = opts->version; if (tun->erspan_ver == 1) { - tun->erspan_idx = ntohl(opts.u.index); + tun->erspan_idx = ntohl(opts->u.index); } else if (tun->erspan_ver == 2) { - tun->erspan_dir = opts.u.md2.dir; - tun->erspan_hwid = get_hwid(&opts.u.md2); + tun->erspan_dir = opts->u.md2.dir; + tun->erspan_hwid = get_hwid(&opts->u.md2); } else { VLOG_WARN("%s invalid erspan version\n", __func__); }
On Thu, May 24, 2018 at 2:16 PM, Ben Pfaff <blp@ovn.org> 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 <blp@ovn.org> 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 <dlu998@gmail.com> 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 <u9012063@gmail.com> >> >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") >> >> > Signed-off-by: Darrell Ball <dlu998@gmail.com> >> >> > --- >> >> > 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. William
On Thu, May 24, 2018 at 05:22:37PM -0700, William Tu wrote: > On Thu, May 24, 2018 at 2:16 PM, Ben Pfaff <blp@ovn.org> 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 <blp@ovn.org> 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 <dlu998@gmail.com> 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 <u9012063@gmail.com> > >> >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") > >> >> > Signed-off-by: Darrell Ball <dlu998@gmail.com> > >> >> > --- > >> >> > 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.
On Thu, May 24, 2018 at 2:16 PM, Ben Pfaff <blp@ovn.org> 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 <blp@ovn.org> 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 <dlu998@gmail.com> > 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 <u9012063@gmail.com> > > >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") > > >> > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > >> > --- > > >> > 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. > > This patch (or variation) is unnecessary. Having a single definition of struct erspan_metadata (as you propose) should make the situation clearer. The '32 bit erspan version' (in lieu of the erspan base hdr incl 4 bit ver) is used in below tx cases: (also in pkt rcv processing) On the V4 side: static void erspan_fb_xmit(struct sk_buff *skb, struct net_device *dev, __be16 proto) { . . . struct erspan_metadata *md; . . . md = ip_tunnel_info_opts(tun_info); . . /* ERSPAN has fixed 8 byte GRE header */ version = md->version; On the V6 side: static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb, struct net_device *dev) . . struct erspan_metadata *md; . . md = ip_tunnel_info_opts(tun_info); . . if (md->version == 1) { The base hdr is otherwise built from other context in erspan_build_header/ erspan_build_header_v2. > 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? > > Thanks, > > Ben. > > Also, I guess that we could just use the data in-place: > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 105ac809073e..5e858f0f9797 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -2781,17 +2781,14 @@ odp_tun_key_from_attr__(const struct nlattr *attr, > bool is_mask, > tun_metadata_from_geneve_nlattr(a, is_mask, tun); > break; > case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS: { > - int attr_len = nl_attr_get_size(a); > - struct erspan_metadata opts; > + const struct erspan_metadata *opts = nl_attr_get(a); > > - memcpy(&opts, nl_attr_get(a), attr_len); > - > - tun->erspan_ver = opts.version; > + tun->erspan_ver = opts->version; > if (tun->erspan_ver == 1) { > - tun->erspan_idx = ntohl(opts.u.index); > + tun->erspan_idx = ntohl(opts->u.index); > } else if (tun->erspan_ver == 2) { > - tun->erspan_dir = opts.u.md2.dir; > - tun->erspan_hwid = get_hwid(&opts.u.md2); > + tun->erspan_dir = opts->u.md2.dir; > + tun->erspan_hwid = get_hwid(&opts->u.md2); > } else { > VLOG_WARN("%s invalid erspan version\n", __func__); > } >
> > 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? > But the kernel UAPI erspan.h defines both 'struct erspan_md2' and 'struct erspan_metadata'. If we choose to include from kernel, then the wire format 'strct erspan_md2' isn't defined using ovs_16aligned_be32. or should we generate an erspan.h using kernel UAPI erspan.h? something like: sed -f ./build-aux/extract-odp-netlink-h < datapath/linux/compat/include/linux/openvswitch.h > include/odp-netlink.h but generate include/erspan.h Thanks William
On Fri, May 25, 2018 at 05:56:51AM -0700, William Tu wrote: > > > > 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? > > > > But the kernel UAPI erspan.h defines both 'struct erspan_md2' and > 'struct erspan_metadata'. If we choose to include from kernel, then the > wire format 'strct erspan_md2' isn't defined using ovs_16aligned_be32. In some cases we have an alternative data structure for use in situations where we need it, e.g. struct ovs_16aligned_ip6_hdr.
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; };
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 <u9012063@gmail.com> Fixes: 068794b43f0e ("erspan: Add flow-based erspan options") Signed-off-by: Darrell Ball <dlu998@gmail.com> --- lib/odp-util.c | 36 ++++++++++++++++++++---------------- lib/packets.h | 6 +++--- 2 files changed, 23 insertions(+), 19 deletions(-)