Re: [PATCH v3] misc: pci_endpoint_test: Use memcpy_toio()/memcpy_fromio() for BAR tests

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 3/21/24 1:45 AM, Niklas Cassel wrote:
> Hello Kuppuswamy,
>
> On Wed, Mar 20, 2024 at 08:53:12AM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Hi,
>>
>> On 3/20/24 2:01 AM, Niklas Cassel wrote:
>>> The current code uses writel()/readl(), which has an implicit memory
>>> barrier for every single readl()/writel().
>>>
>>> Additionally, reading 4 bytes at a time over the PCI bus is not really
>>> optimal, considering that this code is running in an ioctl handler.
>>>
>>> Use memcpy_toio()/memcpy_fromio() for BAR tests.
>>>
>>> Before patch with a 4MB BAR:
>>> $ time /usr/bin/pcitest -b 1
>>> BAR1:           OKAY
>>> real    0m 1.56s
>>>
>>> After patch with a 4MB BAR:
>>> $ time /usr/bin/pcitest -b 1
>>> BAR1:           OKAY
>>> real    0m 0.54s
>>>
>>> Signed-off-by: Niklas Cassel <cassel@kernel.org>
>>> ---
>>> Changes since v2:
>>> -Actually free the allocated memory... (thank you Kuppuswamy)
>>>
>>>  drivers/misc/pci_endpoint_test.c | 68 ++++++++++++++++++++++++++------
>>>  1 file changed, 55 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>>> index 705029ad8eb5..1d361589fb61 100644
>>> --- a/drivers/misc/pci_endpoint_test.c
>>> +++ b/drivers/misc/pci_endpoint_test.c
>>> @@ -272,33 +272,75 @@ static const u32 bar_test_pattern[] = {
>>>  	0xA5A5A5A5,
>>>  };
>>>  
>>> +static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>>> +					enum pci_barno barno, int offset,
>>> +					void *write_buf, void *read_buf,
>>> +					int size)
>>> +{
>>> +	memset(write_buf, bar_test_pattern[barno], size);
>>> +	memcpy_toio(test->bar[barno] + offset, write_buf, size);
>>> +
>>> +	memcpy_fromio(read_buf, test->bar[barno] + offset, size);
>>> +
>>> +	return memcmp(write_buf, read_buf, size);
>>> +}
>>> +
>>>  static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>>>  				  enum pci_barno barno)
>>>  {
>>> -	int j;
>>> -	u32 val;
>>> -	int size;
>>> +	int j, bar_size, buf_size, iters, remain;
>>> +	void *write_buf;
>>> +	void *read_buf;
>>>  	struct pci_dev *pdev = test->pdev;
>>> +	bool ret;
>>>  
>>>  	if (!test->bar[barno])
>>>  		return false;
>>>  
>>> -	size = pci_resource_len(pdev, barno);
>>> +	bar_size = pci_resource_len(pdev, barno);
>>>  
>>>  	if (barno == test->test_reg_bar)
>>> -		size = 0x4;
>>> +		bar_size = 0x4;
>>>  
>>> -	for (j = 0; j < size; j += 4)
>>> -		pci_endpoint_test_bar_writel(test, barno, j,
>>> -					     bar_test_pattern[barno]);
>>> +	buf_size = min(SZ_1M, bar_size);
>> Why 1MB  limit?
> Could you please clarify your concern?

Since you are trying to optimize the number of read/write calls, I
was just wondering why you chose maximum limit of 1MB per
read/write call.  But your following explanation makes sense to
me. I recommend adding some comments about it in commit log
or code.

Code wise, your change looks fine to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>
> A BAR could be several GB, so it does not make sense to always kmalloc()
> a buffer that is of the same size of the BAR.
> (Therefore we copy in to a smaller buffer, iterating over the whole BAR.)
>
> So we have to chose a max limit that we think is likely to succeed even
> when the memory is fragmented, and something that will work on embedded
> systems, etc.
>
> The highest BAR size used by pci-epf-test is by default 1MB, so 1MB
> seemed like a reasonable max limit. (Since we use min(), if the BAR is
> smaller than 1MB, the buffer we allocate will also be smaller than 1MB.
>
> Since we allocate two buffers, we are in the worst case allocating 2x 1MB,
> so I don't think that it is reasonable to have a higher max limit.
>
> If you are using a _very_ resource contained system as RC (and EP) to test
> the pci-epf-test driver, you have probably reduced the default BAR sizes
> defined in pci-epf-test to something smaller already, so 1MB seemed like
> a reasonable max limit.
>
>
> Kind regards,
> Niklas
>
>>>  
>>> -	for (j = 0; j < size; j += 4) {
>>> -		val = pci_endpoint_test_bar_readl(test, barno, j);
>>> -		if (val != bar_test_pattern[barno])
>>> -			return false;
>>> +	write_buf = kmalloc(buf_size, GFP_KERNEL);
>>> +	if (!write_buf)
>>> +		return false;
>>> +
>>> +	read_buf = kmalloc(buf_size, GFP_KERNEL);
>>> +	if (!read_buf) {
>>> +		ret = false;
>>> +		goto err;
>>>  	}
>>>  
>>> -	return true;
>>> +	iters = bar_size / buf_size;
>>> +	for (j = 0; j < iters; j++) {
>>> +		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
>>> +						 write_buf, read_buf,
>>> +						 buf_size)) {
>>> +			ret = false;
>>> +			goto err;
>>> +		}
>>> +	}
>>> +
>>> +	remain = bar_size % buf_size;
>>> +	if (remain) {
>>> +		if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters,
>>> +						 write_buf, read_buf,
>>> +						 remain)) {
>>> +			ret = false;
>>> +			goto err;
>>> +		}
>>> +	}
>>> +
>>> +	ret = true;
>>> +
>>> +err:
>>> +	kfree(write_buf);
>>> +	kfree(read_buf);
>>> +
>>> +	return ret;
>>>  }
>>>  
>>>  static bool pci_endpoint_test_intx_irq(struct pci_endpoint_test *test)
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer