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.am...@intel.com>
>> Sent: Wednesday, April 27, 2022 4:49 PM
>> To: ovs-dev@openvswitch.org
>> Cc: echau...@redhat.com; ktray...@redhat.com; i.maxim...@ovn.org;
>> Ferriter, Cian <cian.ferri...@intel.com>; Stokes, Ian <ian.sto...@intel.com>;
>> f...@sysclose.org; Van Haaren, Harry <harry.van.haa...@intel.com>; Amber,
>> Kumar <kumar.am...@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.am...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
