| Message ID | 20220123183925.1052919-11-yury.norov@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [01/54] net/dsa: don't use bitmap_weight() in b53_arl_read() | expand |
On Sun, Jan 23, 2022 at 10:38:41AM -0800, Yury Norov wrote: > qlogic/qed code calls bitmap_weight() to check if any bit of a given > bitmap is set. It's better to use bitmap_empty() in that case because > bitmap_empty() stops traversing the bitmap as soon as it finds first > set bit, while bitmap_weight() counts all bits unconditionally. > - if (bitmap_weight((unsigned long *)&pmap[item], 64 * 8)) > + if (!bitmap_empty((unsigned long *)&pmap[item], 64 * 8)) > - (bitmap_weight((unsigned long *)&pmap[item], > + (!bitmap_empty((unsigned long *)&pmap[item], Side note, these castings reminds me previous discussion and I'm wondering if you have this kind of potentially problematic places in your TODO as subject to fix.
On Mon, Jan 24, 2022 at 4:29 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sun, Jan 23, 2022 at 10:38:41AM -0800, Yury Norov wrote: > > qlogic/qed code calls bitmap_weight() to check if any bit of a given > > bitmap is set. It's better to use bitmap_empty() in that case because > > bitmap_empty() stops traversing the bitmap as soon as it finds first > > set bit, while bitmap_weight() counts all bits unconditionally. > > > - if (bitmap_weight((unsigned long *)&pmap[item], 64 * 8)) > > + if (!bitmap_empty((unsigned long *)&pmap[item], 64 * 8)) > > > - (bitmap_weight((unsigned long *)&pmap[item], > > + (!bitmap_empty((unsigned long *)&pmap[item], > > Side note, these castings reminds me previous discussion and I'm wondering > if you have this kind of potentially problematic places in your TODO as > subject to fix. In the discussion you mentioned above, the u32* was cast to u64*, which is wrong. The code here is safe because in the worst case, it casts u64* to u32*. This would be OK wrt -Werror=array-bounds. The function itself looks like doing this unsigned long <-> u64 conversions just for printing purpose. I'm not a qlogic expert, so let's wait what people say? The printing part may be refactored although to use %pb" format, similarly to the snippet below (not tested). Thanks, Yury diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c index 23b668de4640..72505517ced1 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c @@ -336,17 +336,8 @@ void qed_rdma_bmap_free(struct qed_hwfn *p_hwfn, /* print aligned non-zero lines, if any */ for (item = 0, line = 0; line < last_line; line++, item += 8) - if (bitmap_weight((unsigned long *)&pmap[item], 64 * 8)) - DP_NOTICE(p_hwfn, - "line 0x%04x: 0x%016llx 0x%016llx 0x%016llx 0x%016llx 0x%016llx 0x%016llx 0x%016llx 0x%016llx\n", - line, - pmap[item], - pmap[item + 1], - pmap[item + 2], - pmap[item + 3], - pmap[item + 4], - pmap[item + 5], - pmap[item + 6], pmap[item + 7]); + if (bitmap_weight(bmap->bitmap, 64 * 8)) + DP_NOTICE(p_hwfn, "line 0x%04x: %512pb\n", line, bmap->bitmap); /* print last unaligned non-zero line, if any */ if ((bmap->max_count % (64 * 8)) &&
From: Yury Norov > Sent: 25 January 2022 21:10 > On Mon, Jan 24, 2022 at 4:29 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Sun, Jan 23, 2022 at 10:38:41AM -0800, Yury Norov wrote: > > > qlogic/qed code calls bitmap_weight() to check if any bit of a given > > > bitmap is set. It's better to use bitmap_empty() in that case because > > > bitmap_empty() stops traversing the bitmap as soon as it finds first > > > set bit, while bitmap_weight() counts all bits unconditionally. > > > > > - if (bitmap_weight((unsigned long *)&pmap[item], 64 * 8)) > > > + if (!bitmap_empty((unsigned long *)&pmap[item], 64 * 8)) > > > > > - (bitmap_weight((unsigned long *)&pmap[item], > > > + (!bitmap_empty((unsigned long *)&pmap[item], > > > > Side note, these castings reminds me previous discussion and I'm wondering > > if you have this kind of potentially problematic places in your TODO as > > subject to fix. > > In the discussion you mentioned above, the u32* was cast to u64*, > which is wrong. The code > here is safe because in the worst case, it casts u64* to u32*. This > would be OK wrt > -Werror=array-bounds. > > The function itself looks like doing this unsigned long <-> u64 > conversions just for printing > purpose. I'm not a qlogic expert, so let's wait what people say? It'll be wrong on BE systems. You just can't cast the argument it has to be long[]. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Jan 25, 2022 at 2:15 PM David Laight <David.Laight@aculab.com> wrote: > > From: Yury Norov > > Sent: 25 January 2022 21:10 > > On Mon, Jan 24, 2022 at 4:29 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Sun, Jan 23, 2022 at 10:38:41AM -0800, Yury Norov wrote: > > > > qlogic/qed code calls bitmap_weight() to check if any bit of a given > > > > bitmap is set. It's better to use bitmap_empty() in that case because > > > > bitmap_empty() stops traversing the bitmap as soon as it finds first > > > > set bit, while bitmap_weight() counts all bits unconditionally. > > > > > > > - if (bitmap_weight((unsigned long *)&pmap[item], 64 * 8)) > > > > + if (!bitmap_empty((unsigned long *)&pmap[item], 64 * 8)) > > > > > > > - (bitmap_weight((unsigned long *)&pmap[item], > > > > + (!bitmap_empty((unsigned long *)&pmap[item], > > > > > > Side note, these castings reminds me previous discussion and I'm wondering > > > if you have this kind of potentially problematic places in your TODO as > > > subject to fix. > > > > In the discussion you mentioned above, the u32* was cast to u64*, > > which is wrong. The code > > here is safe because in the worst case, it casts u64* to u32*. This > > would be OK wrt > > -Werror=array-bounds. > > > > The function itself looks like doing this unsigned long <-> u64 > > conversions just for printing > > purpose. I'm not a qlogic expert, so let's wait what people say? > > It'll be wrong on BE systems. The bitmap_weigh() result will be correct. As you can see, the address is 64-bit aligned anyways. The array boundary violation will never happen as well. DP_NOTICE() may be wrong, or may not. It depends on how important the absolute position of the bit in the printed bitmap is. Nevertheless, printk("%pb") is better and should be used. This whole concern may be simply irrelevant if QED is not supported on 32-bit BE machines. From what I can see, at least Infiniband requires 64BIT. Thanks, Yury > You just can't cast the argument it has to be long[]. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c index 23b668de4640..b6e2e17bac04 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c @@ -336,7 +336,7 @@ void qed_rdma_bmap_free(struct qed_hwfn *p_hwfn, /* print aligned non-zero lines, if any */ for (item = 0, line = 0; line < last_line; line++, item += 8) - if (bitmap_weight((unsigned long *)&pmap[item], 64 * 8)) + if (!bitmap_empty((unsigned long *)&pmap[item], 64 * 8)) DP_NOTICE(p_hwfn, "line 0x%04x: 0x%016llx 0x%016llx 0x%016llx 0x%016llx 0x%016llx 0x%016llx 0x%016llx 0x%016llx\n", line, @@ -350,7 +350,7 @@ void qed_rdma_bmap_free(struct qed_hwfn *p_hwfn, /* print last unaligned non-zero line, if any */ if ((bmap->max_count % (64 * 8)) && - (bitmap_weight((unsigned long *)&pmap[item], + (!bitmap_empty((unsigned long *)&pmap[item], bmap->max_count - item * 64))) { offset = sprintf(str_last_line, "line 0x%04x: ", line); for (; item < last_item; item++) diff --git a/drivers/net/ethernet/qlogic/qed/qed_roce.c b/drivers/net/ethernet/qlogic/qed/qed_roce.c index 071b4aeaddf2..134ecfca96a3 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_roce.c +++ b/drivers/net/ethernet/qlogic/qed/qed_roce.c @@ -76,7 +76,7 @@ void qed_roce_stop(struct qed_hwfn *p_hwfn) * We delay for a short while if an async destroy QP is still expected. * Beyond the added delay we clear the bitmap anyway. */ - while (bitmap_weight(rcid_map->bitmap, rcid_map->max_count)) { + while (!bitmap_empty(rcid_map->bitmap, rcid_map->max_count)) { /* If the HW device is during recovery, all resources are * immediately reset without receiving a per-cid indication * from HW. In this case we don't expect the cid bitmap to be
qlogic/qed code calls bitmap_weight() to check if any bit of a given bitmap is set. It's better to use bitmap_empty() in that case because bitmap_empty() stops traversing the bitmap as soon as it finds first set bit, while bitmap_weight() counts all bits unconditionally. Signed-off-by: Yury Norov <yury.norov@gmail.com> --- drivers/net/ethernet/qlogic/qed/qed_rdma.c | 4 ++-- drivers/net/ethernet/qlogic/qed/qed_roce.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)