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

Eelco Chaudron echaudro at redhat.com
Wed Jul 14 10:33:50 UTC 2021



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



More information about the dev mailing list