[PATCH] iommu/iova: Consider NUMA affinity when allocating memory for per-CPU iova_magazine

[PATCH] iommu/iova: Consider NUMA affinity when allocating memory for per-CPU iova_magazine

[Li RongQing]

per-CPU iova_magazine are dominantly accessed from their own local CPUs,
so allocate them node-local to improve performance.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/iommu/iova.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index d59d0ea..70f89ea 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -597,11 +597,11 @@ unsigned long iova_rcache_range(void)
 	return PAGE_SIZE << (IOVA_RANGE_CACHE_MAX_SIZE - 1);
 }
 
-static struct iova_magazine *iova_magazine_alloc(gfp_t flags)
+static struct iova_magazine *iova_magazine_alloc_node(gfp_t flags, int node)
 {
 	struct iova_magazine *mag;
 
-	mag = kmem_cache_alloc(iova_magazine_cache, flags);
+	mag = kmem_cache_alloc_node(iova_magazine_cache, flags, node);
 	if (mag)
 		mag->size = 0;
 
@@ -707,7 +707,7 @@ static void iova_depot_work_func(struct work_struct *work)
 int iova_domain_init_rcaches(struct iova_domain *iovad)
 {
 	unsigned int cpu;
-	int i, ret;
+	int i, ret, nid;
 
 	iovad->rcaches = kcalloc(IOVA_RANGE_CACHE_MAX_SIZE,
 				 sizeof(struct iova_rcache),
@@ -731,10 +731,11 @@ int iova_domain_init_rcaches(struct iova_domain *iovad)
 		}
 		for_each_possible_cpu(cpu) {
 			cpu_rcache = per_cpu_ptr(rcache->cpu_rcaches, cpu);
+			nid = cpu_to_node(cpu);
 
 			spin_lock_init(&cpu_rcache->lock);
-			cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
-			cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
+			cpu_rcache->loaded = iova_magazine_alloc_node(GFP_KERNEL, nid);
+			cpu_rcache->prev = iova_magazine_alloc_node(GFP_KERNEL, nid);
 			if (!cpu_rcache->loaded || !cpu_rcache->prev) {
 				ret = -ENOMEM;
 				goto out_err;
@@ -777,7 +778,9 @@ static bool __iova_rcache_insert(struct iova_domain *iovad,
 		swap(cpu_rcache->prev, cpu_rcache->loaded);
 		can_insert = true;
 	} else {
-		struct iova_magazine *new_mag = iova_magazine_alloc(GFP_ATOMIC);
+		int nid = cpu_to_node(raw_smp_processor_id());
+		struct iova_magazine *new_mag =
+			iova_magazine_alloc_node(GFP_ATOMIC, nid);
 
 		if (new_mag) {
 			spin_lock(&rcache->lock);
-- 
2.9.4

Re: [PATCH] iommu/iova: Consider NUMA affinity when allocating memory for per-CPU iova_magazine

[Robin Murphy]

On 15/04/2024 7:56 am, Li RongQing wrote:
> per-CPU iova_magazine are dominantly accessed from their own local CPUs,
> so allocate them node-local to improve performance.

Note that this will only hold for certain workloads (typically rather 
light ones, I'd guess) - do you have one where this makes a measurable 
difference? If so I'd be interested to know what sort of numbers we're 
talking about.

> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>   drivers/iommu/iova.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index d59d0ea..70f89ea 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -597,11 +597,11 @@ unsigned long iova_rcache_range(void)
>   	return PAGE_SIZE << (IOVA_RANGE_CACHE_MAX_SIZE - 1);
>   }
>   
> -static struct iova_magazine *iova_magazine_alloc(gfp_t flags)
> +static struct iova_magazine *iova_magazine_alloc_node(gfp_t flags, int node)
>   {
>   	struct iova_magazine *mag;
>   
> -	mag = kmem_cache_alloc(iova_magazine_cache, flags);
> +	mag = kmem_cache_alloc_node(iova_magazine_cache, flags, node);
>   	if (mag)
>   		mag->size = 0;
>   
> @@ -707,7 +707,7 @@ static void iova_depot_work_func(struct work_struct *work)
>   int iova_domain_init_rcaches(struct iova_domain *iovad)
>   {
>   	unsigned int cpu;
> -	int i, ret;
> +	int i, ret, nid;
>   
>   	iovad->rcaches = kcalloc(IOVA_RANGE_CACHE_MAX_SIZE,
>   				 sizeof(struct iova_rcache),
> @@ -731,10 +731,11 @@ int iova_domain_init_rcaches(struct iova_domain *iovad)
>   		}
>   		for_each_possible_cpu(cpu) {
>   			cpu_rcache = per_cpu_ptr(rcache->cpu_rcaches, cpu);
> +			nid = cpu_to_node(cpu);
>   
>   			spin_lock_init(&cpu_rcache->lock);
> -			cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
> -			cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
> +			cpu_rcache->loaded = iova_magazine_alloc_node(GFP_KERNEL, nid);
> +			cpu_rcache->prev = iova_magazine_alloc_node(GFP_KERNEL, nid);

This much seems reasonable - however small the benefit might be, there's 
little harm in aiming for optimal initial conditions, for as long as 
they do happen to last...

>   			if (!cpu_rcache->loaded || !cpu_rcache->prev) {
>   				ret = -ENOMEM;
>   				goto out_err;
> @@ -777,7 +778,9 @@ static bool __iova_rcache_insert(struct iova_domain *iovad,
>   		swap(cpu_rcache->prev, cpu_rcache->loaded);
>   		can_insert = true;
>   	} else {
> -		struct iova_magazine *new_mag = iova_magazine_alloc(GFP_ATOMIC);
> +		int nid = cpu_to_node(raw_smp_processor_id());
> +		struct iova_magazine *new_mag =
> +			iova_magazine_alloc_node(GFP_ATOMIC, nid);

...however if we get to this point then we're already busy enough to be 
churning magazines between the CPU caches and the global depot, so it's 
unlikely that new ones are going to stay CPU-local for very long either.

Thanks,
Robin.

>   
>   		if (new_mag) {
>   			spin_lock(&rcache->lock);

RE: [PATCH] iommu/iova: Consider NUMA affinity when allocating memory for per-CPU iova_magazine

[Li,Rongqing]

> On 15/04/2024 7:56 am, Li RongQing wrote:
> > per-CPU iova_magazine are dominantly accessed from their own local
> > CPUs, so allocate them node-local to improve performance.
> 
> Note that this will only hold for certain workloads (typically rather light ones, I'd
> guess) - do you have one where this makes a measurable difference? If so I'd be
> interested to know what sort of numbers we're talking about.
> 

I use iperf does some simple tests on Intel servers, no obvious difference, maybe it can give a good result on some NUMA unfriendly system

> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >   drivers/iommu/iova.c | 15 +++++++++------
> >   1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c index
> > d59d0ea..70f89ea 100644
> > --- a/drivers/iommu/iova.c
> > +++ b/drivers/iommu/iova.c
> > @@ -597,11 +597,11 @@ unsigned long iova_rcache_range(void)
> >   	return PAGE_SIZE << (IOVA_RANGE_CACHE_MAX_SIZE - 1);
> >   }
> >
> > -static struct iova_magazine *iova_magazine_alloc(gfp_t flags)
> > +static struct iova_magazine *iova_magazine_alloc_node(gfp_t flags,
> > +int node)
> >   {
> >   	struct iova_magazine *mag;
> >
> > -	mag = kmem_cache_alloc(iova_magazine_cache, flags);
> > +	mag = kmem_cache_alloc_node(iova_magazine_cache, flags, node);
> >   	if (mag)
> >   		mag->size = 0;
> >
> > @@ -707,7 +707,7 @@ static void iova_depot_work_func(struct work_struct
> *work)
> >   int iova_domain_init_rcaches(struct iova_domain *iovad)
> >   {
> >   	unsigned int cpu;
> > -	int i, ret;
> > +	int i, ret, nid;
> >
> >   	iovad->rcaches = kcalloc(IOVA_RANGE_CACHE_MAX_SIZE,
> >   				 sizeof(struct iova_rcache),
> > @@ -731,10 +731,11 @@ int iova_domain_init_rcaches(struct iova_domain
> *iovad)
> >   		}
> >   		for_each_possible_cpu(cpu) {
> >   			cpu_rcache = per_cpu_ptr(rcache->cpu_rcaches, cpu);
> > +			nid = cpu_to_node(cpu);
> >
> >   			spin_lock_init(&cpu_rcache->lock);
> > -			cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
> > -			cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
> > +			cpu_rcache->loaded = iova_magazine_alloc_node(GFP_KERNEL,
> nid);
> > +			cpu_rcache->prev = iova_magazine_alloc_node(GFP_KERNEL,
> nid);
> 
> This much seems reasonable - however small the benefit might be, there's little
> harm in aiming for optimal initial conditions, for as long as they do happen to
> last...
> 
> >   			if (!cpu_rcache->loaded || !cpu_rcache->prev) {
> >   				ret = -ENOMEM;
> >   				goto out_err;
> > @@ -777,7 +778,9 @@ static bool __iova_rcache_insert(struct iova_domain
> *iovad,
> >   		swap(cpu_rcache->prev, cpu_rcache->loaded);
> >   		can_insert = true;
> >   	} else {
> > -		struct iova_magazine *new_mag =
> iova_magazine_alloc(GFP_ATOMIC);
> > +		int nid = cpu_to_node(raw_smp_processor_id());
> > +		struct iova_magazine *new_mag =
> > +			iova_magazine_alloc_node(GFP_ATOMIC, nid);
> 
> ...however if we get to this point then we're already busy enough to be churning
> magazines between the CPU caches and the global depot, so it's unlikely that
> new ones are going to stay CPU-local for very long either.
> 

We can drop this change

Thanks

-Li