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

[Amber, Kumar]

Hi Eelco,

Again, looks like email format is broken . I will snip the mail
Pls find the replies inline.

<Snip>

+ uint32_t batch_failed = 0;
+ /* Iterate through each version of miniflow implementations. */
+ for (int j = MFEX_IMPL_MAX; j < MFEX_IMPL_MAX; j++) {

This is really confusing ;) Maybe we could define an enum for the first implementation, as you had in your previous patch, MFEX_IMPL_START_IDX?
So that even if someone changes the enum order it’s clear? Maybe with a separate #define or doing something like:

enum dpif_miniflow_extract_impl_idx {
MFEX_IMPL_AUTOVALIDATOR,
MFEX_IMPL_SCALAR,
MFEX_IMPL_START_IDX,
MFEX_IMPL_MAX = MFEX_IMPL_START_IDX
};

I have decided to create a new define MFEX_IMPL_START_IDX  that is cleaner and nicer to implement. Applied to v7.

+ if ((j < MFEX_IMPL_MAX) || (!mfex_impls[j].available)) {

Why the (j < MFEX_IMPL_MAX) so the below code is always skipped? So the validator is not executed ever?

This was introduced to avoid seg faults if someone tried to use this without the AVX flag enabled.

+ continue;
+ }
+
+ /* Reset keys and offsets before each implementation. */
+
+ uint32_t failed = 0;
+
+ struct ds log_msg = DS_EMPTY_INITIALIZER;
+ ds_put_format(&log_msg, "mfex autovalidator pkt %d\n", i);

Capital MFEX?

Applied to v7.

+ /* 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])) {

In this code we assume FLOWMAP_UNITS == 2, can we add a static assert to make sure this is not changing.
There is also flowmap_equal() which might be better, but the assert might still be needed for the below logging.

Introduced  BUILD_ASSERT_DECL(FLOWMAP_UNITS == 2);

+ ds_put_format(&log_msg, "Good 0x%llx 0x%llx\tTest 0x%llx"

Think here we need some more details like for the ones below:

                              dd_put_format(&log_msg, "Autovalidation map failed\n”

                                                       “Good 0x%llx 0x%llx\tTest 0x%llx”



Applied to v7

+ " 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);
+ ds_put_format(&log_msg, "Autovalidation blocks failed for %s"
+ "pkt %d\nGood hex:\n", mfex_impls[j].name, i);

To save on log file content, we can remove the packet and name, as it’s already logged on error in the if(failed) condition below.
So it will become:

ds_put_format(&log_msg, "Autovalidation blocks failed\n”
"nGood hex:\n", mfex_impls[j].name, i);

Applied to v7.

+ ds_put_hex_dump(&log_msg, &keys[i].buf, block_cnt * 8, 0,
+ false);
+ ds_put_format(&log_msg, "Test hex:\n");
+ ds_put_hex_dump(&log_msg, &test_keys[i].buf, block_cnt * 8, 0,
+ false);
+ failed = 1;
+ }
+
+ packet = packets->packets[i];
+ if ((packet->l2_pad_size != good_l2_pad_size[i]) ||
+ (packet->l2_5_ofs != good_l2_5_ofs[i]) ||
+ (packet->l3_ofs != good_l3_ofs[i]) ||
+ (packet->l4_ofs != good_l4_ofs[i])) {
+ ds_put_format(&log_msg, "Autovalidation packet offsets failed"
+ " for %s pkt %d\n", mfex_impls[j].name, i);

To save on log file content, we can remove the packet and name, as it’s already logged on error in the if(failed) condition below.

Applied to v7.

+ ds_put_format(&log_msg, "Good offsets: l2_pad_size %u,"
+ " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",

<Snip>