[ovs-dev] [v10 06/12] dpif-netdev: Add packet count and core id paramters for study

[Van Haaren, Harry]

> -----Original Message-----
> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Tuesday, July 13, 2021 3:26 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>
> Cc: Amber, Kumar <kumar.amber at intel.com>; ovs-dev at openvswitch.org;
> fbl at sysclose.org; i.maximets at ovn.org; Ferriter, Cian <cian.ferriter at intel.com>;
> Stokes, Ian <ian.stokes at intel.com>
> Subject: Re: [v10 06/12] dpif-netdev: Add packet count and core id paramters for
> study
> 
> 
> 
> On 13 Jul 2021, at 16:09, Van Haaren, Harry wrote:
> 
> > (Off Topic; Eelco, I think your email client is sending HTML replies, perhaps
> check settings to disable?)
> >
> > Eelco Wrote;
> >>  If you ask for a specific existing pmd to run a specific implementation, it
> should NOT affect any existing or newly created pmd.
> >
> > OK, thanks for explaining it clearly with examples below!
> >
> > In general the behavior below is OK, except that we set the default when a
> specific PMD is requested.
> > As a solution, we can set the default behavior only when "-pmd" is NOT
> provided.
> >
> > 1) "all pmds" (no args) mode: we set the default, update all current PMD
> threads, and will update future PMD threads via default.
> > 2) "-pmd <core>" mode: we set only for that PMD thread, and if the PMD
> thread isn't active, existing code will provide good error to user.
> >
> > Does that seem a pragmatic solution? -Harry
> 
> Yes, this is exactly what I had in mind when I added the comment.

Ah great, good stuff.

I've looked at the enabling code & patchset, the command parsing is getting complex.
I tend to agree with Eelco's review, that a single sweep of the args would be a
better and more readable implementation, however today that command is built
in multiple patchs, and each change would cause a rebase-conflict, so rework would
cost more time than we would like, particularly given the upcoming deadlines.

Here a suggestion to pragmatically move forward:
- Merge the code approximately as in this patchset, but with the 1) and 2) suggestions applied above to fixup functional correctness.
    Amber has a v11 almost ready to go that addresses the main issues.

- Commit to reworking the command in a follow-up patch next week. This refactor would use the "sweep argc" method,
   and will continue to have the same user-visible functionality/error-messages, just a cleaner implementation.
   (This avoids the rebase-heavy workflow of refactoring now, as the command is enabled over multiple commits.)

Hope that's a workable solution for all?

Regards, -Harry

<snip previous content>