| Message ID | 20221002151702.3932770-1-yury.norov@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: drop netif_attrmask_next*() | expand |
On Sun, 2 Oct 2022 08:16:58 -0700 Yury Norov wrote: > netif_attrmask_next_and() generates warnings if CONFIG_DEBUG_PER_CPU_MAPS > is enabled. Could you describe the nature of the warning? Is it a false positive or a legit warning? If the former perhaps we should defer until after the next merge window. > It is used in a single place. netif_attrmask_next() is not > used at all. With some rework of __netif_set_xps_queue(), we can drop > both functions, switch the code to well-tested bitmap API and fix the > warning.
On Mon, Oct 03, 2022 at 09:50:48AM -0700, Jakub Kicinski wrote: > On Sun, 2 Oct 2022 08:16:58 -0700 Yury Norov wrote: > > netif_attrmask_next_and() generates warnings if CONFIG_DEBUG_PER_CPU_MAPS > > is enabled. > > Could you describe the nature of the warning? Is it a false positive > or a legit warning? > > If the former perhaps we should defer until after the next merge window. The problem is that netif_attrmask_next_and() is called with n == nr_cpu_ids-1, which triggers cpu_max_bits_warn() after this: https://lore.kernel.org/netdev/20220926103437.322f3c6c@kernel.org/ Underlying bitmap layer handles this correctly, so this wouldn't make problems for people. But this is not a false-positive. Thanks, Yury
On Mon, 3 Oct 2022 11:11:05 -0700 Yury Norov wrote: > On Mon, Oct 03, 2022 at 09:50:48AM -0700, Jakub Kicinski wrote: > > On Sun, 2 Oct 2022 08:16:58 -0700 Yury Norov wrote: > > > netif_attrmask_next_and() generates warnings if CONFIG_DEBUG_PER_CPU_MAPS > > > is enabled. > > > > Could you describe the nature of the warning? Is it a false positive > > or a legit warning? > > > > If the former perhaps we should defer until after the next merge window. > > The problem is that netif_attrmask_next_and() is called with > n == nr_cpu_ids-1, which triggers cpu_max_bits_warn() after this: > > https://lore.kernel.org/netdev/20220926103437.322f3c6c@kernel.org/ I see. Is that patch merged and on it's way? Perhaps we can just revert it and try again after the merge window? > Underlying bitmap layer handles this correctly, so this wouldn't make > problems for people. But this is not a false-positive.
On Mon, Oct 03, 2022 at 04:25:56PM -0700, Jakub Kicinski wrote: > On Mon, 3 Oct 2022 11:11:05 -0700 Yury Norov wrote: > > On Mon, Oct 03, 2022 at 09:50:48AM -0700, Jakub Kicinski wrote: > > > On Sun, 2 Oct 2022 08:16:58 -0700 Yury Norov wrote: > > > > netif_attrmask_next_and() generates warnings if CONFIG_DEBUG_PER_CPU_MAPS > > > > is enabled. > > > > > > Could you describe the nature of the warning? Is it a false positive > > > or a legit warning? > > > > > > If the former perhaps we should defer until after the next merge window. > > > > The problem is that netif_attrmask_next_and() is called with > > n == nr_cpu_ids-1, which triggers cpu_max_bits_warn() after this: > > > > https://lore.kernel.org/netdev/20220926103437.322f3c6c@kernel.org/ > > I see. Is that patch merged and on it's way? This patch is already in pull request. > Perhaps we can just revert it and try again after the merge window? I don't understand this. To me it looks fairly normal - the check has been fixed and merged (likely) in -rc1. After that we have 2 month to spot, fix and test all issues discovered with correct cpumask_check(). I'm not insisting in moving this series in -rc1. Let's give it review and careful testing, and merge in -rc2, 3 or whatever is appropriate. Regarding cpumask_check() patch - I'd like to have it in -rc1 because it will give people enough time to test their code... Would it work? Thanks, Yury
On Mon, 3 Oct 2022 17:07:31 -0700 Yury Norov wrote: > > I see. Is that patch merged and on it's way? > > This patch is already in pull request. > > > Perhaps we can just revert it and try again after the merge window? > > I don't understand this. To me it looks fairly normal - the check has > been fixed and merged (likely) in -rc1. After that we have 2 month to > spot, fix and test all issues discovered with correct cpumask_check(). > > I'm not insisting in moving this series in -rc1. Let's give it review > and careful testing, and merge in -rc2, 3 or whatever is appropriate. > > Regarding cpumask_check() patch - I'd like to have it in -rc1 because > it will give people enough time to test their code... AFAIU you can keep the cpumask_check() patch, we just need to revert the netdev patch from your earlier series? If so I strongly prefer that we revert the broken cleanup rather than try to pile on more re-factoring. The trees are not going anywhere, we can queue the patches for 6.2.
On Mon, Oct 3, 2022 at 5:29 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 3 Oct 2022 17:07:31 -0700 Yury Norov wrote: > > > I see. Is that patch merged and on it's way? > > > > This patch is already in pull request. > > > > > Perhaps we can just revert it and try again after the merge window? > > > > I don't understand this. To me it looks fairly normal - the check has > > been fixed and merged (likely) in -rc1. After that we have 2 month to > > spot, fix and test all issues discovered with correct cpumask_check(). > > > > I'm not insisting in moving this series in -rc1. Let's give it review > > and careful testing, and merge in -rc2, 3 or whatever is appropriate. > > > > Regarding cpumask_check() patch - I'd like to have it in -rc1 because > > it will give people enough time to test their code... > > AFAIU you can keep the cpumask_check() patch, we just need to revert > the netdev patch from your earlier series? Yeah, I meant the "net: fix cpu_max_bits_warn() usage in netif_attrmask_next{,_and}". > If so I strongly prefer that we revert the broken cleanup rather than > try to pile on more re-factoring. What do you mean by broken cleanup? Netdev patch is acked by you, and this series didn't receive negative feedback so far. > The trees are not going anywhere, we can queue the patches for 6.2. Sure, 6.2 is OK as well, but I think any 6.1-rc would be more appropriate.