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 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.
> > > > 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.
> 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.
> 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.
> > > > At the same time, the comments are reworded or dropped if useless.
> > > >
> > > > Fixes: 4c204fe5e5d2 ("common/mlx5: disable relaxed ordering in
> > > > unsuitable
> > > > CPUs")
> > > > Cc: shi...@mellanox.com
> > > > Cc: sta...@dpdk.org
> > > >
> > > > Signed-off-by: Thomas Monjalon <tho...@monjalon.net>
