[ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

Darrell Ball dlu998 at gmail.com
Fri May 25 08:06:05 UTC 2018


On Thu, May 24, 2018 at 2:16 PM, Ben Pfaff <blp at 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 at 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 at 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 at gmail.com>
> > >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> > >> > Signed-off-by: Darrell Ball <dlu998 at 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__);
>              }
>


More information about the dev mailing list