[ovs-dev] OVS DPDK: dpdk_merge pull request

Hi Ben,

The following changes since commit 1ae83bb20677b42d63dbb2140fa8ed3144c6260f:

  netdev-tc-offloads: Add support for action set (2017-11-16 08:10:29 -0800)

are available in the git repository at:

  https://github.com/istokes/ovs dpdk_merge

for you to fetch changes up to af5b0dad30359d14d6412eb80e81783a83a678ce:

  netdev-dpdk: Fix mempool creation with large MTU. (2017-11-17 16:26:33 +0000)

----------------------------------------------------------------
Ilya Maximets (5):
      netdev-dpdk: Fix dpdk_mp leak in case of EEXIST.
      netdev-dpdk: Factor out struct dpdk_mp.
      netdev-dpdk: Remove unused MAX_NB_MBUF.
      netdev-dpdk: Fix calling vhost API with negative vid.
      netdev-dpdk: Fix mempool creation with large MTU.

Kevin Traynor (1):
      dpif-netdev: Rename rxq_interval.

Mark Kavanagh (1):
      netdev-dpdk: replace uint8_t with dpdk_port_t

 lib/dpif-netdev.c |   8 ++++----
 lib/netdev-dpdk.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------------------------------------
 2 files changed, 82 insertions(+), 145 deletions(-)

Thanks
Ian

On Fri, Nov 17, 2017 at 07:27:51PM +0000, Stokes, Ian wrote:
> The following changes since commit 1ae83bb20677b42d63dbb2140fa8ed3144c6260f:
> 
>   netdev-tc-offloads: Add support for action set (2017-11-16 08:10:29 -0800)
> 
> are available in the git repository at:
> 
>   https://github.com/istokes/ovs dpdk_merge
> 
> for you to fetch changes up to af5b0dad30359d14d6412eb80e81783a83a678ce:
> 
>   netdev-dpdk: Fix mempool creation with large MTU. (2017-11-17 16:26:33 +0000)

Thanks a lot.  I merged this branch into master.

This yields a new "sparse" warning:
    ../lib/netdev-tc-offloads.c:584:20: warning: Variable length array is used.
because of this declaration in parse_put_flow_set_masked_action():
    char *set_buff[set_len], *set_data, *set_mask;

It may or may not be worth fixing that.  In favor of fixing it is
keeping OVS sparse warning free, but against it is that the replacement
would probably be malloc(), which is slower.

However, looking a bit closer (now that I've already done the merge), I
think there is a bug here.  set_buff[] is declared as char
*set_buff[set_len], which has size (set_len * sizeof(char *)), but only
set_len bytes of it are used.  I think that the right declaration would
be more like uint8_t set_buff[set_len];, although that would lack the
proper alignment for struct nl_attr.

Maybe something like this would be the appropriate fix for both issues.
Will you please review it?

Thanks,

Ben.

diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index 781d849e4a87..d6b61f6360d0 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -581,7 +581,9 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
                                  bool hasmask)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
-    char *set_buff[set_len], *set_data, *set_mask;
+    uint64_t set_stub[1024 / 8];
+    struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
+    char *set_data, *set_mask;
     char *key = (char *) &flower->rewrite.key;
     char *mask = (char *) &flower->rewrite.mask;
     const struct nlattr *attr;
@@ -589,8 +591,7 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
     size_t size;
 
     /* copy so we can set attr mask to 0 for used ovs key struct members  */
-    memcpy(set_buff, set, set_len);
-    attr = (struct nlattr *) set_buff;
+    attr = ofpbuf_put(&set_buf, set, set_len);
 
     type = nl_attr_type(attr);
     size = nl_attr_get_size(attr) / 2;
@@ -600,6 +601,7 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
     if (type >= ARRAY_SIZE(set_flower_map)
         || !set_flower_map[type][0].size) {
         VLOG_DBG_RL(&rl, "unsupported set action type: %d", type);
+        ofpbuf_uninit(&set_buf);
         return EOPNOTSUPP;
     }
 
@@ -632,9 +634,11 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
     if (hasmask && !is_all_zeros(set_mask, size)) {
         VLOG_DBG_RL(&rl, "unsupported sub attribute of set action type %d",
                     type);
+        ofpbuf_uninit(&set_buf);
         return EOPNOTSUPP;
     }
 
+    ofpbuf_uninit(&set_buf);
     return 0;
 }

> On Fri, Nov 17, 2017 at 07:27:51PM +0000, Stokes, Ian wrote:
> > The following changes since commit
> 1ae83bb20677b42d63dbb2140fa8ed3144c6260f:
> >
> >   netdev-tc-offloads: Add support for action set (2017-11-16 08:10:29
> > -0800)
> >
> > are available in the git repository at:
> >
> >   https://github.com/istokes/ovs dpdk_merge
> >
> > for you to fetch changes up to af5b0dad30359d14d6412eb80e81783a83a678ce:
> >
> >   netdev-dpdk: Fix mempool creation with large MTU. (2017-11-17
> > 16:26:33 +0000)
> 
> Thanks a lot.  I merged this branch into master.

Thanks Ben.

> 
> This yields a new "sparse" warning:
>     ../lib/netdev-tc-offloads.c:584:20: warning: Variable length array is
> used.
> because of this declaration in parse_put_flow_set_masked_action():
>     char *set_buff[set_len], *set_data, *set_mask;
> 

So this was introduced in commit 1ae83bb20677b42d63dbb2140fa8ed3144c6260f: netdev-tc-offloads: Add support for action set. This was pushed just after I had submitted the pull request (but wasn't actually part of the pull request patches).

It's strange though, I ran sparse before submitting the pull request and it came back clean. Checking after the merge and I don't see the warning you see. (Normally I would not submit a pull request if a sparse warning occurred). What version of sparse are you using out of interest? I'm using sparse-0.5.0-10.
 
> It may or may not be worth fixing that.  In favor of fixing it is keeping
> OVS sparse warning free, but against it is that the replacement would
> probably be malloc(), which is slower.
> 
> However, looking a bit closer (now that I've already done the merge), I
> think there is a bug here.  set_buff[] is declared as char
> *set_buff[set_len], which has size (set_len * sizeof(char *)), but only
> set_len bytes of it are used.  I think that the right declaration would be
> more like uint8_t set_buff[set_len];, although that would lack the proper
> alignment for struct nl_attr.
> 
> Maybe something like this would be the appropriate fix for both issues.
> Will you please review it?

Sure, will be happy to review, but would also be good to get someone with tc-offload experience to sign off also.

Roi has also submitted a patch to fix the issue, maybe Roi could review below also?

https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341111.html

> 
> Thanks,
> 
> Ben.
> 
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c index
> 781d849e4a87..d6b61f6360d0 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -581,7 +581,9 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
>                                   bool hasmask)  {
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> -    char *set_buff[set_len], *set_data, *set_mask;
> +    uint64_t set_stub[1024 / 8];
> +    struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
> +    char *set_data, *set_mask;
>      char *key = (char *) &flower->rewrite.key;
>      char *mask = (char *) &flower->rewrite.mask;
>      const struct nlattr *attr;
> @@ -589,8 +591,7 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
>      size_t size;
> 
>      /* copy so we can set attr mask to 0 for used ovs key struct members
> */
> -    memcpy(set_buff, set, set_len);
> -    attr = (struct nlattr *) set_buff;
> +    attr = ofpbuf_put(&set_buf, set, set_len);
> 
>      type = nl_attr_type(attr);
>      size = nl_attr_get_size(attr) / 2;
> @@ -600,6 +601,7 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
>      if (type >= ARRAY_SIZE(set_flower_map)
>          || !set_flower_map[type][0].size) {
>          VLOG_DBG_RL(&rl, "unsupported set action type: %d", type);
> +        ofpbuf_uninit(&set_buf);
>          return EOPNOTSUPP;
>      }
> 
> @@ -632,9 +634,11 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
>      if (hasmask && !is_all_zeros(set_mask, size)) {
>          VLOG_DBG_RL(&rl, "unsupported sub attribute of set action type
> %d",
>                      type);
> +        ofpbuf_uninit(&set_buf);
>          return EOPNOTSUPP;
>      }
> 
> +    ofpbuf_uninit(&set_buf);
>      return 0;
>  }
>

On Tue, Nov 21, 2017 at 01:14:53PM +0000, Stokes, Ian wrote:
> > On Fri, Nov 17, 2017 at 07:27:51PM +0000, Stokes, Ian wrote:
> > > The following changes since commit
> > 1ae83bb20677b42d63dbb2140fa8ed3144c6260f:
> > >
> > >   netdev-tc-offloads: Add support for action set (2017-11-16 08:10:29
> > > -0800)
> > >
> > > are available in the git repository at:
> > >
> > >   https://github.com/istokes/ovs dpdk_merge
> > >
> > > for you to fetch changes up to af5b0dad30359d14d6412eb80e81783a83a678ce:
> > >
> > >   netdev-dpdk: Fix mempool creation with large MTU. (2017-11-17
> > > 16:26:33 +0000)
> > 
> > Thanks a lot.  I merged this branch into master.
> 
> Thanks Ben.
> 
> > 
> > This yields a new "sparse" warning:
> >     ../lib/netdev-tc-offloads.c:584:20: warning: Variable length array is
> > used.
> > because of this declaration in parse_put_flow_set_masked_action():
> >     char *set_buff[set_len], *set_data, *set_mask;
> > 
> 
> So this was introduced in commit 1ae83bb20677b42d63dbb2140fa8ed3144c6260f: netdev-tc-offloads: Add support for action set. This was pushed just after I had submitted the pull request (but wasn't actually part of the pull request patches).
> 
> It's strange though, I ran sparse before submitting the pull request and it came back clean. Checking after the merge and I don't see the warning you see. (Normally I would not submit a pull request if a sparse warning occurred). What version of sparse are you using out of interest? I'm using sparse-0.5.0-10.
>  
> > It may or may not be worth fixing that.  In favor of fixing it is keeping
> > OVS sparse warning free, but against it is that the replacement would
> > probably be malloc(), which is slower.
> > 
> > However, looking a bit closer (now that I've already done the merge), I
> > think there is a bug here.  set_buff[] is declared as char
> > *set_buff[set_len], which has size (set_len * sizeof(char *)), but only
> > set_len bytes of it are used.  I think that the right declaration would be
> > more like uint8_t set_buff[set_len];, although that would lack the proper
> > alignment for struct nl_attr.
> > 
> > Maybe something like this would be the appropriate fix for both issues.
> > Will you please review it?
> 
> Sure, will be happy to review, but would also be good to get someone with tc-offload experience to sign off also.
> 
> Roi has also submitted a patch to fix the issue, maybe Roi could review below also?

Roi's patch got merged, so that fixes the warning.

I still think there is some merit in my additional changes, so I'll
submit a revised patch.