24/07/2020 17:43, Matan Azrad: > Hi Thomas > > From: Thomas Monjalon: > > 19/07/2020 13:41, Matan Azrad: > > > > > > From: Thomas Monjalon: > > > > 19/07/2020 12:56, Matan Azrad: > > > > > > > > > > From: Thomas Monjalon > > > > > > The detection of the CPU was done in a constructor and shared in > > > > > > a global variable. > > > > > > > > > > > > This variable may not be visible in the net PMD because it was > > > > > > not exported as part of the .map file. > > > > > > > > > > Can you explain exactly when it is not visible? > > > > > > > > I depends on linker options. > > It will be good to add here the compiling command which failed for you... > > > > > > > It is fixed by exporting a function, which is cleaner than a > > > > > > variable. > > > > > > > > > > Can you explain why? > > > > > We have classic example - rte_eth_devices. > > > > > > > > There is more control and more abstraction in functions, it can > > > > provide futre- proof abstraction. > > > > > > Also variable have more abstraction - struct. > > > In future, if it will be needed, we can change it. > > > > > > > We should not export variables at all, it is a basic rule of writing > > > > API. > > > > It is variable which is depended only in the running CPU - almost like > > > compile time condition, so it is not regular case. > > > I think it makes sense also to use a singleton variable as internal API. > > > > > > > Having a bad example in ethdev doesn't mean we should follow it. > > > > > > If ethdev rte_eth_devices is bad API, Are you going to change it? > > > > No, we avoid changing API. > > > > It is internal API, I don't understand your concern... > > > > > > > By checking the CPU only at the first call of the function, > > > > > > doing the check in a constructor becomes useless. > > > > > > > > > > Yes, but why not to do it in constructor? this variable is > > > > > initialized only once > > > > and doesn't depend in any parameter. > > > > > > > > Constructor must remain minimal. > > > > If constructor can be avoided, it must be. > > > > This is a golden rule. > > > > > > The cpu detection is a fast code. > > > > > > Using constructor here makes sense: > > > 1. we need only one initialization for all the program. > > > 2. no need to take care of multithreading on the single initialization > > > (are > > your code thread safe?). > > > > I don't see what could be the issue. > > 2 mlx5 devices running configuration from 2 different threads. > The first MR from each one of the devices can be created at the same time. > The ask for the relaxed ordering cpu can be happened at the same time.
DPDK configuration is not thread safe. Do you know any DPDK application configuring devices in 2 different threads? > > > 3. no parameters are required. > > > > > > > > > Note: the priority of the constructor was probably irrelevant. > > > > > > > > No comment about the constructor priority which was set as LOG for > > > > no good reason, proving that this code was not well reviewed? > > > > > > I guess you mean that comment is missing - you right. > > > > No I mean this constructor is declared with LOG priority, but it is not > > doing > > any log registration. > > > I hope you understand the motivation for higher priority, > The LOG is just the one above. > > > > We want to be sure that the variable is ready before any usage of it in > > > the > > drivers (even in driver contractors). > > > > It is not used by other constructors. > > And avoiding constructor dependencies is exactly why we avoid using > > constructors at all. > > Yes, It is for future, because it makes sense the cpu detection query will be > done at initialization time. > > Now, when I understand the community relevant guys don't like priorities(also > I didn't convinced on the reasons), I think you can call it from common init > function because it is the first call of mlx5 constructors. We want to avoid priorities, and more importantly, we want to avoid having too much code in constructors. > We need to fix the race issue introduced by this patch. > My favor is to call it from constructor. Initialization and configuration is supposed to be done by a single thread. There should not have any race condition.
