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

[Amber, Kumar]

Hi Eelco,

The requested changes for the rework of MFEX-set commands are now available in v12 😊

Br Amber

> -----Original Message-----
> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Wednesday, July 14, 2021 4:04 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 18:11, Van Haaren, Harry wrote:
> 
> >> -----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?
> 
> Just reviewed the v11, and I agree it looks even messier. I think it needs a proper
> rewrite and contradicting to your statement above, all changes are
> concentrated in patch 6, so I think it needs to be done in the v12 as we are
> almost there.
> 
> Cheers,
> 
> Eelco