[dpdk-dev,31/41] ethdev: use contiguous allocation for DMA memory

Message ID a43f6a900f8b9eeba981005dbb49fcc5c59778b7.1520083504.git.anatoly.burakov@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply patch file failure

Commit Message

Anatoly Burakov March 3, 2018, 1:46 p.m. UTC
  This fixes the following drivers in one go:

grep -Rl rte_eth_dma_zone_reserve drivers/

drivers/net/avf/avf_rxtx.c
drivers/net/thunderx/nicvf_ethdev.c
drivers/net/e1000/igb_rxtx.c
drivers/net/e1000/em_rxtx.c
drivers/net/fm10k/fm10k_ethdev.c
drivers/net/vmxnet3/vmxnet3_rxtx.c
drivers/net/liquidio/lio_rxtx.c
drivers/net/i40e/i40e_rxtx.c
drivers/net/sfc/sfc.c
drivers/net/ixgbe/ixgbe_rxtx.c
drivers/net/nfp/nfp_net.c

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Comments

Andrew Rybchenko March 3, 2018, 2:05 p.m. UTC | #1
On 03/03/2018 04:46 PM, Anatoly Burakov wrote:
> This fixes the following drivers in one go:

Does it mean that these drivers are broken in the middle of patch set 
and fixed now?
If so, it would be good to avoid it. It breaks bisect.

> grep -Rl rte_eth_dma_zone_reserve drivers/
>
> drivers/net/avf/avf_rxtx.c
> drivers/net/thunderx/nicvf_ethdev.c
> drivers/net/e1000/igb_rxtx.c
> drivers/net/e1000/em_rxtx.c
> drivers/net/fm10k/fm10k_ethdev.c
> drivers/net/vmxnet3/vmxnet3_rxtx.c
> drivers/net/liquidio/lio_rxtx.c
> drivers/net/i40e/i40e_rxtx.c
> drivers/net/sfc/sfc.c
> drivers/net/ixgbe/ixgbe_rxtx.c
> drivers/net/nfp/nfp_net.c
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   lib/librte_ether/rte_ethdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 0590f0c..7935230 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -3401,7 +3401,8 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
>   	if (mz)
>   		return mz;
>   
> -	return rte_memzone_reserve_aligned(z_name, size, socket_id, 0, align);
> +	return rte_memzone_reserve_aligned_contig(z_name, size, socket_id, 0,
> +			align);
>   }
>   
>   int
  
Anatoly Burakov March 5, 2018, 9:08 a.m. UTC | #2
On 03-Mar-18 2:05 PM, Andrew Rybchenko wrote:
> On 03/03/2018 04:46 PM, Anatoly Burakov wrote:
>> This fixes the following drivers in one go:
> 
> Does it mean that these drivers are broken in the middle of patch set 
> and fixed now?
> If so, it would be good to avoid it. It breaks bisect.
> 

Depends on the definition of "broken". Legacy memory mode will still 
work for all drivers throughout the patchset. As for new memory mode, 
yes, it will be "broken in the middle of the patchset", but due to the 
fact that there's enormous amount of code to review between fbarray 
changes, malloc changes, contiguous allocation changes and adding new 
rte_memzone API's, i favored ease of code review over bisect.

I can of course reorder and roll up several different patchset and all 
driver updates into one giant patch, but do you really want to be the 
one reviewing such a patch?
  
Andrew Rybchenko March 5, 2018, 9:15 a.m. UTC | #3
On 03/05/2018 12:08 PM, Burakov, Anatoly wrote:
> On 03-Mar-18 2:05 PM, Andrew Rybchenko wrote:
>> On 03/03/2018 04:46 PM, Anatoly Burakov wrote:
>>> This fixes the following drivers in one go:
>>
>> Does it mean that these drivers are broken in the middle of patch set 
>> and fixed now?
>> If so, it would be good to avoid it. It breaks bisect.
>>
>
> Depends on the definition of "broken". Legacy memory mode will still 
> work for all drivers throughout the patchset. As for new memory mode, 
> yes, it will be "broken in the middle of the patchset", but due to the 
> fact that there's enormous amount of code to review between fbarray 
> changes, malloc changes, contiguous allocation changes and adding new 
> rte_memzone API's, i favored ease of code review over bisect.
>
> I can of course reorder and roll up several different patchset and all 
> driver updates into one giant patch, but do you really want to be the 
> one reviewing such a patch?

Is it possible to:
1. Introduce _contig function
2. Switch users of the contiguous allocation to it as you do now
3. Make the old function to allocate possibly non-contiguous memory
  
Anatoly Burakov March 5, 2018, 10 a.m. UTC | #4
On 05-Mar-18 9:15 AM, Andrew Rybchenko wrote:
> On 03/05/2018 12:08 PM, Burakov, Anatoly wrote:
>> On 03-Mar-18 2:05 PM, Andrew Rybchenko wrote:
>>> On 03/03/2018 04:46 PM, Anatoly Burakov wrote:
>>>> This fixes the following drivers in one go:
>>>
>>> Does it mean that these drivers are broken in the middle of patch set 
>>> and fixed now?
>>> If so, it would be good to avoid it. It breaks bisect.
>>>
>>
>> Depends on the definition of "broken". Legacy memory mode will still 
>> work for all drivers throughout the patchset. As for new memory mode, 
>> yes, it will be "broken in the middle of the patchset", but due to the 
>> fact that there's enormous amount of code to review between fbarray 
>> changes, malloc changes, contiguous allocation changes and adding new 
>> rte_memzone API's, i favored ease of code review over bisect.
>>
>> I can of course reorder and roll up several different patchset and all 
>> driver updates into one giant patch, but do you really want to be the 
>> one reviewing such a patch?
> 
> Is it possible to:
> 1. Introduce _contig function
> 2. Switch users of the contiguous allocation to it as you do now
> 3. Make the old function to allocate possibly non-contiguous memory
> 

Good point. I'll see if i can shuffle patches around for v2. Thanks!
  

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0590f0c..7935230 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3401,7 +3401,8 @@  rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
 	if (mz)
 		return mz;
 
-	return rte_memzone_reserve_aligned(z_name, size, socket_id, 0, align);
+	return rte_memzone_reserve_aligned_contig(z_name, size, socket_id, 0,
+			align);
 }
 
 int