diff mbox series

[ovs-dev,v12,05/16] dpif-avx512: Add HWOL support to avx512 dpif.

Message ID 20210517140434.59555-6-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>

Partial hardware offload is implemented in a very similar way to the
scalar dpif.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 lib/dpif-netdev-avx512.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

Comments

Stokes, Ian June 1, 2021, 6:59 p.m. UTC | #1
> From: Harry van Haaren <harry.van.haaren@intel.com>
> 
> Partial hardware offload is implemented in a very similar way to the
> scalar dpif.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

Thanks for the patch Harry/Cian.

Given its size I wonder would it make sense to merge with patch 4 of the series? Was the intention to do this eventually but just separate it here for reviewing purposes?

I'd like to get a test run on this also going forward on my side so not fully finished on review.

@Finn, Emma would you be able to give this a run when you have a chance?

Minor comments below.


> ---
>  lib/dpif-netdev-avx512.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index 91f51c479..98638de15 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -27,6 +27,7 @@
>  #include "dpif-netdev-private-dpcls.h"
>  #include "dpif-netdev-private-flow.h"
>  #include "dpif-netdev-private-thread.h"
> +#include "dpif-netdev-private-hwol.h"
> 
>  #include "dp-packet.h"
>  #include "netdev.h"
> @@ -112,9 +113,32 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
>          uint32_t i = __builtin_ctz(iter);
>          iter = _blsr_u64(iter);
> 
> -        /* Initialize packet md and do miniflow extract. */
> +        /* Get packet pointer from bitmask and packet md. */
>          struct dp_packet *packet = packets->packets[i];
>          pkt_metadata_init(&packet->md, in_port);
> +
> +        struct dp_netdev_flow *f = NULL;
> +
> +        /* Check for partial hardware offload mark. */
> +        uint32_t mark;
> +        if (dp_packet_has_flow_mark(packet, &mark)) {
> +            f = mark_to_flow_find(pmd, mark);
> +            if (f) {
> +                rules[i] = &f->cr;
> +
> +                /* This is nasty - instead of using the HWOL provided flow,
> +                 * parse the packet data anyway to find the location of the TCP
> +                 * header to extract the TCP flags for the rule.
> +                 */

Is it a case that this info is just not available in the HWOL rule provided?

This overhead would have an effect on performance  but only for the AVX512 DIPF implantation correct? No effect on existing HWOL and scalar?

BR
Ian
> +                pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +
> +                pkt_meta[i].bytes = dp_packet_size(packet);
> +                hwol_emc_smc_hitmask |= (1 << i);
> +                continue;
> +            }
> +        }
> +
> +        /* Do miniflow extract into keys. */
>          struct netdev_flow_key *key = &keys[i];
>          miniflow_extract(packet, &key->mf);
> 
> @@ -125,8 +149,6 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
>          key->len = netdev_flow_key_size(miniflow_n_values(&key->mf));
>          key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key-
> >mf);
> 
> -        struct dp_netdev_flow *f = NULL;
> -
>          if (emc_enabled) {
>              f = emc_lookup(&cache->emc_cache, key);
> 
> --
> 2.31.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ferriter, Cian June 10, 2021, 2:34 p.m. UTC | #2
Hi Ian,

Thanks for the review. My responses are inline.

> -----Original Message-----
> From: Stokes, Ian <ian.stokes@intel.com>
> Sent: Tuesday 1 June 2021 19:59
> To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Finn, Emma <emma.finn@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Cc: i.maximets@ovn.org
> Subject: RE: [ovs-dev] [v12 05/16] dpif-avx512: Add HWOL support to avx512 dpif.
> 
> > From: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > Partial hardware offload is implemented in a very similar way to the
> > scalar dpif.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> Thanks for the patch Harry/Cian.
> 
> Given its size I wonder would it make sense to merge with patch 4 of the series? Was the intention to do this eventually but just
> separate it here for reviewing purposes?
> 

We added support for HWOL after adding support for DPCLS, EMC and SMC, and left it as a separate commit because it seemed logically separate, however it might make sense to combine it with patch 4, especially since patch 4 references hwol with the "hwol_emc_smc_hitmask" variable.

I'll squash this patch into patch 04 in the next version.

> I'd like to get a test run on this also going forward on my side so not fully finished on review.
> 
> @Finn, Emma would you be able to give this a run when you have a chance?
> 
> Minor comments below.
> 
> 
> > ---
> >  lib/dpif-netdev-avx512.c | 28 +++++++++++++++++++++++++---
> >  1 file changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> > index 91f51c479..98638de15 100644
> > --- a/lib/dpif-netdev-avx512.c
> > +++ b/lib/dpif-netdev-avx512.c
> > @@ -27,6 +27,7 @@
> >  #include "dpif-netdev-private-dpcls.h"
> >  #include "dpif-netdev-private-flow.h"
> >  #include "dpif-netdev-private-thread.h"
> > +#include "dpif-netdev-private-hwol.h"
> >
> >  #include "dp-packet.h"
> >  #include "netdev.h"
> > @@ -112,9 +113,32 @@ dp_netdev_input_outer_avx512(struct
> > dp_netdev_pmd_thread *pmd,
> >          uint32_t i = __builtin_ctz(iter);
> >          iter = _blsr_u64(iter);
> >
> > -        /* Initialize packet md and do miniflow extract. */
> > +        /* Get packet pointer from bitmask and packet md. */
> >          struct dp_packet *packet = packets->packets[i];
> >          pkt_metadata_init(&packet->md, in_port);
> > +
> > +        struct dp_netdev_flow *f = NULL;
> > +
> > +        /* Check for partial hardware offload mark. */
> > +        uint32_t mark;
> > +        if (dp_packet_has_flow_mark(packet, &mark)) {
> > +            f = mark_to_flow_find(pmd, mark);
> > +            if (f) {
> > +                rules[i] = &f->cr;
> > +
> > +                /* This is nasty - instead of using the HWOL provided flow,
> > +                 * parse the packet data anyway to find the location of the TCP
> > +                 * header to extract the TCP flags for the rule.
> > +                 */
> 
> Is it a case that this info is just not available in the HWOL rule provided?
> 

Yes, it's not provided by the hardware, and would usually be done in miniflow_extract, so we have to call "parse_tcp_flags()" to get the tcp_flags in software.

> This overhead would have an effect on performance  but only for the AVX512 DIPF implantation correct? No effect on existing HWOL
> and scalar?
> 

This overhead is present for both AVX512 and scalar DPIFs. I think I'll remove the comment, since it's calling out an issue which isn't specific to the AVX512 DPIF and the comment might be misleading.

> BR
> Ian
> > +                pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> > +
> > +                pkt_meta[i].bytes = dp_packet_size(packet);
> > +                hwol_emc_smc_hitmask |= (1 << i);
> > +                continue;
> > +            }
> > +        }
> > +
> > +        /* Do miniflow extract into keys. */
> >          struct netdev_flow_key *key = &keys[i];
> >          miniflow_extract(packet, &key->mf);
> >
> > @@ -125,8 +149,6 @@ dp_netdev_input_outer_avx512(struct
> > dp_netdev_pmd_thread *pmd,
> >          key->len = netdev_flow_key_size(miniflow_n_values(&key->mf));
> >          key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key-
> > >mf);
> >
> > -        struct dp_netdev_flow *f = NULL;
> > -
> >          if (emc_enabled) {
> >              f = emc_lookup(&cache->emc_cache, key);
> >
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian June 16, 2021, 11:10 a.m. UTC | #3
> Hi Ian,
> 
> Thanks for the review. My responses are inline.
> 
> > -----Original Message-----
> > From: Stokes, Ian <ian.stokes@intel.com>
> > Sent: Tuesday 1 June 2021 19:59
> > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Finn,
> Emma <emma.finn@intel.com>; Van Haaren, Harry
> > <harry.van.haaren@intel.com>
> > Cc: i.maximets@ovn.org
> > Subject: RE: [ovs-dev] [v12 05/16] dpif-avx512: Add HWOL support to avx512
> dpif.
> >
> > > From: Harry van Haaren <harry.van.haaren@intel.com>
> > >
> > > Partial hardware offload is implemented in a very similar way to the
> > > scalar dpif.
> > >
> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > Thanks for the patch Harry/Cian.
> >
> > Given its size I wonder would it make sense to merge with patch 4 of the
> series? Was the intention to do this eventually but just
> > separate it here for reviewing purposes?
> >
> 
> We added support for HWOL after adding support for DPCLS, EMC and SMC, and
> left it as a separate commit because it seemed logically separate, however it
> might make sense to combine it with patch 4, especially since patch 4 references
> hwol with the "hwol_emc_smc_hitmask" variable.
> 
> I'll squash this patch into patch 04 in the next version.
> 
> > I'd like to get a test run on this also going forward on my side so not fully
> finished on review.
> >
> > @Finn, Emma would you be able to give this a run when you have a chance?
> >
> > Minor comments below.
> >
> >
> > > ---
> > >  lib/dpif-netdev-avx512.c | 28 +++++++++++++++++++++++++---
> > >  1 file changed, 25 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> > > index 91f51c479..98638de15 100644
> > > --- a/lib/dpif-netdev-avx512.c
> > > +++ b/lib/dpif-netdev-avx512.c
> > > @@ -27,6 +27,7 @@
> > >  #include "dpif-netdev-private-dpcls.h"
> > >  #include "dpif-netdev-private-flow.h"
> > >  #include "dpif-netdev-private-thread.h"
> > > +#include "dpif-netdev-private-hwol.h"
> > >
> > >  #include "dp-packet.h"
> > >  #include "netdev.h"
> > > @@ -112,9 +113,32 @@ dp_netdev_input_outer_avx512(struct
> > > dp_netdev_pmd_thread *pmd,
> > >          uint32_t i = __builtin_ctz(iter);
> > >          iter = _blsr_u64(iter);
> > >
> > > -        /* Initialize packet md and do miniflow extract. */
> > > +        /* Get packet pointer from bitmask and packet md. */
> > >          struct dp_packet *packet = packets->packets[i];
> > >          pkt_metadata_init(&packet->md, in_port);
> > > +
> > > +        struct dp_netdev_flow *f = NULL;
> > > +
> > > +        /* Check for partial hardware offload mark. */
> > > +        uint32_t mark;
> > > +        if (dp_packet_has_flow_mark(packet, &mark)) {
> > > +            f = mark_to_flow_find(pmd, mark);
> > > +            if (f) {
> > > +                rules[i] = &f->cr;
> > > +
> > > +                /* This is nasty - instead of using the HWOL provided flow,
> > > +                 * parse the packet data anyway to find the location of the TCP
> > > +                 * header to extract the TCP flags for the rule.
> > > +                 */
> >
> > Is it a case that this info is just not available in the HWOL rule provided?
> >
> 
> Yes, it's not provided by the hardware, and would usually be done in
> miniflow_extract, so we have to call "parse_tcp_flags()" to get the tcp_flags in
> software.
> 
> > This overhead would have an effect on performance  but only for the AVX512
> DIPF implantation correct? No effect on existing HWOL
> > and scalar?
> >
> 
> This overhead is present for both AVX512 and scalar DPIFs. I think I'll remove the
> comment, since it's calling out an issue which isn't specific to the AVX512 DPIF
> and the comment might be misleading.
> 

Sure, at first review it came across that this was AVX512 specific. If you want to leave the comment above but simply add "similar to scalar implementation" just to flag this it would be fine by me.

Regards
Ian

> > BR
> > Ian
> > > +                pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> > > +
> > > +                pkt_meta[i].bytes = dp_packet_size(packet);
> > > +                hwol_emc_smc_hitmask |= (1 << i);
> > > +                continue;
> > > +            }
> > > +        }
> > > +
> > > +        /* Do miniflow extract into keys. */
> > >          struct netdev_flow_key *key = &keys[i];
> > >          miniflow_extract(packet, &key->mf);
> > >
> > > @@ -125,8 +149,6 @@ dp_netdev_input_outer_avx512(struct
> > > dp_netdev_pmd_thread *pmd,
> > >          key->len = netdev_flow_key_size(miniflow_n_values(&key->mf));
> > >          key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key-
> > > >mf);
> > >
> > > -        struct dp_netdev_flow *f = NULL;
> > > -
> > >          if (emc_enabled) {
> > >              f = emc_lookup(&cache->emc_cache, key);
> > >
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index 91f51c479..98638de15 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -27,6 +27,7 @@ 
 #include "dpif-netdev-private-dpcls.h"
 #include "dpif-netdev-private-flow.h"
 #include "dpif-netdev-private-thread.h"
+#include "dpif-netdev-private-hwol.h"
 
 #include "dp-packet.h"
 #include "netdev.h"
@@ -112,9 +113,32 @@  dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
         uint32_t i = __builtin_ctz(iter);
         iter = _blsr_u64(iter);
 
-        /* Initialize packet md and do miniflow extract. */
+        /* Get packet pointer from bitmask and packet md. */
         struct dp_packet *packet = packets->packets[i];
         pkt_metadata_init(&packet->md, in_port);
+
+        struct dp_netdev_flow *f = NULL;
+
+        /* Check for partial hardware offload mark. */
+        uint32_t mark;
+        if (dp_packet_has_flow_mark(packet, &mark)) {
+            f = mark_to_flow_find(pmd, mark);
+            if (f) {
+                rules[i] = &f->cr;
+
+                /* This is nasty - instead of using the HWOL provided flow,
+                 * parse the packet data anyway to find the location of the TCP
+                 * header to extract the TCP flags for the rule.
+                 */
+                pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
+
+                pkt_meta[i].bytes = dp_packet_size(packet);
+                hwol_emc_smc_hitmask |= (1 << i);
+                continue;
+            }
+        }
+
+        /* Do miniflow extract into keys. */
         struct netdev_flow_key *key = &keys[i];
         miniflow_extract(packet, &key->mf);
 
@@ -125,8 +149,6 @@  dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
         key->len = netdev_flow_key_size(miniflow_n_values(&key->mf));
         key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf);
 
-        struct dp_netdev_flow *f = NULL;
-
         if (emc_enabled) {
             f = emc_lookup(&cache->emc_cache, key);