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

Stokes, Ian ian.stokes at intel.com
Tue Jun 29 16:45:07 UTC 2021


> Hi Ian,
> 
> Thanks for reviews, replies are inlined.
> 

Thanks Amber looking forward to the v5.

BR
Ian
> 
> <Snipped>
> 
> > > +        /* Call optimized miniflow for each batch of packet. */
> > > +        uint32_t hit_mask = mfex_impls[j].extract_func(packets, test_keys,
> > > +                                            keys_size, in_port,
> > > + pmd_handle);
> >
> > Alignment above is out, should be aligned under first argument passed ie.
> > packets.
> 
> Fixed in v5.
> >
> > > +
> > > +        /* Do a miniflow compare for bits, blocks and offsets for all the
> > > +         * classified packets in the hitmask marked by set bits. */
> > > +        while (hit_mask) {
> > > +            /* Index for the set bit. */
> > > +            uint32_t i = __builtin_ctz(hit_mask);
> > > +            /* Set the index in hitmask to Zero. */
> > > +            hit_mask &= (hit_mask - 1);
> > > +
> > > +            uint32_t failed = 0;
> > > +
> > > +            /* Check miniflow bits are equal. */
> > > +            if ((keys[i].mf.map.bits[0] != test_keys[i].mf.map.bits[0]) ||
> > > +                (keys[i].mf.map.bits[1] != test_keys[i].mf.map.bits[1])) {
> > > +                VLOG_ERR("Good 0x%llx 0x%llx\tTest 0x%llx 0x%llx\n",
> > > +                         keys[i].mf.map.bits[0], keys[i].mf.map.bits[1],
> > > +                         test_keys[i].mf.map.bits[0],
> > > +                         test_keys[i].mf.map.bits[1]);
> > > +                failed = 1;
> > > +            }
> > > +
> > > +            if (!miniflow_equal(&keys[i].mf, &test_keys[i].mf)) {
> > > +                uint32_t block_cnt = miniflow_n_values(&keys[i].mf);
> > > +                VLOG_ERR("Autovalidation blocks failed for %s pkt %d",
> > > +                         mfex_impls[j].name, i);
> > > +                VLOG_ERR("  Good hexdump:\n");
> > > +                uint64_t *good_block_ptr = (uint64_t *)&keys[i].buf;
> > > +                uint64_t *test_block_ptr = (uint64_t *)&test_keys[i].buf;
> > > +                for (uint32_t b = 0; b < block_cnt; b++) {
> > > +                    VLOG_ERR("    %"PRIx64"\n", good_block_ptr[b]);
> >
> > For this and other VLOG Errs  rather than using spaces to have you thought
> > of using pad left?
> 
> Fixed in v5.
> > > +                }
> > > +                VLOG_ERR("  Test hexdump:\n");
> > > +                for (uint32_t b = 0; b < block_cnt; b++) {
> > > +                    VLOG_ERR("    %"PRIx64"\n", test_block_ptr[b]);
> > > +                }
> > > +                failed = 1;
> > > +            }
> > > +
> > > +            if ((packets->packets[i]->l2_pad_size != good_l2_pad_size[i]) ||
> > > +                    (packets->packets[i]->l2_5_ofs != good_l2_5_ofs[i]) ||
> > > +                    (packets->packets[i]->l3_ofs != good_l3_ofs[i]) ||
> > > +                    (packets->packets[i]->l4_ofs != good_l4_ofs[i])) {
> > > +                VLOG_ERR("Autovalidation packet offsets failed for %s pkt %d",
> > > +                         mfex_impls[j].name, i);
> > > +                VLOG_ERR("  Good offsets: l2_pad_size %u, l2_5_ofs : %u"
> > > +                         " l3_ofs %u, l4_ofs %u\n",
> > > +                         good_l2_pad_size[i], good_l2_5_ofs[i],
> > > +                         good_l3_ofs[i], good_l4_ofs[i]);
> > > +                VLOG_ERR("  Test offsets: l2_pad_size %u, l2_5_ofs : %u"
> > > +                         " l3_ofs %u, l4_ofs %u\n",
> > > +                         packets->packets[i]->l2_pad_size,
> > > +                         packets->packets[i]->l2_5_ofs,
> > > +                         packets->packets[i]->l3_ofs,
> > > +                         packets->packets[i]->l4_ofs);
> > > +                failed = 1;
> > > +            }
> > > +
> > > +            if (failed) {
> > > +                /* Having dumped the debug info, disable autovalidator. */
> > > +                VLOG_ERR("Autovalidation failed in %s pkt %d, disabling.\n",
> > > +                         mfex_impls[j].name, i);
> > > +                /* Halt OVS here on debug builds. */
> > > +                ovs_assert(0);
> > > +                pmd->miniflow_extract_opt = NULL;
> > > +                break;
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    /* preserve packet correctness by storing back the good offsets in
> > > +     * packets back. */
> >
> > Minor, capitalize Preserve above.
> 
> Fixed in v5.
> >
> > > +    DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> > > +        packet->l2_5_ofs = good_l2_5_ofs[i];
> > > +        packet->l3_ofs = good_l3_ofs[i];
> > > +        packet->l4_ofs = good_l4_ofs[i];
> > > +        packet->l2_pad_size = good_l2_pad_size[i];
> > > +    }
> > > +
> > > +    /* Returning zero implies no packets were hit by autovalidation. This
> > > +     * simplifies unit-tests as changing --enable-mfex-default-autovalidator
> > > +     * would pass/fail. By always returning zero, autovalidator is a little
> > > +     * slower, but we gain consistency in testing.
> >
> > Can you explain this in a little more detail?
> >
> > Is the expectation here that no packets get hit but just simply that the test
> > and comparisons match for each mfex?
> >
> 
> Details included in v5.
> > > +     */
> > > +    return 0;
> > > +}
> > > diff --git a/lib/dpif-netdev-private-extract.h
> > > b/lib/dpif-netdev-private-extract.h
> > > index b7b0b2be4..455a7b590 100644
> > > --- a/lib/dpif-netdev-private-extract.h
> > > +++ b/lib/dpif-netdev-private-extract.h
> > > @@ -24,6 +24,11 @@
> > >  /* Max size of dpif_miniflow_extract_impl array. */  #define
> > > MFEX_IMPLS_MAX_SIZE (16)
> > >
> > > +/* Skip the autovalidator study and null when iterating all available
> >
> > Is study the right word above for the autovalidator? Seems a bit odd.
> 
> Fixed in v5.
> 
> > > + * miniflow implementations.
> > > + */
> > > +#define MFEX_IMPL_START_IDX (1)
> > > +
> > >  /* Forward declarations. */
> > >  struct dp_packet;
> > >  struct miniflow;
> > > @@ -90,5 +95,15 @@ dpif_miniflow_extract_init(void);  int32_t
> > > dpif_miniflow_extract_info_get(struct dpif_miniflow_extract_impl
> > > **out_ptr);
> > >
> > > +/* Retrieve the hitmask of the batch of pakcets which is obtained by
> > > +comparing
> > > + * different miniflow implementations with linear miniflow extract.
> > > + * On error, returns a zero.
> > > + * On success, returns the number of packets in the batch compared.
> > > + */
> > > +uint32_t
> > > +dpif_miniflow_extract_autovalidator(struct dp_packet_batch *batch,
> > > +                                    struct netdev_flow_key *keys,
> > > +                                    uint32_t keys_size, odp_port_t in_port,
> > > +                                    void *pmd_handle);
> > >
> > >  #endif /* DPIF_NETDEV_AVX512_EXTRACT */ diff --git
> > > a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 567ebd952..4f4ab2790
> > > 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -1181,8 +1181,8 @@ dpif_miniflow_extract_impl_set(struct
> > > unixctl_conn *conn, int argc,
> > >      struct ds reply = DS_EMPTY_INITIALIZER;
> > >      ds_put_format(&reply, "Miniflow implementation set to %s.\n",
> > mfex_name);
> > >      const char *reply_str = ds_cstr(&reply);
> > > -    unixctl_command_reply(conn, reply_str);
> > >      VLOG_INFO("%s", reply_str);
> > > +    unixctl_command_reply(conn, reply_str);
> >
> > Is there a reason for swapping the order above?
> >
> 
> This looks more logical .
> >
> > BR
> > Ian
> > >      ds_destroy(&reply);
> > >  }
> > >
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list