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. > > From: Eelco Chaudron <echau...@redhat.com> > Sent: Tuesday, July 13, 2021 2:41 PM > To: Van Haaren, Harry <harry.van.haa...@intel.com> > Cc: Amber, Kumar <kumar.am...@intel.com>; ovs-dev@openvswitch.org; > f...@sysclose.org; i.maxim...@ovn.org; Ferriter, Cian > <cian.ferri...@intel.com>; Stokes, Ian <ian.sto...@intel.com> > Subject: Re: [v10 06/12] dpif-netdev: Add packet count and core id paramters > for study > > > On 13 Jul 2021, at 15:15, Van Haaren, Harry wrote: > > -----Original Message----- > From: Eelco Chaudron <echau...@redhat.com<mailto:echau...@redhat.com>> > Sent: Tuesday, July 13, 2021 1:04 PM > To: Amber, Kumar <kumar.am...@intel.com<mailto:kumar.am...@intel.com>> > Cc: ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org> ; > f...@sysclose.org<mailto:f...@sysclose.org> ; > i.maxim...@ovn.org<mailto:i.maxim...@ovn.org> ; Van > Haaren, Harry <harry.van.haa...@intel.com<mailto:harry.van.haa...@intel.com>> > ; Ferriter, Cian > <cian.ferri...@intel.com<mailto:cian.ferri...@intel.com>> ; Stokes, Ian > <ian.sto...@intel.com<mailto:ian.sto...@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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
