> > On Tue, Jun 30, 2020 at 12:07:32PM +0000, Ananyev, Konstantin wrote: > > > > > > 26/06/2020 16:43, David Marchand: > > > > On Wed, Jun 24, 2020 at 1:59 PM Ananyev, Konstantin > > > > <konstantin.anan...@intel.com> wrote: > > > > > > > Do you mean - make this new dynamic-lcore API return an error if > > > > > > > callied > > > > > > > from secondary process? > > > > > > > > > > > > Yes, and prohibiting from attaching a secondary process if dynamic > > > > > > lcore API has been used in primary. > > > > > > I intend to squash in patch 6: > > > > > > https://github.com/david-marchand/dpdk/commit/e5861ee734bfe2e4dc23d9b919b0db2a32a58aee > > > > > > > > > > But secondary process can attach before lcore_register, so we'll have > > > > > some sort of inconsistency in behaviour. > > > > > > > > If the developer tries to use both features, he gets an ERROR log in > > > > the two init path. > > > > So whatever the order at runtime, we inform the developer (who did not > > > > read/understand the rte_thread_register() documentation) that what he > > > > is doing is unsupported. > > > > > > I agree. > > > Before this patch, pinning a thread on a random core can > > > trigger some issues. > > > After this patch, register an external thread will > > > take care of logging errors in case of inconsistencies. > > > So the user will know he is doing something not supported > > > by the app. > > > > I understand that, and return a meaningful error is definitely > > better the silent crash or memory corruption. > > The problem with that approach, as I said before, MP group > > behaviour becomes non-deterministic. > > > > > > > > It is an nice improvement. > > > > > > > > If we really want to go ahead with such workaround - > > > > > > It is not a workaround. > > > It is fixing some old issues and making clear what is really impossible. > > > > The root cause of the problem is in our MP model design decisions: > > from one side we treat lcore_id as process local data, from other side > > in some shared data-structures we use lcore_id as an index. > > I think to fix it properly we need either: > > make lcore_id data shared or stop using lcore_id as an index for shared > > data. > > So from my perspective this approach is just one of possible workarounds. > > BTW, there is nothing wrong to have a workaround for the problem > > we are not ready to fix right now. > > > > > > > probably better to introduce explicit EAL flag ( --single-process or > > > > > so). > > > > > As Thomas and Bruce suggested, if I understood them properly. > > > > > > No I was thinking to maintain the tri-state information: > > > - secondary is possible > > > - secondary is attached > > > - secondary is forbidden > > > > Ok, then I misunderstood you. > > > > > Asking the user to use an option to forbid attaching a secondary process > > > is the same as telling him it is forbidden. > > > > I don't think it is the same. > > On a live and complex system user can't always predict will the primary proc > > use dynamic lcore and if it will at what particular moment. > > Same for secondary process launching - user might never start it, > > might start it straight after the primary one, > > or might be after several hours. > > > > > The error log is enough in my opinion. > > > > I think it is better than nothing, but probably not the best one. > > Apart from possible non-consistent behaviour, it is quite restrictive: > > dynamic lcore_id wouldn't be available on any DPDK MP deployment. > > Which is a pity - I think it is a cool and useful feature. > > > > What do you guys think about different approach: > > introduce new optional EAL parameter to restrict lcore_id > > values available for the process. > > > > #let say to start primary proc that can use lcore_id=[0-99] only: > > dpdk_primary --lcore-allow=0-99 ... --file-prefix=xz1 > > > > #to start secondary one for it with allowed lcore_id=[100-109]: > > dpdk_secondary --lcore-allow=100-109 ... --file-prefix=xz1 > > --proc-type=secondary > > > > It is still a workaround, but that way we don't need to > > add any new limitations for dynamic lcores and secondary process usage. > > Now it is up to user to decide would multiple-process use the same shared > > data > > and if so - split lcore_id space properly among them > > (same as he has to do now with static lcores). > > A variant (more simple) of your approach could be to add > "--proc-type=standalone" to explicitly disable MP and enable dynamic thread > registration. >
For me it is a bit too restrictive, but yes it is a possible option, and from my perspective - a better one then disabling secondary proc support on the fly. I tried to summarize different options under discussion in another mail of this thread. Please have a look. > > > > > A EAL flag is a stable API from the start, as there is nothing > > > > describing how we can remove one. > > > > So a new EAL flag for an experimental API/feature seems contradictory. > > > > > > > > Going with a new features status API... I think it is beyond this > > > > series. > > > > > > > > Thomas seems to suggest an automatic resolution when features conflict > > > > happens.. ? > > > > > > I suggest allowing the maximum and raise an error when usage conflicts. > > > It seems this is what you did in v4. > > > > > > > I'll send the v4, let's discuss it there if you want. > > > > >
