(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 From: Eelco Chaudron <[email protected]> Sent: Tuesday, July 13, 2021 2:41 PM To: Van Haaren, Harry <[email protected]> Cc: Amber, Kumar <[email protected]>; [email protected]; [email protected]; [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 15:15, Van Haaren, Harry wrote: -----Original Message----- From: Eelco Chaudron <[email protected]<mailto:[email protected]>> Sent: Tuesday, July 13, 2021 1:04 PM To: Amber, Kumar <[email protected]<mailto:[email protected]>> Cc: [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]>; Van Haaren, Harry <[email protected]<mailto:[email protected]>>; Ferriter, Cian <[email protected]<mailto:[email protected]>>; Stokes, Ian <[email protected]<mailto:[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
