| Message ID | 20220427111836.2091315-4-kumar.amber@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Miniflow Extract Testing Improvements | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Hi Ilya, Can you please provide feedback on the patch-set, if it suffices the flow based testing or needs some change to adapt it to flow based testing for AVX512 MFEX ? Regards Amber > -----Original Message----- > From: Amber, Kumar <kumar.amber@intel.com> > Sent: Wednesday, April 27, 2022 4:49 PM > To: ovs-dev@openvswitch.org > Cc: echaudro@redhat.com; ktraynor@redhat.com; i.maximets@ovn.org; > Ferriter, Cian <cian.ferriter@intel.com>; Stokes, Ian <ian.stokes@intel.com>; > fbl@sysclose.org; Van Haaren, Harry <harry.van.haaren@intel.com>; Amber, > Kumar <kumar.amber@intel.com> > Subject: [PATCH v3 3/3] flow: Add autovalidator support to miniflow_extract. > > The patch adds the flag based switch between choice of using > miniflow_extract in normal pipeline or select mfex_autovalidator in debug > and test builds. > > The compile time flag used to select autoval can be done using option: > > ./configure CFLAGS="--enable-mfex-default-autovalidator" > > Signed-off-by: Kumar Amber <kumar.amber@intel.com> > > --- > v3: > - Fix comments from Cian. > --- > --- > lib/dpif-netdev-private-extract.c | 8 ++++---- > lib/flow.c | 34 ++++++++++++++++++++++++++++++- > lib/flow.h | 4 ++++ > 3 files changed, 41 insertions(+), 5 deletions(-) > > diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private- > extract.c > index 42b970e75..bbc0e3c78 100644 > --- a/lib/dpif-netdev-private-extract.c > +++ b/lib/dpif-netdev-private-extract.c > @@ -124,8 +124,8 @@ dpif_miniflow_extract_init(void) > /* For the first call, this will be choosen based on the > * compile time flag. > */ > - VLOG_INFO("Default MFEX Extract implementation is %s.\n", > - mfex_impls[mfex_idx].name); > + VLOG_DBG("Default MFEX Extract implementation is %s.\n", > + mfex_impls[mfex_idx].name); > atomic_store_relaxed(mfex_func, (uintptr_t) mfex_impls > [mfex_idx].extract_func); } @@ -251,7 +251,7 @@ > dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets, > /* Run scalar miniflow_extract to get default result. */ > DP_PACKET_BATCH_FOR_EACH (i, packet, packets) { > pkt_metadata_init(&packet->md, in_port); > - miniflow_extract(packet, &keys[i]); > + miniflow_extract_(packet, &keys[i]); > > /* Store known good metadata to compare with optimized metadata. */ > good_l2_5_ofs[i] = packet->l2_5_ofs; @@ -347,7 +347,7 @@ > dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets, > } > > /* Having dumped the debug info for the batch, disable autovalidator. */ > - if (batch_failed) { > + if (batch_failed && (pmd != NULL)) { > atomic_store_relaxed(&pmd->miniflow_extract_opt, NULL); > } > > diff --git a/lib/flow.c b/lib/flow.c > index 086096d5e..16698cedd 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -36,6 +36,8 @@ > #include "openvswitch/match.h" > #include "dp-packet.h" > #include "dpif-netdev-private-dpcls.h" > +#include "dpif-netdev-private-dpif.h" > +#include "dpif-netdev-private-extract.h" > #include "openflow/openflow.h" > #include "packets.h" > #include "odp-util.h" > @@ -757,7 +759,7 @@ dump_invalid_packet(struct dp_packet *packet, > const char *reason) > * of interest for the flow, otherwise UINT16_MAX. > */ > void > -miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) > +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key > +*key) > { > /* Add code to this function (or its callees) to extract new fields. */ > BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42); @@ -1112,6 +1114,36 @@ > miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) > key->mf.map = mf.map; > } > > +void > +miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) > +{ #ifdef MFEX_AUTOVALIDATOR_DEFAULT > + static struct ovsthread_once once_enable = > OVSTHREAD_ONCE_INITIALIZER; > + if (ovsthread_once_start(&once_enable)) { > + dpif_miniflow_extract_init(); > + ovsthread_once_done(&once_enable); > + } > + struct dp_packet_batch packets; > + const struct pkt_metadata *md = &packet->md; > + dp_packet_batch_init(&packets); > + dp_packet_batch_add(&packets, packet); > + const uint32_t recirc_depth = *recirc_depth_get(); > + > + /* Currently AVX512 DPIF dont support recirculation > + * Once the support will be added the condition would > + * be removed. > + */ > + if (recirc_depth) { > + miniflow_extract_(packet, key); > + } else { > + dpif_miniflow_extract_autovalidator(&packets, key, 1, > + odp_to_u32(md->in_port.odp_port), NULL); > + } > +#else > + miniflow_extract_(packet, key); > +#endif > +} > + > static ovs_be16 > parse_dl_type(const void **datap, size_t *sizep, ovs_be16 *first_vlan_tci_p) > { diff --git a/lib/flow.h b/lib/flow.h index ba7c3c63a..7b277275f 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -543,6 +543,10 @@ struct pkt_metadata; > * were extracted. */ > void > miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key); > + > +void > +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key > +*key); > + > void miniflow_map_init(struct miniflow *, const struct flow *); void > flow_wc_map(const struct flow *, struct flowmap *); size_t > miniflow_alloc(struct miniflow *dsts[], size_t n, > -- > 2.25.1
On 5/26/22 12:39, Amber, Kumar wrote: > Hi Ilya, > > Can you please provide feedback on the patch-set, if it suffices the flow based > testing or needs some change to adapt it to flow based testing for AVX512 MFEX ? > > Regards > Amber > > >> -----Original Message----- >> From: Amber, Kumar <kumar.amber@intel.com> >> Sent: Wednesday, April 27, 2022 4:49 PM >> To: ovs-dev@openvswitch.org >> Cc: echaudro@redhat.com; ktraynor@redhat.com; i.maximets@ovn.org; >> Ferriter, Cian <cian.ferriter@intel.com>; Stokes, Ian <ian.stokes@intel.com>; >> fbl@sysclose.org; Van Haaren, Harry <harry.van.haaren@intel.com>; Amber, >> Kumar <kumar.amber@intel.com> >> Subject: [PATCH v3 3/3] flow: Add autovalidator support to miniflow_extract. >> >> The patch adds the flag based switch between choice of using >> miniflow_extract in normal pipeline or select mfex_autovalidator in debug >> and test builds. >> >> The compile time flag used to select autoval can be done using option: >> >> ./configure CFLAGS="--enable-mfex-default-autovalidator" >> >> Signed-off-by: Kumar Amber <kumar.amber@intel.com> >> >> --- >> v3: >> - Fix comments from Cian. >> --- >> --- >> lib/dpif-netdev-private-extract.c | 8 ++++---- >> lib/flow.c | 34 ++++++++++++++++++++++++++++++- >> lib/flow.h | 4 ++++ >> 3 files changed, 41 insertions(+), 5 deletions(-) >> >> diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private- >> extract.c >> index 42b970e75..bbc0e3c78 100644 >> --- a/lib/dpif-netdev-private-extract.c >> +++ b/lib/dpif-netdev-private-extract.c >> @@ -124,8 +124,8 @@ dpif_miniflow_extract_init(void) >> /* For the first call, this will be choosen based on the >> * compile time flag. >> */ >> - VLOG_INFO("Default MFEX Extract implementation is %s.\n", >> - mfex_impls[mfex_idx].name); >> + VLOG_DBG("Default MFEX Extract implementation is %s.\n", >> + mfex_impls[mfex_idx].name); >> atomic_store_relaxed(mfex_func, (uintptr_t) mfex_impls >> [mfex_idx].extract_func); } @@ -251,7 +251,7 @@ >> dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets, >> /* Run scalar miniflow_extract to get default result. */ >> DP_PACKET_BATCH_FOR_EACH (i, packet, packets) { >> pkt_metadata_init(&packet->md, in_port); >> - miniflow_extract(packet, &keys[i]); >> + miniflow_extract_(packet, &keys[i]); >> >> /* Store known good metadata to compare with optimized metadata. */ >> good_l2_5_ofs[i] = packet->l2_5_ofs; @@ -347,7 +347,7 @@ >> dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets, >> } >> >> /* Having dumped the debug info for the batch, disable autovalidator. */ >> - if (batch_failed) { >> + if (batch_failed && (pmd != NULL)) { >> atomic_store_relaxed(&pmd->miniflow_extract_opt, NULL); >> } >> >> diff --git a/lib/flow.c b/lib/flow.c >> index 086096d5e..16698cedd 100644 >> --- a/lib/flow.c >> +++ b/lib/flow.c >> @@ -36,6 +36,8 @@ >> #include "openvswitch/match.h" >> #include "dp-packet.h" >> #include "dpif-netdev-private-dpcls.h" >> +#include "dpif-netdev-private-dpif.h" >> +#include "dpif-netdev-private-extract.h" "private" dpif-netdev headers should not be included in non dpif-netdev modules. >> #include "openflow/openflow.h" >> #include "packets.h" >> #include "odp-util.h" >> @@ -757,7 +759,7 @@ dump_invalid_packet(struct dp_packet *packet, >> const char *reason) >> * of interest for the flow, otherwise UINT16_MAX. >> */ >> void >> -miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) >> +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key >> +*key) >> { >> /* Add code to this function (or its callees) to extract new fields. */ >> BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42); @@ -1112,6 +1114,36 @@ >> miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) >> key->mf.map = mf.map; >> } >> >> +void >> +miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) >> +{ #ifdef MFEX_AUTOVALIDATOR_DEFAULT >> + static struct ovsthread_once once_enable = >> OVSTHREAD_ONCE_INITIALIZER; >> + if (ovsthread_once_start(&once_enable)) { >> + dpif_miniflow_extract_init(); >> + ovsthread_once_done(&once_enable); >> + } >> + struct dp_packet_batch packets; >> + const struct pkt_metadata *md = &packet->md; >> + dp_packet_batch_init(&packets); >> + dp_packet_batch_add(&packets, packet); >> + const uint32_t recirc_depth = *recirc_depth_get(); >> + >> + /* Currently AVX512 DPIF dont support recirculation >> + * Once the support will be added the condition would >> + * be removed. >> + */ >> + if (recirc_depth) { >> + miniflow_extract_(packet, key); >> + } else { >> + dpif_miniflow_extract_autovalidator(&packets, key, 1, >> + odp_to_u32(md->in_port.odp_port), NULL); >> + } >> +#else >> + miniflow_extract_(packet, key); >> +#endif This doesn't seem right. First of all, it will be impossible to disable the autovalidator if it was chosen in compile time as a default option. There will be also significant performance impact. And, in general, that is not what I was asking for. The idea was to move the implementation selector here, so any part of the code, including AVX512 dpif implementation, can use same plain [mini]flow_extract() to parse the packet and have actual implementation chosen based on the current settings/priorities. Best regards, Ilya Maximets. >> +} >> + >> static ovs_be16 >> parse_dl_type(const void **datap, size_t *sizep, ovs_be16 *first_vlan_tci_p) >> { diff --git a/lib/flow.h b/lib/flow.h index ba7c3c63a..7b277275f 100644 >> --- a/lib/flow.h >> +++ b/lib/flow.h >> @@ -543,6 +543,10 @@ struct pkt_metadata; >> * were extracted. */ >> void >> miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key); >> + >> +void >> +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key >> +*key); >> + >> void miniflow_map_init(struct miniflow *, const struct flow *); void >> flow_wc_map(const struct flow *, struct flowmap *); size_t >> miniflow_alloc(struct miniflow *dsts[], size_t n, >> -- >> 2.25.1
Hi Ilya, Please find the replies inline. > >> #include "dpif-netdev-private-dpcls.h" > >> +#include "dpif-netdev-private-dpif.h" > >> +#include "dpif-netdev-private-extract.h" > > "private" dpif-netdev headers should not be included in non dpif-netdev > modules. > There are many structures which reside in these headers which we need because of the API refactors for example : struct netdev_flow_key which resides in the private-dpcls.h which mfex func API uses. > >> #include "openflow/openflow.h" > >> #include "packets.h" > >> #include "odp-util.h" > >> @@ -757,7 +759,7 @@ dump_invalid_packet(struct dp_packet *packet, > >> const char *reason) > >> * of interest for the flow, otherwise UINT16_MAX. > >> */ > >> void > >> -miniflow_extract(struct dp_packet *packet, struct netdev_flow_key > >> *key) > >> +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key > >> +*key) > >> { > >> /* Add code to this function (or its callees) to extract new fields. */ > >> BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42); @@ -1112,6 +1114,36 > @@ > >> miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) > >> key->mf.map = mf.map; > >> } > >> > >> +void > >> +miniflow_extract(struct dp_packet *packet, struct netdev_flow_key > >> +*key) { #ifdef MFEX_AUTOVALIDATOR_DEFAULT > >> + static struct ovsthread_once once_enable = > >> OVSTHREAD_ONCE_INITIALIZER; > >> + if (ovsthread_once_start(&once_enable)) { > >> + dpif_miniflow_extract_init(); > >> + ovsthread_once_done(&once_enable); > >> + } > >> + struct dp_packet_batch packets; > >> + const struct pkt_metadata *md = &packet->md; > >> + dp_packet_batch_init(&packets); > >> + dp_packet_batch_add(&packets, packet); > >> + const uint32_t recirc_depth = *recirc_depth_get(); > >> + > >> + /* Currently AVX512 DPIF dont support recirculation > >> + * Once the support will be added the condition would > >> + * be removed. > >> + */ > >> + if (recirc_depth) { > >> + miniflow_extract_(packet, key); > >> + } else { > >> + dpif_miniflow_extract_autovalidator(&packets, key, 1, > >> + odp_to_u32(md->in_port.odp_port), NULL); > >> + } > >> +#else > >> + miniflow_extract_(packet, key); > >> +#endif > > This doesn't seem right. First of all, it will be impossible to disable the > autovalidator if it was chosen in compile time as a default option. > There will be also significant performance impact. > > And, in general, that is not what I was asking for. > > The idea was to move the implementation selector here, so any part of the > code, including AVX512 dpif implementation, can use same plain > [mini]flow_extract() to parse the packet and have actual implementation > chosen based on the current settings/priorities. > We tried the following approach as suggested above, but we observed the following problems mentioned below in details: 1. Per packet based scalar mfex API vs Per batch based AVX512 APIs. We need to create a packet batch first, since that’s what the AVX512 APIs accept then process the packet then clone back the packet to the original packet. struct dp_packet_batch batch_; dp_packet_batch_init(&batch_); dp_packet_batch_add(&batch_, packet); miniflow_extract(&batch_, key, 1, odp_to_u32(port_no), pmd); packet = dp_packet_clone(batch_.packets[0]); 2. The mfex function pointer for the current implementation lives in the PMD struct. -> Not all places where miniflow_extract or flow_extract has access to pmd struct. -> To fix the problem we need to either: 1. Pass in the PMD struct everywhere. 2. Fetch the mfex implementation every time. > Best regards, Ilya Maximets. > Regards Amber <snip>
diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-extract.c index 42b970e75..bbc0e3c78 100644 --- a/lib/dpif-netdev-private-extract.c +++ b/lib/dpif-netdev-private-extract.c @@ -124,8 +124,8 @@ dpif_miniflow_extract_init(void) /* For the first call, this will be choosen based on the * compile time flag. */ - VLOG_INFO("Default MFEX Extract implementation is %s.\n", - mfex_impls[mfex_idx].name); + VLOG_DBG("Default MFEX Extract implementation is %s.\n", + mfex_impls[mfex_idx].name); atomic_store_relaxed(mfex_func, (uintptr_t) mfex_impls [mfex_idx].extract_func); } @@ -251,7 +251,7 @@ dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets, /* Run scalar miniflow_extract to get default result. */ DP_PACKET_BATCH_FOR_EACH (i, packet, packets) { pkt_metadata_init(&packet->md, in_port); - miniflow_extract(packet, &keys[i]); + miniflow_extract_(packet, &keys[i]); /* Store known good metadata to compare with optimized metadata. */ good_l2_5_ofs[i] = packet->l2_5_ofs; @@ -347,7 +347,7 @@ dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets, } /* Having dumped the debug info for the batch, disable autovalidator. */ - if (batch_failed) { + if (batch_failed && (pmd != NULL)) { atomic_store_relaxed(&pmd->miniflow_extract_opt, NULL); } diff --git a/lib/flow.c b/lib/flow.c index 086096d5e..16698cedd 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -36,6 +36,8 @@ #include "openvswitch/match.h" #include "dp-packet.h" #include "dpif-netdev-private-dpcls.h" +#include "dpif-netdev-private-dpif.h" +#include "dpif-netdev-private-extract.h" #include "openflow/openflow.h" #include "packets.h" #include "odp-util.h" @@ -757,7 +759,7 @@ dump_invalid_packet(struct dp_packet *packet, const char *reason) * of interest for the flow, otherwise UINT16_MAX. */ void -miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key *key) { /* Add code to this function (or its callees) to extract new fields. */ BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42); @@ -1112,6 +1114,36 @@ miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) key->mf.map = mf.map; } +void +miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key) +{ +#ifdef MFEX_AUTOVALIDATOR_DEFAULT + static struct ovsthread_once once_enable = OVSTHREAD_ONCE_INITIALIZER; + if (ovsthread_once_start(&once_enable)) { + dpif_miniflow_extract_init(); + ovsthread_once_done(&once_enable); + } + struct dp_packet_batch packets; + const struct pkt_metadata *md = &packet->md; + dp_packet_batch_init(&packets); + dp_packet_batch_add(&packets, packet); + const uint32_t recirc_depth = *recirc_depth_get(); + + /* Currently AVX512 DPIF dont support recirculation + * Once the support will be added the condition would + * be removed. + */ + if (recirc_depth) { + miniflow_extract_(packet, key); + } else { + dpif_miniflow_extract_autovalidator(&packets, key, 1, + odp_to_u32(md->in_port.odp_port), NULL); + } +#else + miniflow_extract_(packet, key); +#endif +} + static ovs_be16 parse_dl_type(const void **datap, size_t *sizep, ovs_be16 *first_vlan_tci_p) { diff --git a/lib/flow.h b/lib/flow.h index ba7c3c63a..7b277275f 100644 --- a/lib/flow.h +++ b/lib/flow.h @@ -543,6 +543,10 @@ struct pkt_metadata; * were extracted. */ void miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key); + +void +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key *key); + void miniflow_map_init(struct miniflow *, const struct flow *); void flow_wc_map(const struct flow *, struct flowmap *); size_t miniflow_alloc(struct miniflow *dsts[], size_t n,
The patch adds the flag based switch between choice of using miniflow_extract in normal pipeline or select mfex_autovalidator in debug and test builds. The compile time flag used to select autoval can be done using option: ./configure CFLAGS="--enable-mfex-default-autovalidator" Signed-off-by: Kumar Amber <kumar.amber@intel.com> --- v3: - Fix comments from Cian. --- --- lib/dpif-netdev-private-extract.c | 8 ++++---- lib/flow.c | 34 ++++++++++++++++++++++++++++++- lib/flow.h | 4 ++++ 3 files changed, 41 insertions(+), 5 deletions(-)