Re: [ovs-dev] [v8 02/12] dpif-netdev: Add auto validation function for miniflow extract

2021-07-11 Thread Amber, Kumar
Hi Flavio




> > +if (keys_size < cnt) {
> > +miniflow_extract_func default_func = NULL;
> > +atomic_uintptr_t *pmd_func = (void *)>miniflow_extract_opt;
> > +atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
> > +VLOG_ERR("Invalid key size supplied, Key_size: %d less than"
> > + "batch_size:  %" PRIuSIZE"", keys_size, cnt);
> 
> Please mention that the autovalidator is disabled in this pmd too.
> 

Added a comment in v9.
> > +return 0;
> > +}
> > +
> > +/* Run scalar miniflow_extract to get default result. */
> > +DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> > +pkt_metadata_init(>md, in_port);
> > +miniflow_extract(packet, [i].mf);
> > +
> > +/* Store known good metadata to compare with optimized metadata. */
> > +good_l2_5_ofs[i] = packet->l2_5_ofs;
> > +good_l3_ofs[i] = packet->l3_ofs;
> > +good_l4_ofs[i] = packet->l4_ofs;
> > +good_l2_pad_size[i] = packet->l2_pad_size;
> > +}
> > +
> > +uint32_t batch_failed = 0;
> > +/* Iterate through each version of miniflow implementations. */
> > +for (int j = MFEX_IMPL_START_IDX; j < MFEX_IMPL_MAX; j++) {
> > +if ((j < MFEX_IMPL_MAX) || (!mfex_impls[j].available)) {
> 
> With the fix for MFEX_IMPL_MAX in the previous patch and fixing the define for
> MFEX_IMPL_START_IDX in the future patch, there is no need to check if (j <
> MFEX_IMPL_MAX).
> 
> 
Make sense fixed in v9.
> 

Regards
Amber
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [v8 02/12] dpif-netdev: Add auto validation function for miniflow extract

2021-07-10 Thread Flavio Leitner
On Fri, Jul 09, 2021 at 05:35:52PM +0530, kumar Amber wrote:
> From: Kumar Amber 
> 
> This patch introduced the auto-validation function which
> allows users to compare the batch of packets obtained from
> different miniflow implementations against the linear
> miniflow extract and return a hitmask.
> 
> The autovaidator function can be triggered at runtime using the
> following command:
> 
> $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator
> 
> Signed-off-by: Kumar Amber 
> Co-authored-by: Harry van Haaren 
> Signed-off-by: Harry van Haaren 
> 
> ---
> v6:
> -fix review comments(Eelco)
> v5:
> - fix review comments(Ian, Flavio, Eelco)
> - remove ovs assert and switch to default after a batch of packets
>   is processed
> - Atomic set and get introduced
> - fix raw_ctz for windows build
> ---
> ---
>  NEWS  |   2 +
>  lib/dpif-netdev-private-extract.c | 150 ++
>  lib/dpif-netdev-private-extract.h |  22 +
>  lib/dpif-netdev.c |   2 +-
>  4 files changed, 175 insertions(+), 1 deletion(-)
> 
> diff --git a/NEWS b/NEWS
> index 413ceec6c..e8b4e0405 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -29,6 +29,8 @@ Post-v2.15.0
> CPU supports it. This enhances performance by using the native 
> vpopcount
> instructions, instead of the emulated version of vpopcount.
>   * Add command line option to switch between mfex function pointers.
> + * Add miniflow extract auto-validator function to compare different
> +   miniflow extract implementations against default implementation.
> - ovs-ctl:
>   * New option '--no-record-hostname' to disable hostname configuration
> in ovsdb on startup.
> diff --git a/lib/dpif-netdev-private-extract.c 
> b/lib/dpif-netdev-private-extract.c
> index 1cf133736..2cc1bc9d5 100644
> --- a/lib/dpif-netdev-private-extract.c
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -38,6 +38,11 @@ static miniflow_extract_func default_mfex_func = NULL;
>   */
>  static struct dpif_miniflow_extract_impl mfex_impls[] = {
>  
> +[MFEX_IMPL_AUTOVALIDATOR] = {
> +.probe = NULL,
> +.extract_func = dpif_miniflow_extract_autovalidator,
> +.name = "autovalidator", },
> +
>  [MFEX_IMPL_SCALAR] = {
>  .probe = NULL,
>  .extract_func = NULL,
> @@ -156,3 +161,148 @@ dp_mfex_impl_get_by_name(const char *name, 
> miniflow_extract_func *out_func)
>  
>  return -ENOENT;
>  }
> +
> +uint32_t
> +dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets,
> +struct netdev_flow_key *keys,
> +uint32_t keys_size, odp_port_t in_port,
> +struct dp_netdev_pmd_thread *pmd_handle)
> +{
> +const size_t cnt = dp_packet_batch_size(packets);
> +uint16_t good_l2_5_ofs[NETDEV_MAX_BURST];
> +uint16_t good_l3_ofs[NETDEV_MAX_BURST];
> +uint16_t good_l4_ofs[NETDEV_MAX_BURST];
> +uint16_t good_l2_pad_size[NETDEV_MAX_BURST];
> +struct dp_packet *packet;
> +struct dp_netdev_pmd_thread *pmd = pmd_handle;
> +struct netdev_flow_key test_keys[NETDEV_MAX_BURST];
> +
> +if (keys_size < cnt) {
> +miniflow_extract_func default_func = NULL;
> +atomic_uintptr_t *pmd_func = (void *)>miniflow_extract_opt;
> +atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
> +VLOG_ERR("Invalid key size supplied, Key_size: %d less than"
> + "batch_size:  %" PRIuSIZE"", keys_size, cnt);

Please mention that the autovalidator is disabled in this pmd too.

> +return 0;
> +}
> +
> +/* Run scalar miniflow_extract to get default result. */
> +DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> +pkt_metadata_init(>md, in_port);
> +miniflow_extract(packet, [i].mf);
> +
> +/* Store known good metadata to compare with optimized metadata. */
> +good_l2_5_ofs[i] = packet->l2_5_ofs;
> +good_l3_ofs[i] = packet->l3_ofs;
> +good_l4_ofs[i] = packet->l4_ofs;
> +good_l2_pad_size[i] = packet->l2_pad_size;
> +}
> +
> +uint32_t batch_failed = 0;
> +/* Iterate through each version of miniflow implementations. */
> +for (int j = MFEX_IMPL_START_IDX; j < MFEX_IMPL_MAX; j++) {
> +if ((j < MFEX_IMPL_MAX) || (!mfex_impls[j].available)) {

With the fix for MFEX_IMPL_MAX in the previous patch and fixing
the define for MFEX_IMPL_START_IDX in the future patch, there is
no need to check if (j < MFEX_IMPL_MAX).



> +continue;
> +}
> +
> +/* Reset keys and offsets before each implementation. */
> +memset(test_keys, 0, keys_size * sizeof(struct netdev_flow_key));
> +DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> +dp_packet_reset_offsets(packet);
> +}
> +/* Call optimized miniflow for each batch of packet. */
> +uint32_t hit_mask =