diff mbox series

[ovs-dev,v12,16/16] netdev: Optimize netdev_send_prepare_batch.

Message ID 20210517140434.59555-17-cian.ferriter@intel.com
State Changes Requested
Headers show
Series DPIF Framework + Optimizations | expand

Commit Message

Ferriter, Cian May 17, 2021, 2:04 p.m. UTC 
From: Harry van Haaren <harry.van.haaren@intel.com>

Optimize for the best case here where all packets will be compatible
with 'netdev_flags'.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Co-authored-by: Cian Ferriter <cian.ferriter@intel.com>
Signed-off-by: Cian Ferriter <cian.ferriter@intel.com>
---
 NEWS         |  2 ++
 lib/netdev.c | 31 ++++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 9 deletions(-)

Comments

Stokes, Ian June 15, 2021, 5:03 p.m. UTC | #1 
> From: Harry van Haaren <harry.van.haaren@intel.com>
> 
> Optimize for the best case here where all packets will be compatible
> with 'netdev_flags'.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> Co-authored-by: Cian Ferriter <cian.ferriter@intel.com>
> Signed-off-by: Cian Ferriter <cian.ferriter@intel.com>

Thanks for the patch Cian/Harry, comments inline.

In general I have a concern of the impact on HWOL cases here and how much it would affect that. Have you data on this? Any thoughts from other HWOL folks here?
> ---
>  NEWS         |  2 ++
>  lib/netdev.c | 31 ++++++++++++++++++++++---------
>  2 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index a7ec34af1..8ad3c98db 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -18,6 +18,8 @@ Post-v2.15.0
>         CPU supports it. This enhances performance by using the native vpopcount
>         instructions, instead of the emulated version of vpopcount.
>       * Optimize dp_netdev_output by enhancing compiler optimization potential.
> +     * Optimize netdev sending by assuming the happy case, and using fallback
> +       for if the netdev doesnt meet the required HWOL needs of a packet.
This sounds a bit too low level for a NEWS item. Would suggest changing to single line Optimize netdev sending for best case scenario.

BR
Ian

>     - ovs-ctl:
>       * New option '--no-record-hostname' to disable hostname configuration
>         in ovsdb on startup.
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 91e91955c..29a5f1aa9 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -837,20 +837,33 @@ static void
>  netdev_send_prepare_batch(const struct netdev *netdev,
>                            struct dp_packet_batch *batch)
>  {
> -    struct dp_packet *packet;
> -    size_t i, size = dp_packet_batch_size(batch);
> +    struct dp_packet *p;
> +    uint32_t i, size = dp_packet_batch_size(batch);
> +    char *err_msg = NULL;
> 
> -    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
> -        char *errormsg = NULL;
> +    for (i = 0; i < size; i++) {
> +        p = batch->packets[i];
> +        int pkt_ok = netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg);
> 
> -        if (netdev_send_prepare_packet(netdev->ol_flags, packet, &errormsg)) {
> -            dp_packet_batch_refill(batch, packet, i);
> +        if (OVS_UNLIKELY(!pkt_ok)) {
> +            goto refill_loop;
> +        }
> +    }
> +
> +    return;
> +
> +refill_loop:
> +    /* Loop through packets from the start of the batch again. This is the
> +     * exceptional case where packets aren't compatible with 'netdev_flags'. */
> +    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, p, batch) {
> +        if (netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg)) {
> +            dp_packet_batch_refill(batch, p, i);
>          } else {
> -            dp_packet_delete(packet);
> +            dp_packet_delete(p);
>              COVERAGE_INC(netdev_send_prepare_drops);
>              VLOG_WARN_RL(&rl, "%s: Packet dropped: %s",
> -                         netdev_get_name(netdev), errormsg);
> -            free(errormsg);
> +                         netdev_get_name(netdev), err_msg);
> +            free(err_msg);
>          }
>      }
>  }
> --
> 2.31.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ferriter, Cian June 16, 2021, 11:14 a.m. UTC | #2 
Hi Ian,

Thanks for the review. My responses are inline.

> -----Original Message-----
> From: Stokes, Ian <ian.stokes@intel.com>
> Sent: Tuesday 15 June 2021 18:04
> To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: i.maximets@ovn.org; Gaetan Rivet <gaetanr@nvidia.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize netdev_send_prepare_batch.
> 
> > From: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > Optimize for the best case here where all packets will be compatible
> > with 'netdev_flags'.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > Co-authored-by: Cian Ferriter <cian.ferriter@intel.com>
> > Signed-off-by: Cian Ferriter <cian.ferriter@intel.com>
> 
> Thanks for the patch Cian/Harry, comments inline.
> 
> In general I have a concern of the impact on HWOL cases here and how much it would affect that. Have you data on this? Any
> thoughts from other HWOL folks here?

From our POV, we are optimizing the normal and error free case. We are impacting the error case. When a packet is not compatible with netdev_flags that packet will be deleted from the batch and a " VLOG_WARN_RL()" will be called. This is quite costly as it is.

I'm curious to see what other HWOL folks think.

> > ---
> >  NEWS         |  2 ++
> >  lib/netdev.c | 31 ++++++++++++++++++++++---------
> >  2 files changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index a7ec34af1..8ad3c98db 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -18,6 +18,8 @@ Post-v2.15.0
> >         CPU supports it. This enhances performance by using the native vpopcount
> >         instructions, instead of the emulated version of vpopcount.
> >       * Optimize dp_netdev_output by enhancing compiler optimization potential.
> > +     * Optimize netdev sending by assuming the happy case, and using fallback
> > +       for if the netdev doesnt meet the required HWOL needs of a packet.
> This sounds a bit too low level for a NEWS item. Would suggest changing to single line Optimize netdev sending for best case scenario.
> 

Agreed, your single line sounds better. I've fixed this in the next version.

> BR
> Ian
> 
> >     - ovs-ctl:
> >       * New option '--no-record-hostname' to disable hostname configuration
> >         in ovsdb on startup.
> > diff --git a/lib/netdev.c b/lib/netdev.c
> > index 91e91955c..29a5f1aa9 100644
> > --- a/lib/netdev.c
> > +++ b/lib/netdev.c
> > @@ -837,20 +837,33 @@ static void
> >  netdev_send_prepare_batch(const struct netdev *netdev,
> >                            struct dp_packet_batch *batch)
> >  {
> > -    struct dp_packet *packet;
> > -    size_t i, size = dp_packet_batch_size(batch);
> > +    struct dp_packet *p;
> > +    uint32_t i, size = dp_packet_batch_size(batch);
> > +    char *err_msg = NULL;
> >
> > -    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
> > -        char *errormsg = NULL;
> > +    for (i = 0; i < size; i++) {
> > +        p = batch->packets[i];
> > +        int pkt_ok = netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg);
> >
> > -        if (netdev_send_prepare_packet(netdev->ol_flags, packet, &errormsg)) {
> > -            dp_packet_batch_refill(batch, packet, i);
> > +        if (OVS_UNLIKELY(!pkt_ok)) {
> > +            goto refill_loop;
> > +        }
> > +    }
> > +
> > +    return;
> > +
> > +refill_loop:
> > +    /* Loop through packets from the start of the batch again. This is the
> > +     * exceptional case where packets aren't compatible with 'netdev_flags'. */
> > +    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, p, batch) {
> > +        if (netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg)) {
> > +            dp_packet_batch_refill(batch, p, i);
> >          } else {
> > -            dp_packet_delete(packet);
> > +            dp_packet_delete(p);
> >              COVERAGE_INC(netdev_send_prepare_drops);
> >              VLOG_WARN_RL(&rl, "%s: Packet dropped: %s",
> > -                         netdev_get_name(netdev), errormsg);
> > -            free(errormsg);
> > +                         netdev_get_name(netdev), err_msg);
> > +            free(err_msg);
> >          }
> >      }
> >  }
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian June 16, 2021, 12:48 p.m. UTC | #3 
> Hi Ian,
> 
> Thanks for the review. My responses are inline.
> 
> > -----Original Message-----
> > From: Stokes, Ian <ian.stokes@intel.com>
> > Sent: Tuesday 15 June 2021 18:04
> > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van
> Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: i.maximets@ovn.org; Gaetan Rivet <gaetanr@nvidia.com>; Sriharsha
> Basavapatna <sriharsha.basavapatna@broadcom.com>
> > Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize
> netdev_send_prepare_batch.
> >
> > > From: Harry van Haaren <harry.van.haaren@intel.com>
> > >
> > > Optimize for the best case here where all packets will be compatible
> > > with 'netdev_flags'.
> > >
> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > > Co-authored-by: Cian Ferriter <cian.ferriter@intel.com>
> > > Signed-off-by: Cian Ferriter <cian.ferriter@intel.com>
> >
> > Thanks for the patch Cian/Harry, comments inline.
> >
> > In general I have a concern of the impact on HWOL cases here and how much
> it would affect that. Have you data on this? Any
> > thoughts from other HWOL folks here?
> 
> From our POV, we are optimizing the normal and error free case. We are
> impacting the error case. When a packet is not compatible with netdev_flags
> that packet will be deleted from the batch and a " VLOG_WARN_RL()" will be
> called. This is quite costly as it is.
> 
> I'm curious to see what other HWOL folks think.

+1, again probably not a blocker for the DPIF framework, could be submitted separately from the series if required.

Regards
Ian
> 
> > > ---
> > >  NEWS         |  2 ++
> > >  lib/netdev.c | 31 ++++++++++++++++++++++---------
> > >  2 files changed, 24 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/NEWS b/NEWS
> > > index a7ec34af1..8ad3c98db 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -18,6 +18,8 @@ Post-v2.15.0
> > >         CPU supports it. This enhances performance by using the native
> vpopcount
> > >         instructions, instead of the emulated version of vpopcount.
> > >       * Optimize dp_netdev_output by enhancing compiler optimization
> potential.
> > > +     * Optimize netdev sending by assuming the happy case, and using
> fallback
> > > +       for if the netdev doesnt meet the required HWOL needs of a packet.
> > This sounds a bit too low level for a NEWS item. Would suggest changing to
> single line Optimize netdev sending for best case scenario.
> >
> 
> Agreed, your single line sounds better. I've fixed this in the next version.
> 
> > BR
> > Ian
> >
> > >     - ovs-ctl:
> > >       * New option '--no-record-hostname' to disable hostname configuration
> > >         in ovsdb on startup.
> > > diff --git a/lib/netdev.c b/lib/netdev.c
> > > index 91e91955c..29a5f1aa9 100644
> > > --- a/lib/netdev.c
> > > +++ b/lib/netdev.c
> > > @@ -837,20 +837,33 @@ static void
> > >  netdev_send_prepare_batch(const struct netdev *netdev,
> > >                            struct dp_packet_batch *batch)
> > >  {
> > > -    struct dp_packet *packet;
> > > -    size_t i, size = dp_packet_batch_size(batch);
> > > +    struct dp_packet *p;
> > > +    uint32_t i, size = dp_packet_batch_size(batch);
> > > +    char *err_msg = NULL;
> > >
> > > -    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
> > > -        char *errormsg = NULL;
> > > +    for (i = 0; i < size; i++) {
> > > +        p = batch->packets[i];
> > > +        int pkt_ok = netdev_send_prepare_packet(netdev->ol_flags, p,
> &err_msg);
> > >
> > > -        if (netdev_send_prepare_packet(netdev->ol_flags, packet, &errormsg))
> {
> > > -            dp_packet_batch_refill(batch, packet, i);
> > > +        if (OVS_UNLIKELY(!pkt_ok)) {
> > > +            goto refill_loop;
> > > +        }
> > > +    }
> > > +
> > > +    return;
> > > +
> > > +refill_loop:
> > > +    /* Loop through packets from the start of the batch again. This is the
> > > +     * exceptional case where packets aren't compatible with 'netdev_flags'.
> */
> > > +    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, p, batch) {
> > > +        if (netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg)) {
> > > +            dp_packet_batch_refill(batch, p, i);
> > >          } else {
> > > -            dp_packet_delete(packet);
> > > +            dp_packet_delete(p);
> > >              COVERAGE_INC(netdev_send_prepare_drops);
> > >              VLOG_WARN_RL(&rl, "%s: Packet dropped: %s",
> > > -                         netdev_get_name(netdev), errormsg);
> > > -            free(errormsg);
> > > +                         netdev_get_name(netdev), err_msg);
> > > +            free(err_msg);
> > >          }
> > >      }
> > >  }
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ferriter, Cian June 16, 2021, 4:23 p.m. UTC | #4 
> -----Original Message-----
> From: Stokes, Ian <ian.stokes@intel.com>
> Sent: Wednesday 16 June 2021 13:48
> To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: i.maximets@ovn.org; Gaetan Rivet <gaetanr@nvidia.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize netdev_send_prepare_batch.
> 
> > Hi Ian,
> >
> > Thanks for the review. My responses are inline.
> >
> > > -----Original Message-----
> > > From: Stokes, Ian <ian.stokes@intel.com>
> > > Sent: Tuesday 15 June 2021 18:04
> > > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van
> > Haaren, Harry <harry.van.haaren@intel.com>
> > > Cc: i.maximets@ovn.org; Gaetan Rivet <gaetanr@nvidia.com>; Sriharsha
> > Basavapatna <sriharsha.basavapatna@broadcom.com>
> > > Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize
> > netdev_send_prepare_batch.
> > >
> > > > From: Harry van Haaren <harry.van.haaren@intel.com>
> > > >
> > > > Optimize for the best case here where all packets will be compatible
> > > > with 'netdev_flags'.
> > > >
> > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > > > Co-authored-by: Cian Ferriter <cian.ferriter@intel.com>
> > > > Signed-off-by: Cian Ferriter <cian.ferriter@intel.com>
> > >
> > > Thanks for the patch Cian/Harry, comments inline.
> > >
> > > In general I have a concern of the impact on HWOL cases here and how much
> > it would affect that. Have you data on this? Any
> > > thoughts from other HWOL folks here?
> >
> > From our POV, we are optimizing the normal and error free case. We are
> > impacting the error case. When a packet is not compatible with netdev_flags
> > that packet will be deleted from the batch and a " VLOG_WARN_RL()" will be
> > called. This is quite costly as it is.
> >
> > I'm curious to see what other HWOL folks think.
> 
> +1, again probably not a blocker for the DPIF framework, could be submitted separately from the series if required.
> 

I'll submit this patch separately from the DPIF series.

> Regards
> Ian
> >
> > > > ---
> > > >  NEWS         |  2 ++
> > > >  lib/netdev.c | 31 ++++++++++++++++++++++---------
> > > >  2 files changed, 24 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/NEWS b/NEWS
> > > > index a7ec34af1..8ad3c98db 100644
> > > > --- a/NEWS
> > > > +++ b/NEWS
> > > > @@ -18,6 +18,8 @@ Post-v2.15.0
> > > >         CPU supports it. This enhances performance by using the native
> > vpopcount
> > > >         instructions, instead of the emulated version of vpopcount.
> > > >       * Optimize dp_netdev_output by enhancing compiler optimization
> > potential.
> > > > +     * Optimize netdev sending by assuming the happy case, and using
> > fallback
> > > > +       for if the netdev doesnt meet the required HWOL needs of a packet.
> > > This sounds a bit too low level for a NEWS item. Would suggest changing to
> > single line Optimize netdev sending for best case scenario.
> > >
> >
> > Agreed, your single line sounds better. I've fixed this in the next version.
> >
> > > BR
> > > Ian
> > >
> > > >     - ovs-ctl:
> > > >       * New option '--no-record-hostname' to disable hostname configuration
> > > >         in ovsdb on startup.
> > > > diff --git a/lib/netdev.c b/lib/netdev.c
> > > > index 91e91955c..29a5f1aa9 100644
> > > > --- a/lib/netdev.c
> > > > +++ b/lib/netdev.c
> > > > @@ -837,20 +837,33 @@ static void
> > > >  netdev_send_prepare_batch(const struct netdev *netdev,
> > > >                            struct dp_packet_batch *batch)
> > > >  {
> > > > -    struct dp_packet *packet;
> > > > -    size_t i, size = dp_packet_batch_size(batch);
> > > > +    struct dp_packet *p;
> > > > +    uint32_t i, size = dp_packet_batch_size(batch);
> > > > +    char *err_msg = NULL;
> > > >
> > > > -    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
> > > > -        char *errormsg = NULL;
> > > > +    for (i = 0; i < size; i++) {
> > > > +        p = batch->packets[i];
> > > > +        int pkt_ok = netdev_send_prepare_packet(netdev->ol_flags, p,
> > &err_msg);
> > > >
> > > > -        if (netdev_send_prepare_packet(netdev->ol_flags, packet, &errormsg))
> > {
> > > > -            dp_packet_batch_refill(batch, packet, i);
> > > > +        if (OVS_UNLIKELY(!pkt_ok)) {
> > > > +            goto refill_loop;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    return;
> > > > +
> > > > +refill_loop:
> > > > +    /* Loop through packets from the start of the batch again. This is the
> > > > +     * exceptional case where packets aren't compatible with 'netdev_flags'.
> > */
> > > > +    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, p, batch) {
> > > > +        if (netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg)) {
> > > > +            dp_packet_batch_refill(batch, p, i);
> > > >          } else {
> > > > -            dp_packet_delete(packet);
> > > > +            dp_packet_delete(p);
> > > >              COVERAGE_INC(netdev_send_prepare_drops);
> > > >              VLOG_WARN_RL(&rl, "%s: Packet dropped: %s",
> > > > -                         netdev_get_name(netdev), errormsg);
> > > > -            free(errormsg);
> > > > +                         netdev_get_name(netdev), err_msg);
> > > > +            free(err_msg);
> > > >          }
> > > >      }
> > > >  }
> > > > --
> > > > 2.31.1
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index a7ec34af1..8ad3c98db 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,8 @@  Post-v2.15.0
        CPU supports it. This enhances performance by using the native vpopcount
        instructions, instead of the emulated version of vpopcount.
      * Optimize dp_netdev_output by enhancing compiler optimization potential.
+     * Optimize netdev sending by assuming the happy case, and using fallback
+       for if the netdev doesnt meet the required HWOL needs of a packet.
    - ovs-ctl:
      * New option '--no-record-hostname' to disable hostname configuration
        in ovsdb on startup.
diff --git a/lib/netdev.c b/lib/netdev.c
index 91e91955c..29a5f1aa9 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -837,20 +837,33 @@  static void
 netdev_send_prepare_batch(const struct netdev *netdev,
                           struct dp_packet_batch *batch)
 {
-    struct dp_packet *packet;
-    size_t i, size = dp_packet_batch_size(batch);
+    struct dp_packet *p;
+    uint32_t i, size = dp_packet_batch_size(batch);
+    char *err_msg = NULL;
 
-    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
-        char *errormsg = NULL;
+    for (i = 0; i < size; i++) {
+        p = batch->packets[i];
+        int pkt_ok = netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg);
 
-        if (netdev_send_prepare_packet(netdev->ol_flags, packet, &errormsg)) {
-            dp_packet_batch_refill(batch, packet, i);
+        if (OVS_UNLIKELY(!pkt_ok)) {
+            goto refill_loop;
+        }
+    }
+
+    return;
+
+refill_loop:
+    /* Loop through packets from the start of the batch again. This is the
+     * exceptional case where packets aren't compatible with 'netdev_flags'. */
+    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, p, batch) {
+        if (netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg)) {
+            dp_packet_batch_refill(batch, p, i);
         } else {
-            dp_packet_delete(packet);
+            dp_packet_delete(p);
             COVERAGE_INC(netdev_send_prepare_drops);
             VLOG_WARN_RL(&rl, "%s: Packet dropped: %s",
-                         netdev_get_name(netdev), errormsg);
-            free(errormsg);
+                         netdev_get_name(netdev), err_msg);
+            free(err_msg);
         }
     }
 }