On 13 Jul 2021, at 15:15, Van Haaren, Harry wrote:
-----Original Message-----
From: Eelco Chaudron <[email protected]>
Sent: Tuesday, July 13, 2021 1:04 PM
To: Amber, Kumar <[email protected]>
Cc: [email protected]; [email protected]; [email protected];
Van
Haaren, Harry <[email protected]>; Ferriter, Cian
<[email protected]>; Stokes, Ian <[email protected]>
Subject: Re: [v10 06/12] dpif-netdev: Add packet count and core id
paramters for
study
On 13 Jul 2021, at 7:32, Kumar Amber wrote:
<snip lots of patch>
+ }
int err = dp_mfex_impl_set_default_by_name(mfex_name);
The previous comment was not act upon/replied to:
“””
> Should we call dp_mfex_impl_set_default_by_name() if we only set
a single
> PMD? I do not think, as we change the default for all threads not
just this
specific one.
“””
There's a bigger picture to see here;
Before PMDs threads start polling RXQs, there is nothing.
If we run the MFEX parser set command at this point, it has no effect,
as there is no PMD thread structure to update the function pointer on.
The set_default() here causes that implementation to be saved for
later,
as a result when the PMD-structure is created, it picks up the users
desired
MFEX pointer, and behaves as expected.
Yes it sets the default here every time, even with a single PMD...
actually, even
when there are zero PMDs! But if we don't, then we cause headaches for
the user
in that certain commands require specific states of PMD-threads
launching & polling.
This method is consistent, and always gives the user their desired
behaviour.
I disagree here, as this is all but consistent. Take the following
example:
- You create 5 PMDs (they use the current default, in our case scalar,
as we did not change anything).
- Now you want pmd 1 to use AVX algorithm, using the -pmd option
- Now you add 4 new PMD
Here you would assume that pmd 1 runs the AVX, 2-10 will run scaler.
But because we have updated the default, even when updating a single
PMD, they are not, 6-10 run AVX.
So for consistency, if you ask all PMDs (not supplying the -pmd option)
to run a specific implementation, they should run it (newly created or
existing). If you ask for a specific existing pmd to run a specific
implementation, it should NOT affect any existing or newly created pmd.
+ /* If PMD thread was specified, but it wasn't found, return
error. */
+ if (pmd_thread_specified && !pms_thread_update_ok) {
As mentioned the previous review, this error needs to move up before
we
change the default value, i.e. before
dp_mfex_impl_set_default_by_name()
See above comment for explanation as to why that is first.
It comes down to the fact that we want to set the default, even when
there
are zero PMD threads to update.
If the user passes a specific pmd-thread, we must iterate them to
identify if
there's an error, hence the user-facing error-logging is here (and not
above).
See above :)
Hope that clarifies the current implementation/design. Code level
changes
that were <snipped> from this reply are being addressed by Amber.
Regards, -Harry
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
