| Message ID | 20221014030459.3272206-2-guoren@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Fixup cpu_mask usage | expand |
On Thu, 13 Oct 2022 23:04:58 -0400 guoren@kernel.org wrote: > - for (j = -1; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids), > - j < nr_ids;) { > + for (j = -1; j < nr_ids; > + j = netif_attrmask_next_and(j, online_mask, mask, nr_ids)) { This does not look equivalent, have you tested it? nr_ids is unsigned, doesn't it mean we'll never enter the loop? Can we instead revert 854701ba4c and take the larger rework Yury has posted a week ago into net-next?
On Thu, 13 Oct 2022 20:35:44 -0700 Jakub Kicinski wrote: > Can we instead revert 854701ba4c and take the larger rework Yury > has posted a week ago into net-next? Oh, it was reposted today: https://lore.kernel.org/all/20221013234349.1165689-2-yury.norov@gmail.com/ But we need a revert of 854701ba4c as well to cover the issue back up for 6.1, AFAIU.
On Thu, Oct 13, 2022 at 08:39:11PM -0700, Jakub Kicinski wrote: > On Thu, 13 Oct 2022 20:35:44 -0700 Jakub Kicinski wrote: > > Can we instead revert 854701ba4c and take the larger rework Yury > > has posted a week ago into net-next? > > Oh, it was reposted today: > > https://lore.kernel.org/all/20221013234349.1165689-2-yury.norov@gmail.com/ > > But we need a revert of 854701ba4c as well to cover the issue back up > for 6.1, AFAIU. The patch 854701ba4c is technically correct. I fixed most of warnings in advance, but nobody can foresee everything, right? I expected some noise, and now we have just a few things to fix. This is what for -rc releases exist, didn't they? I suggest to keep the patch, because this is the only way to make cpumask_check()-related issues visible to people. If things will go as they go now, I expect that -rc3 will be clean from cpumask_check() warnings. Thanks, Yury
On Fri, Oct 14, 2022 at 11:35 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 13 Oct 2022 23:04:58 -0400 guoren@kernel.org wrote: > > - for (j = -1; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids), > > - j < nr_ids;) { > > + for (j = -1; j < nr_ids; > > + j = netif_attrmask_next_and(j, online_mask, mask, nr_ids)) { > > This does not look equivalent, have you tested it? > > nr_ids is unsigned, doesn't it mean we'll never enter the loop? Yes, you are right. Any unsigned int would break the result. (gdb) p (int)-1 < (int)2 $1 = 1 (gdb) p (int)-1 < (unsigned int)2 $2 = 0 (gdb) p (unsigned int)-1 < (int)2 $4 = 0 So it should be: - for (j = -1; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids), - j < nr_ids;) { + for (j = -1; j < (int)nr_ids; + j = netif_attrmask_next_and(j, online_mask, mask, nr_ids)) { Right? Of cause, nr_ids couldn't be 0xffffffff (-1). > > Can we instead revert 854701ba4c and take the larger rework Yury > has posted a week ago into net-next?
On Fri, Oct 14, 2022 at 12:42 PM Yury Norov <yury.norov@gmail.com> wrote: > > On Thu, Oct 13, 2022 at 08:39:11PM -0700, Jakub Kicinski wrote: > > On Thu, 13 Oct 2022 20:35:44 -0700 Jakub Kicinski wrote: > > > Can we instead revert 854701ba4c and take the larger rework Yury > > > has posted a week ago into net-next? > > > > Oh, it was reposted today: > > > > https://lore.kernel.org/all/20221013234349.1165689-2-yury.norov@gmail.com/ > > > > But we need a revert of 854701ba4c as well to cover the issue back up > > for 6.1, AFAIU. > > The patch 854701ba4c is technically correct. I fixed most of warnings in > advance, but nobody can foresee everything, right? I expected some noise, > and now we have just a few things to fix. This is what for -rc releases > exist, didn't they? Your job is great, I just want to help with some fixes. Fixes them in -rc would be a good point. > > I suggest to keep the patch, because this is the only way to make > cpumask_check()-related issues visible to people. If things will go as > they go now, I expect that -rc3 will be clean from cpumask_check() > warnings. > > Thanks, > Yury
On Thu, Oct 13, 2022 at 11:04:58PM -0400, guoren@kernel.org wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > Don't pass nr_bits as arg1, cpu_max_bits_warn would cause warning > now 854701ba4c39 ("net: fix cpu_max_bits_warn() usage in > netif_attrmask_next{,_and}"). > > ------------[ cut here ]------------ > WARNING: CPU: 2 PID: 1 at include/linux/cpumask.h:110 __netif_set_xps_queue+0x14e/0x770 > Modules linked in: Submitting Patches documentation suggests to cut this to only what makes sense for the report.
On Fri, Oct 14, 2022 at 6:01 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Oct 13, 2022 at 11:04:58PM -0400, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > Don't pass nr_bits as arg1, cpu_max_bits_warn would cause warning > > now 854701ba4c39 ("net: fix cpu_max_bits_warn() usage in > > netif_attrmask_next{,_and}"). > > > > ------------[ cut here ]------------ > > WARNING: CPU: 2 PID: 1 at include/linux/cpumask.h:110 __netif_set_xps_queue+0x14e/0x770 > > Modules linked in: > > Submitting Patches documentation suggests to cut this to only what makes sense > for the report. Right, thx for mentioning. > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, 14 Oct 2022 14:38:56 +0800 Guo Ren wrote: > > This does not look equivalent, have you tested it? > > > > nr_ids is unsigned, doesn't it mean we'll never enter the loop? > > Yes, you are right. Any unsigned int would break the result. > (gdb) p (int)-1 < (int)2 > $1 = 1 > (gdb) p (int)-1 < (unsigned int)2 > $2 = 0 > (gdb) p (unsigned int)-1 < (int)2 > $4 = 0 > > So it should be: > - for (j = -1; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids), > - j < nr_ids;) { > + for (j = -1; j < (int)nr_ids; > + j = netif_attrmask_next_and(j, online_mask, mask, nr_ids)) { > > Right? Of cause, nr_ids couldn't be 0xffffffff (-1). No. You can't enter the loop with -1 as the iterator either. Let's move on.
On Thu, 13 Oct 2022 21:42:41 -0700 Yury Norov wrote: > > Oh, it was reposted today: > > > > https://lore.kernel.org/all/20221013234349.1165689-2-yury.norov@gmail.com/ > > > > But we need a revert of 854701ba4c as well to cover the issue back up > > for 6.1, AFAIU. > > The patch 854701ba4c is technically correct. I fixed most of warnings in > advance, but nobody can foresee everything, right? I expected some noise, > and now we have just a few things to fix. I got 6 warnings booting my machine after pulling back from Linus (which included your patches in net for the first time). And that's not including the XPS and the virtio warning. > This is what for -rc releases exist, didn't they? > > I suggest to keep the patch, because this is the only way to make > cpumask_check()-related issues visible to people. If things will go as > they go now, I expect that -rc3 will be clean from cpumask_check() > warnings. This sounds too close to saying that "it's okay for -rc1 to be broken". Why were your changes not in linux-next for a month before the merge window? :( We will not be merging a refactoring series into net to silence an arguably over-eager warning. We need a minimal fix, Guo Ren's patches seem to miss the mark so I reckon the best use of everyone's time is to just drop the exposing patch and retry in -next
On Fri, Oct 14, 2022 at 9:03 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 13 Oct 2022 21:42:41 -0700 Yury Norov wrote: > > > Oh, it was reposted today: > > > > > > https://lore.kernel.org/all/20221013234349.1165689-2-yury.norov@gmail.com/ > > > > > > But we need a revert of 854701ba4c as well to cover the issue back up > > > for 6.1, AFAIU. > > > > The patch 854701ba4c is technically correct. I fixed most of warnings in > > advance, but nobody can foresee everything, right? I expected some noise, > > and now we have just a few things to fix. > > I got 6 warnings booting my machine after pulling back from Linus > (which included your patches in net for the first time). > And that's not including the XPS and the virtio warning. > > > This is what for -rc releases exist, didn't they? > > > > I suggest to keep the patch, because this is the only way to make > > cpumask_check()-related issues visible to people. If things will go as > > they go now, I expect that -rc3 will be clean from cpumask_check() > > warnings. > > This sounds too close to saying that "it's okay for -rc1 to be broken". > Why were your changes not in linux-next for a month before the merge > window? :( They spent about a month in -next. Nobody cared. > We will not be merging a refactoring series into net to silence an > arguably over-eager warning. We need a minimal fix, Guo Ren's patches > seem to miss the mark so I reckon the best use of everyone's time is > to just drop the exposing patch and retry in -next
On Fri, 14 Oct 2022 09:16:01 -0700 Yury Norov wrote: > > We will not be merging a refactoring series into net to silence an > > arguably over-eager warning. We need a minimal fix, Guo Ren's patches > > seem to miss the mark so I reckon the best use of everyone's time is > > to just drop the exposing patch and retry in -next
On Fri, Oct 14, 2022 at 11:52 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 14 Oct 2022 14:38:56 +0800 Guo Ren wrote: > > > This does not look equivalent, have you tested it? > > > > > > nr_ids is unsigned, doesn't it mean we'll never enter the loop? > > > > Yes, you are right. Any unsigned int would break the result. > > (gdb) p (int)-1 < (int)2 > > $1 = 1 > > (gdb) p (int)-1 < (unsigned int)2 > > $2 = 0 > > (gdb) p (unsigned int)-1 < (int)2 > > $4 = 0 > > > > So it should be: > > - for (j = -1; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids), > > - j < nr_ids;) { > > + for (j = -1; j < (int)nr_ids; > > + j = netif_attrmask_next_and(j, online_mask, mask, nr_ids)) { > > > > Right? Of cause, nr_ids couldn't be 0xffffffff (-1). > > No. You can't enter the loop with -1 as the iterator either. > Let's move on. Oops, how about the below: for (j = netif_attrmask_next_and(-1, online_mask, mask, nr_ids); j < (int)nr_ids; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids)) {
On Sat, Oct 15, 2022 at 12:03 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 13 Oct 2022 21:42:41 -0700 Yury Norov wrote: > > > Oh, it was reposted today: > > > > > > https://lore.kernel.org/all/20221013234349.1165689-2-yury.norov@gmail.com/ > > > > > > But we need a revert of 854701ba4c as well to cover the issue back up > > > for 6.1, AFAIU. > > > > The patch 854701ba4c is technically correct. I fixed most of warnings in > > advance, but nobody can foresee everything, right? I expected some noise, > > and now we have just a few things to fix. > > I got 6 warnings booting my machine after pulling back from Linus > (which included your patches in net for the first time). > And that's not including the XPS and the virtio warning. Oh, that's a wide effect than we thought. > > > This is what for -rc releases exist, didn't they? > > > > I suggest to keep the patch, because this is the only way to make > > cpumask_check()-related issues visible to people. If things will go as > > they go now, I expect that -rc3 will be clean from cpumask_check() > > warnings. > > This sounds too close to saying that "it's okay for -rc1 to be broken". > Why were your changes not in linux-next for a month before the merge > window? :( > > We will not be merging a refactoring series into net to silence an > arguably over-eager warning. We need a minimal fix, Guo Ren's patches > seem to miss the mark so I reckon the best use of everyone's time is > to just drop the exposing patch and retry in -next
diff --git a/net/core/dev.c b/net/core/dev.c index fa53830d0683..9ec8b10ae329 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2589,8 +2589,8 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask, copy = true; /* allocate memory for queue storage */ - for (j = -1; j = netif_attrmask_next_and(j, online_mask, mask, nr_ids), - j < nr_ids;) { + for (j = -1; j < nr_ids; + j = netif_attrmask_next_and(j, online_mask, mask, nr_ids)) { if (!new_dev_maps) { new_dev_maps = kzalloc(maps_sz, GFP_KERNEL); if (!new_dev_maps) {