Re: [PATCH v4 05/14] net-next/yunsilicon: Add eq and alloc

[Simon Horman <horms@kernel.org>]

On Thu, Feb 20, 2025 at 11:35:26PM +0800, tianx wrote:
> On 2025/2/19 1:10, Simon Horman wrote:
> > On Thu, Feb 13, 2025 at 05:14:14PM +0800, Xin Tian wrote:

...

> >> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/alloc.c b/drivers/net/ethernet/yunsilicon/xsc/pci/alloc.c
> > ...
> >
> >> +/* Handling for queue buffers -- we allocate a bunch of memory and
> >> + * register it in a memory region at HCA virtual address 0.  If the
> >> + * requested size is > max_direct, we split the allocation into
> >> + * multiple pages, so we don't require too much contiguous memory.
> >> + */
> > I can't help but think there is an existing API to handle this.
> failed to find one

Yes, me neither.

> >> +int xsc_buf_alloc(struct xsc_core_device *xdev, int size, int max_direct,
> > I think unsigned long would be slightly better types for size and max_direct.
> yes, will modify
> >> +		  struct xsc_buf *buf)
> >> +{
> >> +	dma_addr_t t;
> >> +
> >> +	buf->size = size;
> >> +	if (size <= max_direct) {
> >> +		buf->nbufs        = 1;
> >> +		buf->npages       = 1;
> >> +		buf->page_shift   = get_order(size) + PAGE_SHIFT;
> >> +		buf->direct.buf   = dma_alloc_coherent(&xdev->pdev->dev,
> >> +						       size,
> >> +						       &t,
> >> +						       GFP_KERNEL | __GFP_ZERO);
> >> +		if (!buf->direct.buf)
> >> +			return -ENOMEM;
> >> +
> >> +		buf->direct.map = t;
> >> +
> >> +		while (t & ((1 << buf->page_shift) - 1)) {
> > I think GENMASK() can be used here.
> ok
> >> +			--buf->page_shift;
> >> +			buf->npages *= 2;
> >> +		}
> >> +	} else {
> >> +		int i;
> >> +
> >> +		buf->direct.buf  = NULL;
> >> +		buf->nbufs       = (size + PAGE_SIZE - 1) / PAGE_SIZE;
> > I think this is open-coding DIV_ROUND_UP
> right, I'll change
> >> +		buf->npages      = buf->nbufs;
> >> +		buf->page_shift  = PAGE_SHIFT;
> >> +		buf->page_list   = kcalloc(buf->nbufs, sizeof(*buf->page_list),
> >> +					   GFP_KERNEL);
> >> +		if (!buf->page_list)
> >> +			return -ENOMEM;
> >> +
> >> +		for (i = 0; i < buf->nbufs; i++) {
> >> +			buf->page_list[i].buf =
> >> +				dma_alloc_coherent(&xdev->pdev->dev, PAGE_SIZE,
> >> +						   &t, GFP_KERNEL | __GFP_ZERO);
> >> +			if (!buf->page_list[i].buf)
> >> +				goto err_free;
> >> +
> >> +			buf->page_list[i].map = t;
> >> +		}
> >> +

> >> +		if (BITS_PER_LONG == 64) {
> >> +			struct page **pages;
> >> +
> >> +			pages = kmalloc_array(buf->nbufs, sizeof(*pages),
> >> +					      GFP_KERNEL);
> >> +			if (!pages)
> >> +				goto err_free;
> >> +			for (i = 0; i < buf->nbufs; i++) {
> >> +				void *addr = buf->page_list[i].buf;
> >> +
> >> +				if (is_vmalloc_addr(addr))
> >> +					pages[i] = vmalloc_to_page(addr);
> >> +				else
> >> +					pages[i] = virt_to_page(addr);
> >> +			}
> >> +			buf->direct.buf = vmap(pages, buf->nbufs,
> >> +					       VM_MAP, PAGE_KERNEL);
> >> +			kfree(pages);
> >> +			if (!buf->direct.buf)
> >> +				goto err_free;
> >> +		}
> > I think some explanation is warranted of why the above is relevant
> > only when BITS_PER_LONG == 64.
> Some strange historical reasons, and no need for the check now. I'll 
> clean this up

Thanks.

If you do need 64bit only logic, then perhaps it can be moved to a
separate function. It could guard code using something like this.

int some_func(struct xsc_buf *buf)
{
	if (!IS_ENABLED(CONFIG_64BIT))
		return 0;

	...
}

Or if that is not possible, something like this:

#ifdef CONFIG_64BIT
int some_func(struct xsc_buf *buf)
{
	...
}
#else /* CONFIG_64BIT */
int some_func(struct xsc_buf *buf) { return 0; }
#fi /* CONFIG_64BIT */

> >> +	}
> >> +
> >> +	return 0;
> >> +
> >> +err_free:
> >> +	xsc_buf_free(xdev, buf);
> >> +
> >> +	return -ENOMEM;
> >> +}
> > ...
> >
> >> +void xsc_fill_page_array(struct xsc_buf *buf, __be64 *pas, int npages)
> > As per my comment on unsigned long in my response to another patch,
> > I think npages can be unsigned long.
> ok
> >> +{
> >> +	int shift = PAGE_SHIFT - PAGE_SHIFT_4K;
> >> +	int mask = (1 << shift) - 1;
> > Likewise, I think that mask should be an unsigned long.
> > Or, both shift and mask could be #defines, as they are compile-time
> > constants.
> >
> > Also, mask can be generated using GENMASK, e.g.
> >
> > #define XSC_PAGE_ARRAY_MASK GENMASK(PAGE_SHIFT, PAGE_SHIFT_4K)
> > #define XSC_PAGE_ARRAY_SHIFT (PAGE_SHIFT - PAGE_SHIFT_4K)
> >
> > And I note, in the (common) case of 4k pages, that both shift and mask are 0.
> 
> Thank you for the suggestion, but that's not quite the case here. The 
> |shift| and |mask| are not used to extract fields from data. Instead, 
> they are part of a calculation. In |xsc_buf_alloc|, we allocate the 
> buffer based on the system's page size. However, in this function, we 
> need to break each page in the |buflist| into 4KB chunks, populate the 
> |pas| array with the corresponding DMA addresses, and then map them to 
> hardware.
> 
> The |shift| is calculated as |PAGE_SHIFT - PAGE_SHIFT_4K|, allowing us 
> to convert the 4KB chunk index (|i|) to the corresponding page index in 
> |buflist| with |i >> shift|. The |i & mask| gives us the offset of the 
> current 4KB chunk within the page, and by applying |((i & mask) << 
> PAGE_SHIFT_4K)|, we can compute the offset of that chunk within the page.
> 
> I hope this makes things clearer!

Thanks, that is clear.

I do still think that the shift and mask could
be compile-time constants rather than local variables.
And it does seem to me that GENMASK can be used to generate the mask.

...