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

[Van Haaren, Harry]

> -----Original Message-----
> From: Ilya Maximets <i.maximets at ovn.org>
> Sent: Thursday, June 24, 2021 12:06 PM
> To: Ilya Maximets <i.maximets at ovn.org>; Stokes, Ian <ian.stokes at intel.com>;
> Amber, Kumar <kumar.amber at intel.com>; dev at openvswitch.org; Van Haaren,
> Harry <harry.van.haaren at intel.com>
> Subject: Re: [ovs-dev] [v4 02/12] dpif-netdev: Add auto validation function for
> miniflow extract

Hi Ilya,

Thanks for reviewing.

> On 6/24/21 12:58 PM, Ilya Maximets wrote:
> > On 6/24/21 12:46 PM, Stokes, Ian wrote:
> >>> +
> >>> +            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?
> >
> > FWIW, I'd prefer having a dynamic string for this kind of complex logs
> > constructed with ds_put_hex_dump() and printed as a single log message.
> > This way it will not be intermixed with other logs.

Ah, I wasn't aware of the ds_put_hex_dump() functionality, agree this is
a good improvement to add the data into a single log entry. Done a quick
POC and this is indeed a good improvement, thanks.

<snip>

> And these logs must be rate-limited, as if this log is going to be printed,
> it will be printed for every single packet or for lots of them anyway.
> This might grow log size very fast.

If the logs here get triggered, its means that packets can have miniflows built
up that are incorrect (compared to scalar impl.). That's pretty bad, so today we
have an "ovs_assert(0)" in there to stop, but we also switch off the PMD's mfex
func pointer (setting it to NULL). This avoids spamming to the logs, logging only
the packet that caused the issue. (See the patch/code snippet I pasted below for impl.)

Once the user re-enables autovalidator via the miniflow-parser-set command,
another log-print will occur if a packet doesn't match (and again mfex opt
autovalidator will be switched off).

Hope the logic here makes sense. Will remove the ovs_assert().
I feel that "auto disable" of the MFEX opt ptr is a very nice way to
avoid spamming logs and also enable user to continue probing if required.

Regards, -Harry

> +            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;
> +            }