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

[Eelco Chaudron]

On 13 Jul 2021, at 15:15, Van Haaren, Harry wrote:

>> -----Original Message-----
>> From: Eelco Chaudron <echaudro at redhat.com>
>> Sent: Tuesday, July 13, 2021 1:04 PM
>> To: Amber, Kumar <kumar.amber at intel.com>
>> Cc: ovs-dev at openvswitch.org; fbl at sysclose.org; i.maximets at ovn.org; 
>> Van
>> Haaren, Harry <harry.van.haaren at intel.com>; 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 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