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

[Simon Horman <horms@kernel.org>]

On Tue, Feb 25, 2025 at 10:34:24AM +0800, Xin Tian wrote:
> On 2025/2/25 2:58, Simon Horman wrote:
> > 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.
> 
> Hi, Simon,
> 
> Assuming we use GENMASK, the mask should be defined as GENMASK(shift - 
> 1, 0).
> 
> When the system page size is 4K, shift will be 0, which will cause an error.

Understood, so much for that idea.