Hi,
On Tue, Jun 29, 2021 at 03:20:36PM +0000, Amber, Kumar wrote:
> Hi Flavio,
>
> Replies inline.
>
> > >
> > > Guess the above needs to be atomic.
> > >
> > > Removed based on Flavio comments.
> >
> > I asked to initialize that using an API and Eelco is asking to set it
> > atomically.
> > The requests are complementary, right?
> >
>
> Yes True sorry for confusion so we have refactored the code a bit to use
> Atomic set and get along with the API
> Wherever applicable since here on any failure we would want to fall back to
> Scalar we would not need the API
> To find default implementation.
OK, no problem. Looking forward to the next version.
> > >
> > > + VLOG_ERR("failed to get miniflow extract function
> > > + implementations\n");
> > >
> > > Capital F to be in sync with your other error messages?
> > >
> > > Removed based on Flavio comments.
> >
> > Not sure if I got this. I mentioned that the '\n' is not needed at the end
> > of all
> > VLOG_* calls. Eelco is asking to start with capital 'F'. So the requests are
> > complementary, unless with the refactor the message went away.
> >
> > Just make sure to follow the logging style convention in OVS.
>
> Sorry for confusion I have fixed all the VLOGS with this convention.
great!
fbl
> >
> > fbl
> >
> >
> >
> > >
> > > + return 0;
> > > + }
> > > + ovs_assert(keys_size >= cnt);
> > >
> > > I don’t think we should assert here. Just return an error like above, so
> > > in
> > production, we get notified, and this implementation gets disabled.
> > >
> > > Actually we do else one would most likely be overwriting the assigned
> > array space for keys and will hit a Seg fault at some point.
> > >
> > > And hence we would like to know at the compile time if this is the case.
> > >
> > > + struct netdev_flow_key test_keys[NETDEV_MAX_BURST];
> > > +
> > > + /* 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].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; }
> > > +
> > > + /* Iterate through each version of miniflow implementations. */ for
> > > + (int j = MFEX_IMPL_START_IDX; j < ARRAY_SIZE(mfex_impls); j++) { if
> > > + (!mfex_impls[j].available) { 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 = mfex_impls[j].extract_func(packets, test_keys, keys_size,
> > > + in_port, pmd_handle);
> > > +
> > > + /* 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]); } 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) {
> > >
> > > Why stop now!? I think we should run all implementations, as others
> > might need fixing too!
> > >
> > > We had the same model as above by you but with so many debug
> > messages
> > > flooding made it impossible for us to
> > >
> > > Know the root cause and ovs_assert(0); have been removes so will no
> > > longer cause a crash but will simply disable the autovalidator
> > >
> > > + /* 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. */
> > > + DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> > > + packet->l2_5_ofs = good_l2_5_ofs[i]; l3_ofs = good_l3_ofs[i]; l4_ofs
> > > + packet->= good_l4_ofs[i]; 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.
> > > + */
> > > + 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
> > > + * 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);
> > >
> > > Forgot to comment on this in my previous patch:
> > >
> > > /* Function pointer prototype to be implemented in the optimized
> > > miniflow
> > >
> > > * extract code.
> > > * returns the hitmask of the processed packets on success.
> > > * returns zero on failure.
> > >
> > > */
> > > typedef uint32_t (*miniflow_extract_func)(struct dp_packet_batch
> > > *batch, struct netdev_flow_key *keys, uint32_t keys_size, odp_port_t
> > > in_port, void *pmd_handle);
> > >
> > > Maybe we could add some description of what the input parameters
> > mean, are expected? For example that key_size need to be at least the size
> > of the batch? If this is the case, do we need it, and can we just assume
> > keys
> > should be at least batch size long?
> > >
> > > Yes put a comment in there about key size in the description in v5.
> > >
> > > But as per convention whenever we pass an array we should also put the
> > > size of the array and this also complies with various
> > >
> > > Security checkers and also prevents potential exploitations.
> > >
> > > +/* 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);
> > > ds_destroy(&reply);
> > > }
> > >
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > d...@openvswitch.org<mailto:d...@openvswitch.org>
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > _______________________________________________
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > --
> > fbl
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
