Re: [RFC PATCH 05/49] qed: rework qed_rdma_bmap_free()

[Andy Shevchenko]

On Thu, Feb 10, 2022 at 02:48:49PM -0800, Yury Norov wrote:
> qed_rdma_bmap_free() is mostly an opencoded version of printk("%*pb").
> Using %*pb format simplifies the code, and helps to avoid inefficient
> usage of bitmap_weight().
> 
> While here, reorganize logic to avoid calculating bmap weight if check
> is false.

I like this kind of patches,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> Signed-off-by: Yury Norov <yury.norov@xxxxxxxxx>
> ---
> 
> This is RFC because it changes lines printing format to bitmap %*pb. If
> it hurts userspace, it's better to drop the patch.

How? The only way is some strange script that parses dmesg, but dmesg almost
never was an ABI, moreover, with printk() indexing feature (recently
introduced) the one who parses such messages can actually find the (new)
format as well.

>  drivers/net/ethernet/qlogic/qed/qed_rdma.c | 45 +++++++---------------
>  1 file changed, 14 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> index 23b668de4640..f4c04af9d4dd 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> @@ -319,44 +319,27 @@ static int qed_rdma_alloc(struct qed_hwfn *p_hwfn)
>  void qed_rdma_bmap_free(struct qed_hwfn *p_hwfn,
>  			struct qed_bmap *bmap, bool check)
>  {
> -	int weight = bitmap_weight(bmap->bitmap, bmap->max_count);
> -	int last_line = bmap->max_count / (64 * 8);
> -	int last_item = last_line * 8 +
> -	    DIV_ROUND_UP(bmap->max_count % (64 * 8), 64);
> -	u64 *pmap = (u64 *)bmap->bitmap;
> -	int line, item, offset;
> -	u8 str_last_line[200] = { 0 };
> -
> -	if (!weight || !check)
> +	unsigned int bit, weight, nbits;
> +	unsigned long *b;
> +
> +	if (!check)
> +		goto end;
> +
> +	weight = bitmap_weight(bmap->bitmap, bmap->max_count);
> +	if (!weight)
>  		goto end;
>  
>  	DP_NOTICE(p_hwfn,
>  		  "%s bitmap not free - size=%d, weight=%d, 512 bits per line\n",
>  		  bmap->name, bmap->max_count, weight);
>  
> -	/* 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))
> +	for (bit = 0; bit < bmap->max_count; bit += 512) {
> +		b =  bmap->bitmap + BITS_TO_LONGS(bit);
> +		nbits = min(bmap->max_count - bit, 512);
> +
> +		if (!bitmap_empty(b, nbits))
>  			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]);
> -
> -	/* print last unaligned non-zero line, if any */
> -	if ((bmap->max_count % (64 * 8)) &&
> -	    (bitmap_weight((unsigned long *)&pmap[item],
> -			   bmap->max_count - item * 64))) {
> -		offset = sprintf(str_last_line, "line 0x%04x: ", line);
> -		for (; item < last_item; item++)
> -			offset += sprintf(str_last_line + offset,
> -					  "0x%016llx ", pmap[item]);
> -		DP_NOTICE(p_hwfn, "%s\n", str_last_line);
> +				  "line 0x%04x: %*pb\n", bit / 512, nbits, b);
>  	}
>  
>  end:
> -- 
> 2.32.0
> 

-- 
With Best Regards,
Andy Shevchenko