Re: [patch 22/30] net/mlx5: Replace irq_to_desc() abuse
On Thu, 2020-12-10 at 20:25 +0100, Thomas Gleixner wrote:
> No driver has any business with the internals of an interrupt
> descriptor. Storing a pointer to it just to use yet another helper at
> the
> actual usage site to retrieve the affinity mask is creative at best.
> Just
> because C does not allow encapsulation does not mean that the kernel
> has no
> limits.
>
you can't blame the developers for using stuff from include/linux/
Not all developers are the same, and sometime we don't read in between
the lines, you can't assume all driver developers to be expert on irq
APIs disciplines.
your rules must be programmatically expressed, for instance,
you can just hide struct irq_desc and irq_to_desc() in kernel/irq/ and
remove them from include/linux/ header files, if you want privacy in
your subsystem, don't put all your header files on display under
include/linux.
> Retrieve a pointer to the affinity mask itself and use that. It's
> still
> using an interface which is usually not for random drivers, but
> definitely
> less hideous than the previous hack.
>
> Signed-off-by: Thomas Gleixner
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en.h |2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c |2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c |6 +-
> 3 files changed, 3 insertions(+), 7 deletions(-)
>
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -669,7 +669,7 @@ struct mlx5e_channel {
> spinlock_t async_icosq_lock;
>
> /* data path - accessed per napi poll */
> - struct irq_desc *irq_desc;
> + const struct cpumask *aff_mask;
> struct mlx5e_ch_stats *stats;
>
> /* control */
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -1998,7 +1998,7 @@ static int mlx5e_open_channel(struct mlx
> c->num_tc = params->num_tc;
> c->xdp = !!params->xdp_prog;
> c->stats= &priv->channel_stats[ix].ch;
> - c->irq_desc = irq_to_desc(irq);
> + c->aff_mask = irq_get_affinity_mask(irq);
as long as the affinity mask pointer stays the same for the lifetime of
the irq vector.
Assuming that:
Acked-by: Saeed Mahameed
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 23/30] net/mlx5: Use effective interrupt affinity
On Thu, 2020-12-10 at 20:25 +0100, Thomas Gleixner wrote: > Using the interrupt affinity mask for checking locality is not really > working well on architectures which support effective affinity masks. > > The affinity mask is either the system wide default or set by user > space, > but the architecture can or even must reduce the mask to the > effective set, > which means that checking the affinity mask itself does not really > tell > about the actual target CPUs. > > Signed-off-by: Thomas Gleixner > Cc: Saeed Mahameed > Cc: Leon Romanovsky > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: net...@vger.kernel.org > Cc: linux-r...@vger.kernel.org > Acked-by: Saeed Mahameed ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/6] Regressions for "imply" behavior change
On Thu, 2020-04-16 at 11:52 -0300, Jason Gunthorpe wrote: > On Thu, Apr 16, 2020 at 02:38:50PM +0200, Arnd Bergmann wrote: > > On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula > > wrote: > > > On Thu, 16 Apr 2020, Arnd Bergmann wrote: > > > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed < > > > > sae...@mellanox.com> wrote: > > > > > BTW how about adding a new Kconfig option to hide the details > > > > > of > > > > > ( BAR || !BAR) ? as Jason already explained and suggested, > > > > > this will > > > > > make it easier for the users and developers to understand the > > > > > actual > > > > > meaning behind this tristate weird condition. > > > > > > > > > > e.g have a new keyword: > > > > > reach VXLAN > > > > > which will be equivalent to: > > > > > depends on VXLAN && !VXLAN > > > > > > > > I'd love to see that, but I'm not sure what keyword is best. > > > > For your > > > > suggestion of "reach", that would probably do the job, but I'm > > > > not > > > > sure if this ends up being more or less confusing than what we > > > > have > > > > today. > > > > > > Ah, perfect bikeshedding topic! > > > > > > Perhaps "uses"? If the dependency is enabled it gets used as a > > > dependency. > > > > That seems to be the best naming suggestion so far > > Uses also makes sense to me. > > > > Of course, this is all just talk until someone(tm) posts a patch > > > actually making the change. I've looked at the kconfig tool > > > sources > > > before; not going to make the same mistake again. > > > > Right. OTOH whoever implements it gets to pick the color of the > > bikeshed. ;-) > > I hope someone takes it up, especially now that imply, which > apparently used to do this, doesn't any more :) > Well, I have a patch since yesterday.. i though You and Arnd didn't like the idea, so i dropped the whole thing :), but apparently you just didn't like the name of the new option. "uses" seems like a good name .. I will post the patch. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/6] Regressions for "imply" behavior change
On Tue, 2020-04-14 at 20:47 +0200, Arnd Bergmann wrote: > On Tue, Apr 14, 2020 at 7:49 PM Saeed Mahameed > wrote: > > On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote: > > > On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe > > > wrote: > > > Correct. > > > > > > > Great ! > > > > Then bottom line we will change mlx5/Kconfig: to > > > > depends on VXLAN || !VXLAN > > Ok > BTW how about adding a new Kconfig option to hide the details of ( BAR || !BAR) ? as Jason already explained and suggested, this will make it easier for the users and developers to understand the actual meaning behind this tristate weird condition. e.g have a new keyword: reach VXLAN which will be equivalent to: depends on VXLAN && !VXLAN > > This will force MLX5_CORE to m when necessary to make vxlan > > reachable > > to mlx5_core. So no need for explicit use of IS_REACHABLE(). > > in mlx5 there are 4 of these: > > > > imply PTP_1588_CLOCK > > imply VXLAN > > imply MLXFW > > imply PCI_HYPERV_INTERFACE > > As mentioned earlier, we do need to replace the 'imply > PTP_1588_CLOCK' > with the same > > depends on PTP_1588_CLOCK || !PTP_1588_CLOCK > > So far I have not seen problems for the other two options, so I > assume they > are fine for now -- it seems to build just fine without > PCI_HYPERV_INTERFACE, > and MLXFW has no other dependencies, meaning that 'imply' is the > same as 'select' here. Using 'select MLXFW' would make it clearer > perhaps. No, I would like to avoid select and allow building mlx5 without MLXFW, MLXFW already has a stub protected with IS_REACHABLE(), this is why we don't have an issue with it. > > Arnd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/6] Regressions for "imply" behavior change
On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote: > On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe wrote: > > On Tue, Apr 14, 2020 at 04:27:41PM +0200, Arnd Bergmann wrote: > > > On Tue, Apr 14, 2020 at 3:29 PM Jason Gunthorpe > > > wrote: > > > > On Fri, Apr 10, 2020 at 07:04:27PM +, Saeed Mahameed wrote: > > > which in turn leads to mlx5_core.ko *not* containing > > > mlx5_vxlan.o, > > > and in turn causing that link error against > > > mlx5_vxlan_create/mlx5_vxlan_destroy, unless the IS_ENABLED() > > > is changed to IS_REACHABLE(). > > > > What about the reverse if mlx5_core is 'm' and VLXAN is 'y'? > > > > mlx5_core-m := mlx5_core.o > > mlx5_core-y += mlx5_vxlan.o > > > > Magically works out? > > Yes, Kbuild takes care of that case. > > > > > IIRC that isn't what the expression does, if vxlan is 'n' then > > > > n || !n == true > > > > > > It forces MLX5_CORE to 'm' or 'n' but not 'y' if VXLAN=m, > > > but allows any option if VXLAN=y > > > > And any option if VXLAN=n ? > > Correct. > Great ! Then bottom line we will change mlx5/Kconfig: to depends on VXLAN || !VXLAN This will force MLX5_CORE to m when necessary to make vxlan reachable to mlx5_core. So no need for explicit use of IS_REACHABLE(). in mlx5 there are 4 of these: imply PTP_1588_CLOCK imply VXLAN imply MLXFW imply PCI_HYPERV_INTERFACE I will make a patch. Thanks, Saeed. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/6] Regressions for "imply" behavior change
On Fri, 2020-04-10 at 14:13 -0300, Jason Gunthorpe wrote: > On Fri, Apr 10, 2020 at 02:40:42AM +0000, Saeed Mahameed wrote: > > > This assumes that the module using FOO has its own flag > > representing > > FOO which is not always the case. > > > > for example in mlx5 we use VXLAN config flag directly to compile > > VXLAN > > related files: > > > > mlx5/core/Makefile: > > > > obj-$(CONFIG_MLX5_CORE) += mlx5_core.o > > > > mlx5_core-y := mlx5_core.o > > mlx5_core-$(VXLAN) += mlx5_vxlan.o > > > > and in mlx5_main.o we do: > > Does this work if VXLAN = m ? Yes, if VXLAN IS_REACHABLE to MLX5, mlx5_vxlan.o will be compiled/linked. > > > if (IS_ENABLED(VXLAN)) > >mlx5_vxlan_init() > > > > after the change in imply semantics: > > our options are: > > > > 1) use IS_REACHABLE(VXLAN) instead of IS_ENABLED(VXLAN) > > > > 2) have MLX5_VXLAN in mlx5 Kconfig and use IS_ENABLED(MLX5_VXLAN) > > config MLX5_VXLAN > > depends on VXLAN || !VXLAN > > bool > > Does this trick work when vxlan is a bool not a tristate? > > Why not just put the VXLAN || !VXLAN directly on MLX5_CORE? > so force MLX5_CORE to n if vxlan is not reachable ? why ? mlx5_core can perfectly live without vxlan .. all we need to know is if VXLAN is supported and reachable, if so, then we want to also support vxlan in mlx5 (i.e compile mlx5_vxlan.o) and how do we compile mlx5_vxlan.o wihout a single flag can i do in Makefile : mlx5_core-$(VXLAN || !VXLAN) += mlx5_vxlan.o ?? > Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/6] Regressions for "imply" behavior change
On Thu, 2020-04-09 at 11:41 +0300, Jani Nikula wrote: > On Wed, 08 Apr 2020, Jason Gunthorpe wrote: > > On Wed, Apr 08, 2020 at 10:49:48PM +0200, Arnd Bergmann wrote: > > > On Wed, Apr 8, 2020 at 10:38 PM Nicolas Pitre > > > wrote: > > > > On Wed, 8 Apr 2020, Arnd Bergmann wrote: > > > > > I have created workarounds for the Kconfig files, which now > > > > > stop using > > > > > imply and do something else in each case. I don't know > > > > > whether there was > > > > > a bug in the kconfig changes that has led to allowing > > > > > configurations that > > > > > were not meant to be legal even with the new semantics, or if > > > > > the Kconfig > > > > > files have simply become incorrect now and the tool works as > > > > > expected. > > > > > > > > In most cases it is the code that has to be fixed. It typically > > > > does: > > > > > > > > if (IS_ENABLED(CONFIG_FOO)) > > > > foo_init(); > > > > > > > > Where it should rather do: > > > > > > > > if (IS_REACHABLE(CONFIG_FOO)) > > > > foo_init(); > > > > > > > > A couple of such patches have been produced and queued in their > > > > respective trees already. > > > > > > I try to use IS_REACHABLE() only as a last resort, as it tends to > > > confuse users when a subsystem is built as a module and already > > > loaded but something relying on that subsystem does not use it. > > > > > > In the six patches I made, I had to use IS_REACHABLE() once, > > > for the others I tended to use a Kconfig dependency like > > > > > > 'depends on FOO || FOO=n' > > This assumes that the module using FOO has its own flag representing FOO which is not always the case. for example in mlx5 we use VXLAN config flag directly to compile VXLAN related files: mlx5/core/Makefile: obj-$(CONFIG_MLX5_CORE) += mlx5_core.o mlx5_core-y := mlx5_core.o mlx5_core-$(VXLAN) += mlx5_vxlan.o and in mlx5_main.o we do: if (IS_ENABLED(VXLAN)) mlx5_vxlan_init() after the change in imply semantics: our options are: 1) use IS_REACHABLE(VXLAN) instead of IS_ENABLED(VXLAN) 2) have MLX5_VXLAN in mlx5 Kconfig and use IS_ENABLED(MLX5_VXLAN) config MLX5_VXLAN depends on VXLAN || !VXLAN bool So i understand that every one agree to use solution #2 ? > > It is unfortunate kconfig doesn't have a language feature for this > > idiom, as the above is confounding without a lot of kconfig > > knowledge > > > > > I did come up with the IS_REACHABLE() macro originally, but that > > > doesn't mean I think it's a good idea to use it liberally ;-) > > > > It would be nice to have some uniform policy here > > > > I also don't like the IS_REACHABLE solution, it makes this more > > complicated, not less.. > > Just chiming "me too" here. > > IS_REACHABLE() is not a solution, it's a hack to hide a dependency > link > problem under the carpet, in a way that is difficult for the user to > debug and figure out. > > The user thinks they've enabled a feature, but it doesn't get used > anyway, because a builtin depends on something that is a module and > therefore not reachable. Can someone please give me an example where > that kind of behaviour is desirable? > > AFAICT IS_REACHABLE() is becoming more and more common in the kernel, > but arguably it's just making more undesirable configurations > possible. Configurations that should simply be blocked by using > suitable > dependencies on the Kconfig level. > > For example, you have two graphics drivers, one builtin and another > module. Then you have backlight as a module. Using IS_REACHABLE(), > backlight would work in one driver, but not the other. I'm sure there > is > the oddball person who finds this desirable, but the overwhelming > majority would just make the deps such that either you make all of > them > modules, or also require backlight to be builtin. > the previous imply semantics handled this by forcing backlight to be built-in, which worked nicely. > > BR, > Jani. > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 0/6] Regressions for "imply" behavior change
On Wed, 2020-04-08 at 16:38 -0400, Nicolas Pitre wrote: > On Wed, 8 Apr 2020, Arnd Bergmann wrote: > > > Hi everyone, > > > > I've just restarted doing randconfig builds on top of mainline > > Linux and > > found a couple of regressions with missing dependency from the > > recent > > change in the "imply" keyword in Kconfig, presumably these two > > patches: > > > > 3a9dd3ecb207 kconfig: make 'imply' obey the direct dependency > > def2fbffe62c kconfig: allow symbols implied by y to become m > > > > I have created workarounds for the Kconfig files, which now stop > > using > > imply and do something else in each case. I don't know whether > > there was > > a bug in the kconfig changes that has led to allowing > > configurations that > > were not meant to be legal even with the new semantics, or if the > > Kconfig > > files have simply become incorrect now and the tool works as > > expected. > > In most cases it is the code that has to be fixed. It typically does: > > if (IS_ENABLED(CONFIG_FOO)) > foo_init(); > > Where it should rather do: > > if (IS_REACHABLE(CONFIG_FOO)) > foo_init(); > > A couple of such patches have been produced and queued in their > respective trees already. > > Yes i have a patch in mlx5-net branch converting IS_ENABLED to IS_REACHABLE in mlx5, i will post it today. Thanks, Saeed. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
