[PATCH v4] net/iavf:enhance NUMA affinity heuristic

[PATCH v4] net/iavf:enhance NUMA affinity heuristic

[Kaisen You]

Trying to allocate memory on the first detected numa node has less
chance to find some memory actually available rather than on the main
lcore numa node (especially when the DPDK application is started only
on one numa node).

Fixes: 705356f0811f ("eal: simplify control thread creation")
Fixes: bb0bd346d5c1 ("eal: suggest using --lcores option")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Kaisen You <kaisenx.you@intel.com>
---
Changes since v3:
- add the assignment of socket_id in thread initialization,

Changes since v2:
- add uncommitted local change and fix compilation,

Changes since v1:
- accomodate for configurations with main lcore running on multiples
  physical cores belonging to different numa,
---
 lib/eal/common/eal_common_thread.c | 1 +
 lib/eal/common/malloc_heap.c       | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 38d83a6885..21bff971f8 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -251,6 +251,7 @@ static void *ctrl_thread_init(void *arg)
 	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
+	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d7c410b786..3ee19aee15 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -717,6 +717,10 @@ malloc_get_numa_socket(void)
 			return socket_id;
 	}
 
+	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
+	if (socket_id != (unsigned int)SOCKET_ID_ANY)
+		return socket_id;
+
 	return rte_socket_id_by_idx(0);
 }
 
-- 
2.34.1

Re: [PATCH v4] net/iavf:enhance NUMA affinity heuristic

[Thomas Monjalon]

31/01/2023 16:05, Kaisen You:
>  lib/eal/common/eal_common_thread.c | 1 +
>  lib/eal/common/malloc_heap.c       | 4 ++++
>  2 files changed, 5 insertions(+)

Why the title refers to net/iavf?

RE: [PATCH v4] net/iavf:enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: 2023年2月1日 0:06
> To: You, KaisenX <kaisenx.you@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; david.marchand@redhat.com; Matz, Olivier
> <olivier.matz@6wind.com>; ferruh.yigit@amd.com; You, KaisenX
> <kaisenx.you@intel.com>; zhoumin@loongson.cn; Burakov, Anatoly
> <anatoly.burakov@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v4] net/iavf:enhance NUMA affinity heuristic
> 
> 31/01/2023 16:05, Kaisen You:
> >  lib/eal/common/eal_common_thread.c | 1 +
> >  lib/eal/common/malloc_heap.c       | 4 ++++
> >  2 files changed, 5 insertions(+)
> 
> Why the title refers to net/iavf?

Sorry, the issue that was solved before is related to net/iavf. 
The new solution is compatible with all situations. I will release a new patch.
> 

[PATCH v5] enhance NUMA affinity heuristic

[Kaisen You]

Trying to allocate memory on the first detected numa node has less
chance to find some memory actually available rather than on the main
lcore numa node (especially when the DPDK application is started only
on one numa node).

Fixes: 705356f0811f ("eal: simplify control thread creation")
Fixes: bb0bd346d5c1 ("eal: suggest using --lcores option")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Kaisen You <kaisenx.you@intel.com>
---
Changes since v4:
- mod the patch title,

Changes since v3:
- add the assignment of socket_id in thread initialization,

Changes since v2:
- add uncommitted local change and fix compilation,

Changes since v1:
- accomodate for configurations with main lcore running on multiples
  physical cores belonging to different numa,
---
 lib/eal/common/eal_common_thread.c | 1 +
 lib/eal/common/malloc_heap.c       | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 38d83a6885..21bff971f8 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -251,6 +251,7 @@ static void *ctrl_thread_init(void *arg)
 	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
+	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d7c410b786..3ee19aee15 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -717,6 +717,10 @@ malloc_get_numa_socket(void)
 			return socket_id;
 	}
 
+	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
+	if (socket_id != (unsigned int)SOCKET_ID_ANY)
+		return socket_id;
+
 	return rte_socket_id_by_idx(0);
 }
 
-- 
2.34.1

Re: [PATCH v5] enhance NUMA affinity heuristic

[Burakov, Anatoly]

On 2/1/2023 12:20 PM, Kaisen You wrote:
> Trying to allocate memory on the first detected numa node has less
> chance to find some memory actually available rather than on the main
> lcore numa node (especially when the DPDK application is started only
> on one numa node).
> 
> Fixes: 705356f0811f ("eal: simplify control thread creation")
> Fixes: bb0bd346d5c1 ("eal: suggest using --lcores option")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> ---
> Changes since v4:
> - mod the patch title,
> 
> Changes since v3:
> - add the assignment of socket_id in thread initialization,
> 
> Changes since v2:
> - add uncommitted local change and fix compilation,
> 
> Changes since v1:
> - accomodate for configurations with main lcore running on multiples
>    physical cores belonging to different numa,
> ---
>   lib/eal/common/eal_common_thread.c | 1 +
>   lib/eal/common/malloc_heap.c       | 4 ++++
>   2 files changed, 5 insertions(+)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 38d83a6885..21bff971f8 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -251,6 +251,7 @@ static void *ctrl_thread_init(void *arg)
>   	void *routine_arg = params->arg;
>   
>   	__rte_thread_init(rte_lcore_id(), cpuset);
> +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
>   	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
>   	if (params->ret != 0) {
>   		__atomic_store_n(&params->ctrl_thread_status,
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index d7c410b786..3ee19aee15 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -717,6 +717,10 @@ malloc_get_numa_socket(void)
>   			return socket_id;
>   	}
>   
> +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> +		return socket_id;
> +
>   	return rte_socket_id_by_idx(0);
>   }
>   

I may be lacking context, but I don't quite get the suggested change. 
 From what I understand, the original has to do with assigning lcore 
cpusets in such a way that an lcore ends up having two socket ID's 
(because it's been assigned to CPU's on different sockets). Why is this 
allowed in the first place? It seems like a user error to me, as it 
breaks many of the fundamental assumptions DPDK makes.

I'm fine with using main lcore socket for control threads, I just don't 
think the `socket_id != SOCKET_ID_ANY` thing should be checked here, 
because it apparently tries to compensate for a problem with cpuset of 
the main thread, which shouldn't have happened to begin with.

-- 
Thanks,
Anatoly

Re: [PATCH v5] enhance NUMA affinity heuristic

[Burakov, Anatoly]

On 2/15/2023 2:22 PM, Burakov, Anatoly wrote:
> On 2/1/2023 12:20 PM, Kaisen You wrote:
>> Trying to allocate memory on the first detected numa node has less
>> chance to find some memory actually available rather than on the main
>> lcore numa node (especially when the DPDK application is started only
>> on one numa node).
>>
>> Fixes: 705356f0811f ("eal: simplify control thread creation")
>> Fixes: bb0bd346d5c1 ("eal: suggest using --lcores option")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> Signed-off-by: Kaisen You <kaisenx.you@intel.com>
>> ---
>> Changes since v4:
>> - mod the patch title,
>>
>> Changes since v3:
>> - add the assignment of socket_id in thread initialization,
>>
>> Changes since v2:
>> - add uncommitted local change and fix compilation,
>>
>> Changes since v1:
>> - accomodate for configurations with main lcore running on multiples
>>    physical cores belonging to different numa,
>> ---
>>   lib/eal/common/eal_common_thread.c | 1 +
>>   lib/eal/common/malloc_heap.c       | 4 ++++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/lib/eal/common/eal_common_thread.c 
>> b/lib/eal/common/eal_common_thread.c
>> index 38d83a6885..21bff971f8 100644
>> --- a/lib/eal/common/eal_common_thread.c
>> +++ b/lib/eal/common/eal_common_thread.c
>> @@ -251,6 +251,7 @@ static void *ctrl_thread_init(void *arg)
>>       void *routine_arg = params->arg;
>>       __rte_thread_init(rte_lcore_id(), cpuset);
>> +    RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
>>       params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), 
>> cpuset);
>>       if (params->ret != 0) {
>>           __atomic_store_n(&params->ctrl_thread_status,
>> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
>> index d7c410b786..3ee19aee15 100644
>> --- a/lib/eal/common/malloc_heap.c
>> +++ b/lib/eal/common/malloc_heap.c
>> @@ -717,6 +717,10 @@ malloc_get_numa_socket(void)
>>               return socket_id;
>>       }
>> +    socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
>> +    if (socket_id != (unsigned int)SOCKET_ID_ANY)
>> +        return socket_id;
>> +
>>       return rte_socket_id_by_idx(0);
>>   }
> 
> I may be lacking context, but I don't quite get the suggested change. 
>  From what I understand, the original has to do with assigning lcore 
> cpusets in such a way that an lcore ends up having two socket ID's 
> (because it's been assigned to CPU's on different sockets). Why is this 
> allowed in the first place? It seems like a user error to me, as it 
> breaks many of the fundamental assumptions DPDK makes.
> 
> I'm fine with using main lcore socket for control threads, I just don't 
> think the `socket_id != SOCKET_ID_ANY` thing should be checked here, 
> because it apparently tries to compensate for a problem with cpuset of 
> the main thread, which shouldn't have happened to begin with.
> 
Just to be clear: I don't have any objections to this patch otherwise.

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

RE: [PATCH v5] enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: 2023年2月15日 22:23
> To: You, KaisenX <kaisenx.you@intel.com>; dev@dpdk.org
> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; thomas@monjalon.net;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org
> Subject: Re: [PATCH v5] enhance NUMA affinity heuristic
> 
> On 2/1/2023 12:20 PM, Kaisen You wrote:
> > Trying to allocate memory on the first detected numa node has less
> > chance to find some memory actually available rather than on the main
> > lcore numa node (especially when the DPDK application is started only
> > on one numa node).
> >
> > Fixes: 705356f0811f ("eal: simplify control thread creation")
> > Fixes: bb0bd346d5c1 ("eal: suggest using --lcores option")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> > ---
> > Changes since v4:
> > - mod the patch title,
> >
> > Changes since v3:
> > - add the assignment of socket_id in thread initialization,
> >
> > Changes since v2:
> > - add uncommitted local change and fix compilation,
> >
> > Changes since v1:
> > - accomodate for configurations with main lcore running on multiples
> >    physical cores belonging to different numa,
> > ---
> >   lib/eal/common/eal_common_thread.c | 1 +
> >   lib/eal/common/malloc_heap.c       | 4 ++++
> >   2 files changed, 5 insertions(+)
> >
> > diff --git a/lib/eal/common/eal_common_thread.c
> > b/lib/eal/common/eal_common_thread.c
> > index 38d83a6885..21bff971f8 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -251,6 +251,7 @@ static void *ctrl_thread_init(void *arg)
> >   	void *routine_arg = params->arg;
> >
> >   	__rte_thread_init(rte_lcore_id(), cpuset);
> > +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
> >   	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(),
> cpuset);
> >   	if (params->ret != 0) {
> >   		__atomic_store_n(&params->ctrl_thread_status,
> > diff --git a/lib/eal/common/malloc_heap.c
> > b/lib/eal/common/malloc_heap.c index d7c410b786..3ee19aee15 100644
> > --- a/lib/eal/common/malloc_heap.c
> > +++ b/lib/eal/common/malloc_heap.c
> > @@ -717,6 +717,10 @@ malloc_get_numa_socket(void)
> >   			return socket_id;
> >   	}
> >
> > +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> > +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > +		return socket_id;
> > +
> >   	return rte_socket_id_by_idx(0);
> >   }
> >
> 
> I may be lacking context, but I don't quite get the suggested change.
>  From what I understand, the original has to do with assigning lcore cpusets in
> such a way that an lcore ends up having two socket ID's (because it's been
> assigned to CPU's on different sockets). Why is this allowed in the first place?
> It seems like a user error to me, as it breaks many of the fundamental
> assumptions DPDK makes.
> 
In a dual socket system, if all used cores are in socket 1 and the NIC is in socket 1, 
no memory is allocated for socket 0. This is to optimize memory consumption.

I agree with you. If the startup parameters can ensure that both sockets 
allocate memory, there will be no problem.
However, due to the different CPU topologies of different systems, 
It is difficult for users to ensure that the startup parameter contains two cpu nodes.

> I'm fine with using main lcore socket for control threads, I just don't think the
> `socket_id != SOCKET_ID_ANY` thing should be checked here, because it
> apparently tries to compensate for a problem with cpuset of the main thread,
> which shouldn't have happened to begin with.
> 
This issue has been explained in detail in the discussion of the patch v1 version. 
I will forward the previous email to you. The content of the email will also better 
let you know the purpose of submitting this patch.

> --
> Thanks,
> Anatoly

Re: [PATCH v5] enhance NUMA affinity heuristic

[Thomas Monjalon]

I'm not comfortable with this patch.

First, there is no comment in the code which helps to understand the logic.
Second, I'm afraid changing the value of the per-core variable _socket_id
may have an impact on some applications.


16/02/2023 03:50, You, KaisenX:
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > On 2/1/2023 12:20 PM, Kaisen You wrote:
> > > Trying to allocate memory on the first detected numa node has less
> > > chance to find some memory actually available rather than on the main
> > > lcore numa node (especially when the DPDK application is started only
> > > on one numa node).
> > >
> > > Fixes: 705356f0811f ("eal: simplify control thread creation")
> > > Fixes: bb0bd346d5c1 ("eal: suggest using --lcores option")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> > > ---
> > > Changes since v4:
> > > - mod the patch title,
> > >
> > > Changes since v3:
> > > - add the assignment of socket_id in thread initialization,
> > >
> > > Changes since v2:
> > > - add uncommitted local change and fix compilation,
> > >
> > > Changes since v1:
> > > - accomodate for configurations with main lcore running on multiples
> > >    physical cores belonging to different numa,
> > > ---
> > >   lib/eal/common/eal_common_thread.c | 1 +
> > >   lib/eal/common/malloc_heap.c       | 4 ++++
> > >   2 files changed, 5 insertions(+)
> > >
> > > diff --git a/lib/eal/common/eal_common_thread.c
> > > b/lib/eal/common/eal_common_thread.c
> > > index 38d83a6885..21bff971f8 100644
> > > --- a/lib/eal/common/eal_common_thread.c
> > > +++ b/lib/eal/common/eal_common_thread.c
> > > @@ -251,6 +251,7 @@ static void *ctrl_thread_init(void *arg)
> > >   	void *routine_arg = params->arg;
> > >
> > >   	__rte_thread_init(rte_lcore_id(), cpuset);
> > > +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
> > >   	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(),
> > cpuset);
> > >   	if (params->ret != 0) {
> > >   		__atomic_store_n(&params->ctrl_thread_status,
> > > diff --git a/lib/eal/common/malloc_heap.c
> > > b/lib/eal/common/malloc_heap.c index d7c410b786..3ee19aee15 100644
> > > --- a/lib/eal/common/malloc_heap.c
> > > +++ b/lib/eal/common/malloc_heap.c
> > > @@ -717,6 +717,10 @@ malloc_get_numa_socket(void)
> > >   			return socket_id;
> > >   	}
> > >
> > > +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> > > +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > > +		return socket_id;
> > > +
> > >   	return rte_socket_id_by_idx(0);
> > >   }
> > >
> > 
> > I may be lacking context, but I don't quite get the suggested change.
> >  From what I understand, the original has to do with assigning lcore cpusets in
> > such a way that an lcore ends up having two socket ID's (because it's been
> > assigned to CPU's on different sockets). Why is this allowed in the first place?
> > It seems like a user error to me, as it breaks many of the fundamental
> > assumptions DPDK makes.
> > 
> In a dual socket system, if all used cores are in socket 1 and the NIC is in socket 1, 
> no memory is allocated for socket 0. This is to optimize memory consumption.
> 
> I agree with you. If the startup parameters can ensure that both sockets 
> allocate memory, there will be no problem.
> However, due to the different CPU topologies of different systems, 
> It is difficult for users to ensure that the startup parameter contains two cpu nodes.
> 
> > I'm fine with using main lcore socket for control threads, I just don't think the
> > `socket_id != SOCKET_ID_ANY` thing should be checked here, because it
> > apparently tries to compensate for a problem with cpuset of the main thread,
> > which shouldn't have happened to begin with.
> > 
> This issue has been explained in detail in the discussion of the patch v1 version. 
> I will forward the previous email to you. The content of the email will also better 
> let you know the purpose of submitting this patch.

RE: [PATCH v5] enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: 2023年3月3日 22:07
> To: Burakov, Anatoly <anatoly.burakov@intel.com>; You, KaisenX
> <kaisenx.you@intel.com>
> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com
> Subject: Re: [PATCH v5] enhance NUMA affinity heuristic
> 
> I'm not comfortable with this patch.
> 
> First, there is no comment in the code which helps to understand the logic.
> Second, I'm afraid changing the value of the per-core variable _socket_id
> may have an impact on some applications.
> 
Thank you for your reply.
First, about comments, I can submit a new patch to add comments to help understand.
Second, if you do not change the value of the per-core variable_ socket_ id,
/lib/eal/common/malloc_heap.c
malloc_get_numa_socket(void)
{
        const struct internal_config *conf = eal_get_internal_configuration();
        unsigned int socket_id = rte_socket_id();   // The return value of "rte_socket_id()" is 1
        unsigned int idx;

        if (socket_id != (unsigned int)SOCKET_ID_ANY)
                return socket_id;    //so return here 

This will cause return here, This function returns the socket_id of unallocated memory.

If you have a better solution, I can modify it.
> 16/02/2023 03:50, You, KaisenX:
> > From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > > On 2/1/2023 12:20 PM, Kaisen You wrote:
> > > > Trying to allocate memory on the first detected numa node has less
> > > > chance to find some memory actually available rather than on the
> > > > main lcore numa node (especially when the DPDK application is
> > > > started only on one numa node).
> > > >
> > > > Fixes: 705356f0811f ("eal: simplify control thread creation")
> > > > Fixes: bb0bd346d5c1 ("eal: suggest using --lcores option")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> > > > ---
> > > > Changes since v4:
> > > > - mod the patch title,
> > > >
> > > > Changes since v3:
> > > > - add the assignment of socket_id in thread initialization,
> > > >
> > > > Changes since v2:
> > > > - add uncommitted local change and fix compilation,
> > > >
> > > > Changes since v1:
> > > > - accomodate for configurations with main lcore running on multiples
> > > >    physical cores belonging to different numa,
> > > > ---
> > > >   lib/eal/common/eal_common_thread.c | 1 +
> > > >   lib/eal/common/malloc_heap.c       | 4 ++++
> > > >   2 files changed, 5 insertions(+)
> > > >
> > > > diff --git a/lib/eal/common/eal_common_thread.c
> > > > b/lib/eal/common/eal_common_thread.c
> > > > index 38d83a6885..21bff971f8 100644
> > > > --- a/lib/eal/common/eal_common_thread.c
> > > > +++ b/lib/eal/common/eal_common_thread.c
> > > > @@ -251,6 +251,7 @@ static void *ctrl_thread_init(void *arg)
> > > >   	void *routine_arg = params->arg;
> > > >
> > > >   	__rte_thread_init(rte_lcore_id(), cpuset);
> > > > +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
> > > >   	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(),
> > > cpuset);
> > > >   	if (params->ret != 0) {
> > > >   		__atomic_store_n(&params->ctrl_thread_status,
> > > > diff --git a/lib/eal/common/malloc_heap.c
> > > > b/lib/eal/common/malloc_heap.c index d7c410b786..3ee19aee15
> 100644
> > > > --- a/lib/eal/common/malloc_heap.c
> > > > +++ b/lib/eal/common/malloc_heap.c
> > > > @@ -717,6 +717,10 @@ malloc_get_numa_socket(void)
> > > >   			return socket_id;
> > > >   	}
> > > >
> > > > +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> > > > +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > > > +		return socket_id;
> > > > +
> > > >   	return rte_socket_id_by_idx(0);
> > > >   }
> > > >
> > >
> > > I may be lacking context, but I don't quite get the suggested change.
> > >  From what I understand, the original has to do with assigning lcore
> > > cpusets in such a way that an lcore ends up having two socket ID's
> > > (because it's been assigned to CPU's on different sockets). Why is this
> allowed in the first place?
> > > It seems like a user error to me, as it breaks many of the
> > > fundamental assumptions DPDK makes.
> > >
> > In a dual socket system, if all used cores are in socket 1 and the NIC
> > is in socket 1, no memory is allocated for socket 0. This is to optimize
> memory consumption.
> >
> > I agree with you. If the startup parameters can ensure that both
> > sockets allocate memory, there will be no problem.
> > However, due to the different CPU topologies of different systems, It
> > is difficult for users to ensure that the startup parameter contains two cpu
> nodes.
> >
> > > I'm fine with using main lcore socket for control threads, I just
> > > don't think the `socket_id != SOCKET_ID_ANY` thing should be checked
> > > here, because it apparently tries to compensate for a problem with
> > > cpuset of the main thread, which shouldn't have happened to begin with.
> > >
> > This issue has been explained in detail in the discussion of the patch v1
> version.
> > I will forward the previous email to you. The content of the email
> > will also better let you know the purpose of submitting this patch.
> 
> 

RE: [PATCH v5] enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: You, KaisenX
> Sent: 2023年3月9日 9:58
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com;
> Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: RE: [PATCH v5] enhance NUMA affinity heuristic
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: 2023年3月3日 22:07
> > To: Burakov, Anatoly <anatoly.burakov@intel.com>; You, KaisenX
> > <kaisenx.you@intel.com>
> > Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> > david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> > ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> > Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com
> > Subject: Re: [PATCH v5] enhance NUMA affinity heuristic
> >
> > I'm not comfortable with this patch.
> >
> > First, there is no comment in the code which helps to understand the logic.
> > Second, I'm afraid changing the value of the per-core variable
> > _socket_id may have an impact on some applications.
> >
Hi Thomas, I'm sorry to bother you again, but we can't think of a better solution for now,
would you please give me some suggestion, and then I will modify it accordingly.

> Thank you for your reply.
> First, about comments, I can submit a new patch to add comments to help
> understand.
> Second, if you do not change the value of the per-core variable_ socket_ id,
> /lib/eal/common/malloc_heap.c
> malloc_get_numa_socket(void)
> {
>         const struct internal_config *conf = eal_get_internal_configuration();
>         unsigned int socket_id = rte_socket_id();   // The return value of
> "rte_socket_id()" is 1
>         unsigned int idx;
> 
>         if (socket_id != (unsigned int)SOCKET_ID_ANY)
>                 return socket_id;    //so return here
> 
> This will cause return here, This function returns the socket_id of unallocated
> memory.
> 
> If you have a better solution, I can modify it.
> > 16/02/2023 03:50, You, KaisenX:
> > > From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > > > On 2/1/2023 12:20 PM, Kaisen You wrote:
> > > > > Trying to allocate memory on the first detected numa node has
> > > > > less chance to find some memory actually available rather than
> > > > > on the main lcore numa node (especially when the DPDK
> > > > > application is started only on one numa node).
> > > > >
> > > > > Fixes: 705356f0811f ("eal: simplify control thread creation")
> > > > > Fixes: bb0bd346d5c1 ("eal: suggest using --lcores option")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > > Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> > > > > ---
> > > > > Changes since v4:
> > > > > - mod the patch title,
> > > > >
> > > > > Changes since v3:
> > > > > - add the assignment of socket_id in thread initialization,
> > > > >
> > > > > Changes since v2:
> > > > > - add uncommitted local change and fix compilation,
> > > > >
> > > > > Changes since v1:
> > > > > - accomodate for configurations with main lcore running on multiples
> > > > >    physical cores belonging to different numa,
> > > > > ---
> > > > >   lib/eal/common/eal_common_thread.c | 1 +
> > > > >   lib/eal/common/malloc_heap.c       | 4 ++++
> > > > >   2 files changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/lib/eal/common/eal_common_thread.c
> > > > > b/lib/eal/common/eal_common_thread.c
> > > > > index 38d83a6885..21bff971f8 100644
> > > > > --- a/lib/eal/common/eal_common_thread.c
> > > > > +++ b/lib/eal/common/eal_common_thread.c
> > > > > @@ -251,6 +251,7 @@ static void *ctrl_thread_init(void *arg)
> > > > >   	void *routine_arg = params->arg;
> > > > >
> > > > >   	__rte_thread_init(rte_lcore_id(), cpuset);
> > > > > +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
> > > > >   	params->ret =
> > > > > rte_thread_set_affinity_by_id(rte_thread_self(),
> > > > cpuset);
> > > > >   	if (params->ret != 0) {
> > > > >   		__atomic_store_n(&params->ctrl_thread_status,
> > > > > diff --git a/lib/eal/common/malloc_heap.c
> > > > > b/lib/eal/common/malloc_heap.c index d7c410b786..3ee19aee15
> > 100644
> > > > > --- a/lib/eal/common/malloc_heap.c
> > > > > +++ b/lib/eal/common/malloc_heap.c
> > > > > @@ -717,6 +717,10 @@ malloc_get_numa_socket(void)
> > > > >   			return socket_id;
> > > > >   	}
> > > > >
> > > > > +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> > > > > +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > > > > +		return socket_id;
> > > > > +
> > > > >   	return rte_socket_id_by_idx(0);
> > > > >   }
> > > > >
> > > >
> > > > I may be lacking context, but I don't quite get the suggested change.
> > > >  From what I understand, the original has to do with assigning
> > > > lcore cpusets in such a way that an lcore ends up having two
> > > > socket ID's (because it's been assigned to CPU's on different
> > > > sockets). Why is this
> > allowed in the first place?
> > > > It seems like a user error to me, as it breaks many of the
> > > > fundamental assumptions DPDK makes.
> > > >
> > > In a dual socket system, if all used cores are in socket 1 and the
> > > NIC is in socket 1, no memory is allocated for socket 0. This is to
> > > optimize
> > memory consumption.
> > >
> > > I agree with you. If the startup parameters can ensure that both
> > > sockets allocate memory, there will be no problem.
> > > However, due to the different CPU topologies of different systems,
> > > It is difficult for users to ensure that the startup parameter
> > > contains two cpu
> > nodes.
> > >
> > > > I'm fine with using main lcore socket for control threads, I just
> > > > don't think the `socket_id != SOCKET_ID_ANY` thing should be
> > > > checked here, because it apparently tries to compensate for a
> > > > problem with cpuset of the main thread, which shouldn't have
> happened to begin with.
> > > >
> > > This issue has been explained in detail in the discussion of the
> > > patch v1
> > version.
> > > I will forward the previous email to you. The content of the email
> > > will also better let you know the purpose of submitting this patch.
> >
> >

Re: [PATCH v5] enhance NUMA affinity heuristic

[Thomas Monjalon]

13/04/2023 02:56, You, KaisenX:
> From: You, KaisenX
> > From: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > I'm not comfortable with this patch.
> > >
> > > First, there is no comment in the code which helps to understand the logic.
> > > Second, I'm afraid changing the value of the per-core variable
> > > _socket_id may have an impact on some applications.
> > >
> Hi Thomas, I'm sorry to bother you again, but we can't think of a better solution for now,
> would you please give me some suggestion, and then I will modify it accordingly.

You need to better explain the logic
both in the commit message and in code comments.
When it will be done, it will be easier to have a discussion
with other maintainers and community experts.
Thank you

> > Thank you for your reply.
> > First, about comments, I can submit a new patch to add comments to help
> > understand.
> > Second, if you do not change the value of the per-core variable_ socket_ id,
> > /lib/eal/common/malloc_heap.c
> > malloc_get_numa_socket(void)
> > {
> >         const struct internal_config *conf = eal_get_internal_configuration();
> >         unsigned int socket_id = rte_socket_id();   // The return value of
> > "rte_socket_id()" is 1
> >         unsigned int idx;
> > 
> >         if (socket_id != (unsigned int)SOCKET_ID_ANY)
> >                 return socket_id;    //so return here
> > 
> > This will cause return here, This function returns the socket_id of unallocated
> > memory.
> > 
> > If you have a better solution, I can modify it.

RE: [PATCH v5] enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: 2023年4月19日 20:17
> To: You, KaisenX <kaisenx.you@intel.com>
> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com;
> Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: Re: [PATCH v5] enhance NUMA affinity heuristic
> 
> 13/04/2023 02:56, You, KaisenX:
> > From: You, KaisenX
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > >
> > > > I'm not comfortable with this patch.
> > > >
> > > > First, there is no comment in the code which helps to understand the
> logic.
> > > > Second, I'm afraid changing the value of the per-core variable
> > > > _socket_id may have an impact on some applications.
> > > >
> > Hi Thomas, I'm sorry to bother you again, but we can't think of a
> > better solution for now, would you please give me some suggestion, and
> then I will modify it accordingly.
> 
> You need to better explain the logic
> both in the commit message and in code comments.
> When it will be done, it will be easier to have a discussion with other
> maintainers and community experts.
> Thank you
> 
Thank you for your reply, I'll explain my patch in more detail next.

When a DPDK application is started on only one numa node, memory is allocated 
for only one socket.When interrupt threads use memory, memory may not be found 
on the socket where the interrupt thread is currently located, and memory has to be 
reallocated on the hugepage, this operation can lead to performance degradation.

So my modification is in the function malloc_get_numa_socket to make sure 
that the first socket with memory can be returned.

If you can accept my explanation and modification, I will send the V6 
version to improve the commit message and code comments.

> > > Thank you for your reply.
> > > First, about comments, I can submit a new patch to add comments to
> > > help understand.
> > > Second, if you do not change the value of the per-core variable_
> > > socket_ id, /lib/eal/common/malloc_heap.c
> > > malloc_get_numa_socket(void)
> > > {
> > >         const struct internal_config *conf = eal_get_internal_configuration();
> > >         unsigned int socket_id = rte_socket_id();   // The return value of
> > > "rte_socket_id()" is 1
> > >         unsigned int idx;
> > >
> > >         if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > >                 return socket_id;    //so return here
> > >
> > > This will cause return here, This function returns the socket_id of
> > > unallocated memory.
> > >
> > > If you have a better solution, I can modify it.
> 
> 
> 

Re: [PATCH v5] enhance NUMA affinity heuristic

[Thomas Monjalon]

21/04/2023 04:34, You, KaisenX:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 13/04/2023 02:56, You, KaisenX:
> > > From: You, KaisenX
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > >
> > > > > I'm not comfortable with this patch.
> > > > >
> > > > > First, there is no comment in the code which helps to understand the
> > logic.
> > > > > Second, I'm afraid changing the value of the per-core variable
> > > > > _socket_id may have an impact on some applications.
> > > > >
> > > Hi Thomas, I'm sorry to bother you again, but we can't think of a
> > > better solution for now, would you please give me some suggestion, and
> > then I will modify it accordingly.
> > 
> > You need to better explain the logic
> > both in the commit message and in code comments.
> > When it will be done, it will be easier to have a discussion with other
> > maintainers and community experts.
> > Thank you
> > 
> Thank you for your reply, I'll explain my patch in more detail next.
> 
> When a DPDK application is started on only one numa node,

What do you mean by started on only one node?

> memory is allocated for only one socket.
> When interrupt threads use memory, memory may not be found 
> on the socket where the interrupt thread is currently located,

Why interrupt thread is on a different socket?

> and memory has to be reallocated on the hugepage,
> this operation can lead to performance degradation.
> 
> So my modification is in the function malloc_get_numa_socket to make sure 
> that the first socket with memory can be returned.
> 
> If you can accept my explanation and modification, I will send the V6 
> version to improve the commit message and code comments.
> 
> > > > Thank you for your reply.
> > > > First, about comments, I can submit a new patch to add comments to
> > > > help understand.
> > > > Second, if you do not change the value of the per-core variable_
> > > > socket_ id, /lib/eal/common/malloc_heap.c
> > > > malloc_get_numa_socket(void)
> > > > {
> > > >         const struct internal_config *conf = eal_get_internal_configuration();
> > > >         unsigned int socket_id = rte_socket_id();   // The return value of
> > > > "rte_socket_id()" is 1
> > > >         unsigned int idx;
> > > >
> > > >         if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > > >                 return socket_id;    //so return here
> > > >
> > > > This will cause return here, This function returns the socket_id of
> > > > unallocated memory.
> > > >
> > > > If you have a better solution, I can modify it.

RE: [PATCH v5] enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: 2023年4月21日 16:13
> To: You, KaisenX <kaisenx.you@intel.com>
> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com;
> Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: Re: [PATCH v5] enhance NUMA affinity heuristic
> 
> 21/04/2023 04:34, You, KaisenX:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 13/04/2023 02:56, You, KaisenX:
> > > > From: You, KaisenX
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > >
> > > > > > I'm not comfortable with this patch.
> > > > > >
> > > > > > First, there is no comment in the code which helps to
> > > > > > understand the
> > > logic.
> > > > > > Second, I'm afraid changing the value of the per-core variable
> > > > > > _socket_id may have an impact on some applications.
> > > > > >
> > > > Hi Thomas, I'm sorry to bother you again, but we can't think of a
> > > > better solution for now, would you please give me some suggestion,
> > > > and
> > > then I will modify it accordingly.
> > >
> > > You need to better explain the logic both in the commit message and
> > > in code comments.
> > > When it will be done, it will be easier to have a discussion with
> > > other maintainers and community experts.
> > > Thank you
> > >
> > Thank you for your reply, I'll explain my patch in more detail next.
> >
> > When a DPDK application is started on only one numa node,
> 
> What do you mean by started on only one node?
When the dpdk application is started with the startup parameter "-l 40-59" 
(this range is on the same node as the system cpu processor).Only memory is 
allocated for this node when the process is initialized.
> 
> > memory is allocated for only one socket.
> > When interrupt threads use memory, memory may not be found on the
> > socket where the interrupt thread is currently located,
> 
> Why interrupt thread is on a different socket?
The above only allocates memory on node1, but the interrupt thread is created on node0.
Interrupt threads are created by rte_ctrl_thread_create() ,rte_ctrl_thread_create()' 
does NOT run on main lcore, it can run on any core except data plane cores. 
So interrupt thread can run on any core.
> > and memory has to be reallocated on the hugepage, this operation can
> > lead to performance degradation.
> >
> > So my modification is in the function malloc_get_numa_socket to make
> > sure that the first socket with memory can be returned.
> >
> > If you can accept my explanation and modification, I will send the V6
> > version to improve the commit message and code comments.
> >
> > > > > Thank you for your reply.
> > > > > First, about comments, I can submit a new patch to add comments
> > > > > to help understand.
> > > > > Second, if you do not change the value of the per-core variable_
> > > > > socket_ id, /lib/eal/common/malloc_heap.c
> > > > > malloc_get_numa_socket(void)
> > > > > {
> > > > >         const struct internal_config *conf =
> eal_get_internal_configuration();
> > > > >         unsigned int socket_id = rte_socket_id();   // The return value of
> > > > > "rte_socket_id()" is 1
> > > > >         unsigned int idx;
> > > > >
> > > > >         if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > > > >                 return socket_id;    //so return here
> > > > >
> > > > > This will cause return here, This function returns the socket_id
> > > > > of unallocated memory.
> > > > >
> > > > > If you have a better solution, I can modify it.
> 
> 
> 

RE: [PATCH v5] enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: You, KaisenX <kaisenx.you@intel.com>
> Sent: 2023年4月23日 14:52
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com;
> Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: RE: [PATCH v5] enhance NUMA affinity heuristic
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: 2023年4月21日 16:13
> > To: You, KaisenX <kaisenx.you@intel.com>
> > Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> > david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> > ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> > Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com;
> > Burakov, Anatoly <anatoly.burakov@intel.com>
> > Subject: Re: [PATCH v5] enhance NUMA affinity heuristic
> >
> > 21/04/2023 04:34, You, KaisenX:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 13/04/2023 02:56, You, KaisenX:
> > > > > From: You, KaisenX
> > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > >
> > > > > > > I'm not comfortable with this patch.
> > > > > > >
> > > > > > > First, there is no comment in the code which helps to
> > > > > > > understand the
> > > > logic.
> > > > > > > Second, I'm afraid changing the value of the per-core
> > > > > > > variable _socket_id may have an impact on some applications.
> > > > > > >
> > > > > Hi Thomas, I'm sorry to bother you again, but we can't think of
> > > > > a better solution for now, would you please give me some
> > > > > suggestion, and
> > > > then I will modify it accordingly.
> > > >
> > > > You need to better explain the logic both in the commit message
> > > > and in code comments.
> > > > When it will be done, it will be easier to have a discussion with
> > > > other maintainers and community experts.
> > > > Thank you
> > > >
> > > Thank you for your reply, I'll explain my patch in more detail next.
> > >
> > > When a DPDK application is started on only one numa node,
> >
> > What do you mean by started on only one node?
> When the dpdk application is started with the startup parameter "-l 40-59"
> (this range is on the same node as the system cpu processor).Only memory is
> allocated for this node when the process is initialized.
> >
> > > memory is allocated for only one socket.
> > > When interrupt threads use memory, memory may not be found on the
> > > socket where the interrupt thread is currently located,
> >
> > Why interrupt thread is on a different socket?
> The above only allocates memory on node1, but the interrupt thread is
> created on node0.
> Interrupt threads are created by
> rte_ctrl_thread_create() ,rte_ctrl_thread_create()'
> does NOT run on main lcore, it can run on any core except data plane cores.
> So interrupt thread can run on any core.
To avoid ambiguity, I'll explain my commet again. Interrupt threads are created by
rte_ctrl_thread_create() , rte_ctrl_thread_create() doesn't control which node the 
thread it creates runs on, interrupt threads can run on any core except data plane 
cores. So interrupt thread can run on any core.
> > > and memory has to be reallocated on the hugepage, this operation can
> > > lead to performance degradation.
> > >
> > > So my modification is in the function malloc_get_numa_socket to make
> > > sure that the first socket with memory can be returned.
> > >
> > > If you can accept my explanation and modification, I will send the
> > > V6 version to improve the commit message and code comments.
> > >
> > > > > > Thank you for your reply.
> > > > > > First, about comments, I can submit a new patch to add
> > > > > > comments to help understand.
> > > > > > Second, if you do not change the value of the per-core
> > > > > > variable_ socket_ id, /lib/eal/common/malloc_heap.c
> > > > > > malloc_get_numa_socket(void)
> > > > > > {
> > > > > >         const struct internal_config *conf =
> > eal_get_internal_configuration();
> > > > > >         unsigned int socket_id = rte_socket_id();   // The return value of
> > > > > > "rte_socket_id()" is 1
> > > > > >         unsigned int idx;
> > > > > >
> > > > > >         if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > > > > >                 return socket_id;    //so return here
> > > > > >
> > > > > > This will cause return here, This function returns the
> > > > > > socket_id of unallocated memory.
> > > > > >
> > > > > > If you have a better solution, I can modify it.
> >
> >
> >

Re: [PATCH v5] enhance NUMA affinity heuristic

[Thomas Monjalon]

OK please send v6 with comments in the code where appropriate.
We'll continue the discussion in v6.
Thanks


23/04/2023 10:57, You, KaisenX:
> 
> > -----Original Message-----
> > From: You, KaisenX <kaisenx.you@intel.com>
> > Sent: 2023年4月23日 14:52
> > To: Thomas Monjalon <thomas@monjalon.net>
> > Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> > david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> > ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> > Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com;
> > Burakov, Anatoly <anatoly.burakov@intel.com>
> > Subject: RE: [PATCH v5] enhance NUMA affinity heuristic
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Sent: 2023年4月21日 16:13
> > > To: You, KaisenX <kaisenx.you@intel.com>
> > > Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> > > david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> > > ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org;
> > > Richardson, Bruce <bruce.richardson@intel.com>; jerinj@marvell.com;
> > > Burakov, Anatoly <anatoly.burakov@intel.com>
> > > Subject: Re: [PATCH v5] enhance NUMA affinity heuristic
> > >
> > > 21/04/2023 04:34, You, KaisenX:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 13/04/2023 02:56, You, KaisenX:
> > > > > > From: You, KaisenX
> > > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > >
> > > > > > > > I'm not comfortable with this patch.
> > > > > > > >
> > > > > > > > First, there is no comment in the code which helps to
> > > > > > > > understand the
> > > > > logic.
> > > > > > > > Second, I'm afraid changing the value of the per-core
> > > > > > > > variable _socket_id may have an impact on some applications.
> > > > > > > >
> > > > > > Hi Thomas, I'm sorry to bother you again, but we can't think of
> > > > > > a better solution for now, would you please give me some
> > > > > > suggestion, and
> > > > > then I will modify it accordingly.
> > > > >
> > > > > You need to better explain the logic both in the commit message
> > > > > and in code comments.
> > > > > When it will be done, it will be easier to have a discussion with
> > > > > other maintainers and community experts.
> > > > > Thank you
> > > > >
> > > > Thank you for your reply, I'll explain my patch in more detail next.
> > > >
> > > > When a DPDK application is started on only one numa node,
> > >
> > > What do you mean by started on only one node?
> > When the dpdk application is started with the startup parameter "-l 40-59"
> > (this range is on the same node as the system cpu processor).Only memory is
> > allocated for this node when the process is initialized.
> > >
> > > > memory is allocated for only one socket.
> > > > When interrupt threads use memory, memory may not be found on the
> > > > socket where the interrupt thread is currently located,
> > >
> > > Why interrupt thread is on a different socket?
> > The above only allocates memory on node1, but the interrupt thread is
> > created on node0.
> > Interrupt threads are created by
> > rte_ctrl_thread_create() ,rte_ctrl_thread_create()'
> > does NOT run on main lcore, it can run on any core except data plane cores.
> > So interrupt thread can run on any core.
> To avoid ambiguity, I'll explain my commet again. Interrupt threads are created by
> rte_ctrl_thread_create() , rte_ctrl_thread_create() doesn't control which node the 
> thread it creates runs on, interrupt threads can run on any core except data plane 
> cores. So interrupt thread can run on any core.
> > > > and memory has to be reallocated on the hugepage, this operation can
> > > > lead to performance degradation.
> > > >
> > > > So my modification is in the function malloc_get_numa_socket to make
> > > > sure that the first socket with memory can be returned.
> > > >
> > > > If you can accept my explanation and modification, I will send the
> > > > V6 version to improve the commit message and code comments.
> > > >
> > > > > > > Thank you for your reply.
> > > > > > > First, about comments, I can submit a new patch to add
> > > > > > > comments to help understand.
> > > > > > > Second, if you do not change the value of the per-core
> > > > > > > variable_ socket_ id, /lib/eal/common/malloc_heap.c
> > > > > > > malloc_get_numa_socket(void)
> > > > > > > {
> > > > > > >         const struct internal_config *conf =
> > > eal_get_internal_configuration();
> > > > > > >         unsigned int socket_id = rte_socket_id();   // The return value of
> > > > > > > "rte_socket_id()" is 1
> > > > > > >         unsigned int idx;
> > > > > > >
> > > > > > >         if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > > > > > >                 return socket_id;    //so return here
> > > > > > >
> > > > > > > This will cause return here, This function returns the
> > > > > > > socket_id of unallocated memory.
> > > > > > >
> > > > > > > If you have a better solution, I can modify it.
> > >
> > >
> > >
> 
> 

[PATCH v6] enhance NUMA affinity heuristic

[Kaisen You]

Trying to allocate memory on the first detected numa node,it has less
chance to find some memory actually available rather than on the main
lcore numa node (especially when the DPDK application is started only
on one numa node).

Fixes: 8b0a1b8cb481 ("eal: stop using pthread for lcores and control threads")
Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Kaisen You <kaisenx.you@intel.com>

---
Changes since v5:
- Add comments to the code,

Changes since v4:
- mod the patch title,

Changes since v3:
- add the assignment of socket_id in thread initialization,

Changes since v2:
- add uncommitted local change and fix compilation,

Changes since v1:
- accomodate for configurations with main lcore running on multiples
  physical cores belonging to different numa,
---
 lib/eal/common/eal_common_thread.c | 4 ++++
 lib/eal/common/malloc_heap.c       | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 079a385630..d65bfe251b 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -252,6 +252,10 @@ static int ctrl_thread_init(void *arg)
 	struct rte_thread_ctrl_params *params = arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
+	/* set the value of the per-core variable _socket_id.
+	 * Convenient for threads to find memory.
+	 */
+	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d25bdc98f9..a624f08cf7 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -716,6 +716,12 @@ malloc_get_numa_socket(void)
 		if (conf->socket_mem[socket_id] != 0)
 			return socket_id;
 	}
+	/* Trying to allocate memory on the main lcore numa node.
+	 * especially when the DPDK application is started only on one numa node.
+	 */
+	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
+	if (socket_id != (unsigned int)SOCKET_ID_ANY)
+		return socket_id;
 
 	return rte_socket_id_by_idx(0);
 }
-- 
2.25.1

Re: [PATCH v6] enhance NUMA affinity heuristic

[Thomas Monjalon]

25/04/2023 07:16, Kaisen You:
> Trying to allocate memory on the first detected numa node,it has less
> chance to find some memory actually available rather than on the main
> lcore numa node (especially when the DPDK application is started only
> on one numa node).

You didn't change the explanations here as discussed previously.

> 
> Fixes: 8b0a1b8cb481 ("eal: stop using pthread for lcores and control threads")
> Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> 
> ---
> Changes since v5:
> - Add comments to the code,
> 
> Changes since v4:
> - mod the patch title,
> 
> Changes since v3:
> - add the assignment of socket_id in thread initialization,
> 
> Changes since v2:
> - add uncommitted local change and fix compilation,
> 
> Changes since v1:
> - accomodate for configurations with main lcore running on multiples
>   physical cores belonging to different numa,
> ---
>  lib/eal/common/eal_common_thread.c | 4 ++++
>  lib/eal/common/malloc_heap.c       | 6 ++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 079a385630..d65bfe251b 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -252,6 +252,10 @@ static int ctrl_thread_init(void *arg)
>  	struct rte_thread_ctrl_params *params = arg;
>  
>  	__rte_thread_init(rte_lcore_id(), cpuset);
> +	/* set the value of the per-core variable _socket_id.
> +	 * Convenient for threads to find memory.
> +	 */

That's an useless comment.
We want to know WHY you are setting to SOCKET_ID_ANY.

> +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
>  	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
>  	if (params->ret != 0) {
>  		__atomic_store_n(&params->ctrl_thread_status,
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index d25bdc98f9..a624f08cf7 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -716,6 +716,12 @@ malloc_get_numa_socket(void)
>  		if (conf->socket_mem[socket_id] != 0)
>  			return socket_id;
>  	}
> +	/* Trying to allocate memory on the main lcore numa node.
> +	 * especially when the DPDK application is started only on one numa node.
> +	 */
> +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> +		return socket_id;

You should explain why we could reach this point, i.e. SOCKET_ID_ANY.

>  
>  	return rte_socket_id_by_idx(0);
>  }

RE: [PATCH v6] enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: 2023年4月27日 14:58
> To: You, KaisenX <kaisenx.you@intel.com>
> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> ferruh.yigit@amd.com; Burakov, Anatoly <anatoly.burakov@intel.com>; You,
> KaisenX <kaisenx.you@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v6] enhance NUMA affinity heuristic
> 
> 25/04/2023 07:16, Kaisen You:
> > Trying to allocate memory on the first detected numa node,it has less
> > chance to find some memory actually available rather than on the main
> > lcore numa node (especially when the DPDK application is started only
> > on one numa node).
> 
> You didn't change the explanations here as discussed previously.
> 
> >
> > Fixes: 8b0a1b8cb481 ("eal: stop using pthread for lcores and control
> > threads")
> > Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> >
> > ---
> > Changes since v5:
> > - Add comments to the code,
> >
> > Changes since v4:
> > - mod the patch title,
> >
> > Changes since v3:
> > - add the assignment of socket_id in thread initialization,
> >
> > Changes since v2:
> > - add uncommitted local change and fix compilation,
> >
> > Changes since v1:
> > - accomodate for configurations with main lcore running on multiples
> >   physical cores belonging to different numa,
> > ---
> >  lib/eal/common/eal_common_thread.c | 4 ++++
> >  lib/eal/common/malloc_heap.c       | 6 ++++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/lib/eal/common/eal_common_thread.c
> > b/lib/eal/common/eal_common_thread.c
> > index 079a385630..d65bfe251b 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -252,6 +252,10 @@ static int ctrl_thread_init(void *arg)
> >  	struct rte_thread_ctrl_params *params = arg;
> >
> >  	__rte_thread_init(rte_lcore_id(), cpuset);
> > +	/* set the value of the per-core variable _socket_id.
> > +	 * Convenient for threads to find memory.
> > +	 */
> 
> That's an useless comment.
> We want to know WHY you are setting to SOCKET_ID_ANY.
> 
> > +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
> >  	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(),
> cpuset);
> >  	if (params->ret != 0) {
> >  		__atomic_store_n(&params->ctrl_thread_status,
> > diff --git a/lib/eal/common/malloc_heap.c
> > b/lib/eal/common/malloc_heap.c index d25bdc98f9..a624f08cf7 100644
> > --- a/lib/eal/common/malloc_heap.c
> > +++ b/lib/eal/common/malloc_heap.c
> > @@ -716,6 +716,12 @@ malloc_get_numa_socket(void)
> >  		if (conf->socket_mem[socket_id] != 0)
> >  			return socket_id;
> >  	}
> > +	/* Trying to allocate memory on the main lcore numa node.
> > +	 * especially when the DPDK application is started only on one numa
> node.
> > +	 */
> > +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> > +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > +		return socket_id;
> 
> You should explain why we could reach this point, i.e. SOCKET_ID_ANY.

I'm sorry I didn't reply to you in time due to COVID-19 infection, I will send the v7 
version as soon as possible to fix the problem you mentioned.
> 
> >
> >  	return rte_socket_id_by_idx(0);
> >  }
> 
> 

[PATCH v7] enhance NUMA affinity heuristic

[Kaisen You]

When a DPDK application is started on only one numa node, memory is 
allocated for only one socket. When interrupt threads use memory, 
memory may not be found on the socket where the interrupt thread 
is currently located, and memory has to be reallocated on the hugepage, 
this operation will lead to performance degradation.

Fixes: 705356f0811f ("eal: simplify control thread creation")
Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
Cc: stable@dpdk.org

Signed-off-by: Kaisen You <kaisenx.you@intel.com>
---
Changes since v6:
- New explanation for easy understanding,

Changes since v5:
- Add comments to the code,

Changes since v4:
- mod the patch title,

Changes since v3:
- add the assignment of socket_id in thread initialization,

Changes since v2:
- add uncommitted local change and fix compilation,

Changes since v1:
- accomodate for configurations with main lcore running on multiples
  physical cores belonging to different numa,
---
 lib/eal/common/eal_common_thread.c | 6 ++++++
 lib/eal/common/malloc_heap.c       | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 079a385630..6479b66da1 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -252,6 +252,12 @@ static int ctrl_thread_init(void *arg)
 	struct rte_thread_ctrl_params *params = arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
+	/* set the value of the per-core variable _socket_id to SOCKET_ID_ANY.
+	 * Satisfy the judgment condition when threads find memory.
+	 * If SOCKET_ID_ANY is not specified, the thread may go to a node with
+	 * unallocated memory in a subsequent memory search.
+	 */
+	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d25bdc98f9..6d37f8afee 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -716,6 +716,15 @@ malloc_get_numa_socket(void)
 		if (conf->socket_mem[socket_id] != 0)
 			return socket_id;
 	}
+	/* Trying to allocate memory on the main lcore numa node.
+	 * especially when the DPDK application is started only on one numa node.
+	 */
+	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
+	/* When the socket_id obtained in the main lcore numa is SOCKET_ID_ANY,
+	 * The probability of finding memory on rte_socket_id_by_idx(0) is higher.
+	 */
+	if (socket_id != (unsigned int)SOCKET_ID_ANY)
+		return socket_id;
 
 	return rte_socket_id_by_idx(0);
 }
-- 
2.25.1

Re: [PATCH v7] enhance NUMA affinity heuristic

[Burakov, Anatoly]

On 5/23/2023 3:50 AM, Kaisen You wrote:
> When a DPDK application is started on only one numa node, memory is
> allocated for only one socket. When interrupt threads use memory,
> memory may not be found on the socket where the interrupt thread
> is currently located, and memory has to be reallocated on the hugepage,
> this operation will lead to performance degradation.
> 
> Fixes: 705356f0811f ("eal: simplify control thread creation")
> Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>

Hi You,

I've suggested comment rewordings based on my understanding of the issue.

> ---
> Changes since v6:
> - New explanation for easy understanding,
> 
> Changes since v5:
> - Add comments to the code,
> 
> Changes since v4:
> - mod the patch title,
> 
> Changes since v3:
> - add the assignment of socket_id in thread initialization,
> 
> Changes since v2:
> - add uncommitted local change and fix compilation,
> 
> Changes since v1:
> - accomodate for configurations with main lcore running on multiples
>    physical cores belonging to different numa,
> ---
>   lib/eal/common/eal_common_thread.c | 6 ++++++
>   lib/eal/common/malloc_heap.c       | 9 +++++++++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 079a385630..6479b66da1 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -252,6 +252,12 @@ static int ctrl_thread_init(void *arg)
>   	struct rte_thread_ctrl_params *params = arg;
>   
>   	__rte_thread_init(rte_lcore_id(), cpuset);
> +	/* set the value of the per-core variable _socket_id to SOCKET_ID_ANY.
> +	 * Satisfy the judgment condition when threads find memory.
> +	 * If SOCKET_ID_ANY is not specified, the thread may go to a node with
> +	 * unallocated memory in a subsequent memory search.

I suggest a different comment wording:

Set control thread socket ID to SOCKET_ID_ANY as control threads may be 
scheduled on any NUMA node.

> +	 */
> +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
>   	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
>   	if (params->ret != 0) {
>   		__atomic_store_n(&params->ctrl_thread_status,
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index d25bdc98f9..6d37f8afee 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -716,6 +716,15 @@ malloc_get_numa_socket(void)
>   		if (conf->socket_mem[socket_id] != 0)
>   			return socket_id;
>   	}
> +	/* Trying to allocate memory on the main lcore numa node.
> +	 * especially when the DPDK application is started only on one numa node.
> +	 */

I suggest the following comment wording:

We couldn't find quickly find a NUMA node where memory was available, so 
fall back to using main lcore socket ID.

> +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> +	/* When the socket_id obtained in the main lcore numa is SOCKET_ID_ANY,
> +	 * The probability of finding memory on rte_socket_id_by_idx(0) is higher.
> +	 */

I suggest the following comment wording:

Main lcore socket ID may be SOCKET_ID_ANY in cases when main lcore 
thread is affinitized to multiple NUMA nodes.

> +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> +		return socket_id;
>   

I suggest adding comment here:

Failed to find meaningful socket ID, so just use the first one available.

>   	return rte_socket_id_by_idx(0);
>   }

I believe these comments offer better explanation as to why we are doing 
the things we do here.

Whether or not you decide to take these corrections on board,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [PATCH v7] enhance NUMA affinity heuristic

[Burakov, Anatoly]

On 5/23/2023 3:50 AM, Kaisen You wrote:
> When a DPDK application is started on only one numa node, memory is
> allocated for only one socket. When interrupt threads use memory,
> memory may not be found on the socket where the interrupt thread
> is currently located, and memory has to be reallocated on the hugepage,
> this operation will lead to performance degradation.
> 
> Fixes: 705356f0811f ("eal: simplify control thread creation")
> Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> ---

For the record, I still think that this is a solution for a problem that 
should be fixed elsewhere, because a DPDK lcore (even main lcore!) 
having a specific NUMA node affinity is one of the most fundamental 
assumptions about DPDK, and I feel like we're inviting problems if we 
allow lcores to have multiple NUMA node affinities.

For example, if I run DPDK test app with the following command-line:

--lcores "1@(1,29),2@(30)"

The malloc autotest will fail because main lcore now returns -1 when 
we're calling `rte_socket_id()` from it. Correspondigly, any API's that 
use `rte_socket_id()` internally for various purposes (especially 
indexing arrays!) will now have to account for the fact that 
`rte_socket_id()` can just return -1 and it is not an exceptional situation.

IMO if we want to keep this behavior, EAL should at least warn the user 
that a DPDK lcore was assigned SOCKET_ID_ANY on account of multiple NUMA 
nodes being in its cpuset. So, as an unrealted change (so, i'm not 
suggesting doing it in this specific patchset), I would suggest that 
`thread_update_affinity()` should warn about DPDK lcore being assigned 
socket ID like that.

-- 
Thanks,
Anatoly

RE: [PATCH v7] enhance NUMA affinity heuristic

[You, KaisenX]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: 2023年5月23日 18:45
> To: You, KaisenX <kaisenx.you@intel.com>; dev@dpdk.org
> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; thomas@monjalon.net;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>;
> ferruh.yigit@amd.com; zhoumin@loongson.cn; stable@dpdk.org
> Subject: Re: [PATCH v7] enhance NUMA affinity heuristic
> 
> On 5/23/2023 3:50 AM, Kaisen You wrote:
> > When a DPDK application is started on only one numa node, memory is
> > allocated for only one socket. When interrupt threads use memory,
> > memory may not be found on the socket where the interrupt thread is
> > currently located, and memory has to be reallocated on the hugepage,
> > this operation will lead to performance degradation.
> >
> > Fixes: 705356f0811f ("eal: simplify control thread creation")
> > Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> 
> Hi You,
> 
> I've suggested comment rewordings based on my understanding of the issue.
> 
> > ---
> > Changes since v6:
> > - New explanation for easy understanding,
> >
> > Changes since v5:
> > - Add comments to the code,
> >
> > Changes since v4:
> > - mod the patch title,
> >
> > Changes since v3:
> > - add the assignment of socket_id in thread initialization,
> >
> > Changes since v2:
> > - add uncommitted local change and fix compilation,
> >
> > Changes since v1:
> > - accomodate for configurations with main lcore running on multiples
> >    physical cores belonging to different numa,
> > ---
> >   lib/eal/common/eal_common_thread.c | 6 ++++++
> >   lib/eal/common/malloc_heap.c       | 9 +++++++++
> >   2 files changed, 15 insertions(+)
> >
> > diff --git a/lib/eal/common/eal_common_thread.c
> > b/lib/eal/common/eal_common_thread.c
> > index 079a385630..6479b66da1 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -252,6 +252,12 @@ static int ctrl_thread_init(void *arg)
> >   	struct rte_thread_ctrl_params *params = arg;
> >
> >   	__rte_thread_init(rte_lcore_id(), cpuset);
> > +	/* set the value of the per-core variable _socket_id to
> SOCKET_ID_ANY.
> > +	 * Satisfy the judgment condition when threads find memory.
> > +	 * If SOCKET_ID_ANY is not specified, the thread may go to a node
> with
> > +	 * unallocated memory in a subsequent memory search.
> 
> I suggest a different comment wording:
> 
> Set control thread socket ID to SOCKET_ID_ANY as control threads may be
> scheduled on any NUMA node.
> 
> > +	 */
> > +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
> >   	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(),
> cpuset);
> >   	if (params->ret != 0) {
> >   		__atomic_store_n(&params->ctrl_thread_status,
> > diff --git a/lib/eal/common/malloc_heap.c
> > b/lib/eal/common/malloc_heap.c index d25bdc98f9..6d37f8afee 100644
> > --- a/lib/eal/common/malloc_heap.c
> > +++ b/lib/eal/common/malloc_heap.c
> > @@ -716,6 +716,15 @@ malloc_get_numa_socket(void)
> >   		if (conf->socket_mem[socket_id] != 0)
> >   			return socket_id;
> >   	}
> > +	/* Trying to allocate memory on the main lcore numa node.
> > +	 * especially when the DPDK application is started only on one numa
> node.
> > +	 */
> 
> I suggest the following comment wording:
> 
> We couldn't find quickly find a NUMA node where memory was available, so
> fall back to using main lcore socket ID.
> 
> > +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> > +	/* When the socket_id obtained in the main lcore numa is
> SOCKET_ID_ANY,
> > +	 * The probability of finding memory on rte_socket_id_by_idx(0) is
> higher.
> > +	 */
> 
> I suggest the following comment wording:
> 
> Main lcore socket ID may be SOCKET_ID_ANY in cases when main lcore
> thread is affinitized to multiple NUMA nodes.
> 
> > +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> > +		return socket_id;
> >
> 
> I suggest adding comment here:
> 
> Failed to find meaningful socket ID, so just use the first one available.
> 
> >   	return rte_socket_id_by_idx(0);
> >   }
> 
> I believe these comments offer better explanation as to why we are doing
> the things we do here.
> 
> Whether or not you decide to take these corrections on board,
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Thank you for your acked and suggestions, I will adopt your suggestions in the V8 version.
> 
> --
> Thanks,
> Anatoly

[PATCH v8] enhance NUMA affinity heuristic

[Kaisen You]

When a DPDK application is started on only one numa node, memory is
allocated for only one socket. When interrupt threads use memory,
memory may not be found on the socket where the interrupt thread
is currently located, and memory has to be reallocated on the hugepage,
this operation will lead to performance degradation.

Fixes: 705356f0811f ("eal: simplify control thread creation")
Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
Cc: stable@dpdk.org

Signed-off-by: Kaisen You <kaisenx.you@intel.com>

---
Changes since v7:
- Update commet,

Changes since v6:
- New explanation for easy understanding,

Changes since v5:
- Add comments to the code,

Changes since v4:
- mod the patch title,

Changes since v3:
- add the assignment of socket_id in thread initialization,

Changes since v2:
- add uncommitted local change and fix compilation,

Changes since v1:
- accomodate for configurations with main lcore running on multiples
  physical cores belonging to different numa,
---
 lib/eal/common/eal_common_thread.c | 2 ++
 lib/eal/common/malloc_heap.c       | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 079a385630..95720a61c0 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -252,6 +252,8 @@ static int ctrl_thread_init(void *arg)
 	struct rte_thread_ctrl_params *params = arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
+	/* Set control thread socket ID to SOCKET_ID_ANY as
+	 * control threads may be scheduled on any NUMA node.
+	 */
+	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d25bdc98f9..5de2317827 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -716,7 +716,12 @@ malloc_get_numa_socket(void)
 		if (conf->socket_mem[socket_id] != 0)
 			return socket_id;
 	}
-
+	/* We couldn't find quickly find a NUMA node where memory
+	 * was available, so fall back to using main lcore socket ID.
+	 */
+	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
+	/* Main lcore socket ID may be SOCKET_ID_ANY in cases when main
+	 * lcore thread is affinitized to multiple NUMA nodes.
+	 */
+	if (socket_id != (unsigned int)SOCKET_ID_ANY)
+		return socket_id;
+	/* Failed to find meaningful socket ID, so just use the first one available */
 	return rte_socket_id_by_idx(0);
 }
 
-- 
2.25.1

[PATCH v8] enhance NUMA affinity heuristic

[Kaisen You]

When a DPDK application is started on only one numa node, memory is
allocated for only one socket. When interrupt threads use memory,
memory may not be found on the socket where the interrupt thread
is currently located, and memory has to be reallocated on the hugepage,
this operation will lead to performance degradation.

Fixes: 705356f0811f ("eal: simplify control thread creation")
Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
Cc: stable@dpdk.org

Signed-off-by: Kaisen You <kaisenx.you@intel.com>
---
Changes since v7:
- Update commet,

Changes since v6:
- New explanation for easy understanding,

Changes since v5:
- Add comments to the code,

Changes since v4:
- mod the patch title,

Changes since v3:
- add the assignment of socket_id in thread initialization,

Changes since v2:
- add uncommitted local change and fix compilation,

Changes since v1:
- accomodate for configurations with main lcore running on multiples
  physical cores belonging to different numa,
---
 lib/eal/common/eal_common_thread.c |  4 ++++
 lib/eal/common/malloc_heap.c       | 11 ++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 079a385630..22480aa61f 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -252,6 +252,10 @@ static int ctrl_thread_init(void *arg)
 	struct rte_thread_ctrl_params *params = arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
+	/* Set control thread socket ID to SOCKET_ID_ANY as control
+	 * threads may be scheduled on any NUMA node.
+	 */
+	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d25bdc98f9..d833a71e7a 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -716,7 +716,16 @@ malloc_get_numa_socket(void)
 		if (conf->socket_mem[socket_id] != 0)
 			return socket_id;
 	}
-
+	/* We couldn't find quickly find a NUMA node where memory was available,
+	 * so fall back to using main lcore socket ID.
+	 */
+	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
+	/* Main lcore socket ID may be SOCKET_ID_ANY in cases when main lcore
+	 * thread is affinitized to multiple NUMA nodes.
+	 */
+	if (socket_id != (unsigned int)SOCKET_ID_ANY)
+		return socket_id;
+	/* Failed to find meaningful socket ID, so just use the first one available */
 	return rte_socket_id_by_idx(0);
 }
 
-- 
2.25.1

Re: [PATCH v8] enhance NUMA affinity heuristic

[Burakov, Anatoly]

On 5/26/2023 9:45 AM, Kaisen You wrote:
> When a DPDK application is started on only one numa node, memory is
> allocated for only one socket. When interrupt threads use memory,
> memory may not be found on the socket where the interrupt thread
> is currently located, and memory has to be reallocated on the hugepage,
> this operation will lead to performance degradation.
> 
> Fixes: 705356f0811f ("eal: simplify control thread creation")
> Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> ---
> Changes since v7:
> - Update commet,
> 
> Changes since v6:
> - New explanation for easy understanding,
> 
> Changes since v5:
> - Add comments to the code,
> 
> Changes since v4:
> - mod the patch title,
> 
> Changes since v3:
> - add the assignment of socket_id in thread initialization,
> 
> Changes since v2:
> - add uncommitted local change and fix compilation,
> 
> Changes since v1:
> - accomodate for configurations with main lcore running on multiples
>    physical cores belonging to different numa,
> ---
>   lib/eal/common/eal_common_thread.c |  4 ++++
>   lib/eal/common/malloc_heap.c       | 11 ++++++++++-
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 079a385630..22480aa61f 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -252,6 +252,10 @@ static int ctrl_thread_init(void *arg)
>   	struct rte_thread_ctrl_params *params = arg;
>   
>   	__rte_thread_init(rte_lcore_id(), cpuset);
> +	/* Set control thread socket ID to SOCKET_ID_ANY as control
> +	 * threads may be scheduled on any NUMA node.
> +	 */
> +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
>   	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
>   	if (params->ret != 0) {
>   		__atomic_store_n(&params->ctrl_thread_status,
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index d25bdc98f9..d833a71e7a 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -716,7 +716,16 @@ malloc_get_numa_socket(void)
>   		if (conf->socket_mem[socket_id] != 0)
>   			return socket_id;
>   	}
> -
> +	/* We couldn't find quickly find a NUMA node where memory was available,

typo: `find quickly find`, should probably be `quickly find`

Can be fixed on apply.

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

> +	 * so fall back to using main lcore socket ID.
> +	 */
> +	socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
> +	/* Main lcore socket ID may be SOCKET_ID_ANY in cases when main lcore
> +	 * thread is affinitized to multiple NUMA nodes.
> +	 */
> +	if (socket_id != (unsigned int)SOCKET_ID_ANY)
> +		return socket_id;
> +	/* Failed to find meaningful socket ID, so just use the first one available */
>   	return rte_socket_id_by_idx(0);
>   }
>   

-- 
Thanks,
Anatoly

Re: [PATCH v8] enhance NUMA affinity heuristic

[Stephen Hemminger]

On Fri, 26 May 2023 15:44:15 +0100
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> > +	/* Set control thread socket ID to SOCKET_ID_ANY as control
> > +	 * threads may be scheduled on any NUMA node.
> > +	 */
> > +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;

This is not always true. Read the control thread documentation.
If DPDK application is run in a cgroup with cpuset, it maybe limited differently.

Re: [PATCH v8] enhance NUMA affinity heuristic

[Burakov, Anatoly]

On 5/26/2023 6:50 PM, Stephen Hemminger wrote:
> On Fri, 26 May 2023 15:44:15 +0100
> "Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:
> 
>>> +	/* Set control thread socket ID to SOCKET_ID_ANY as control
>>> +	 * threads may be scheduled on any NUMA node.
>>> +	 */
>>> +	RTE_PER_LCORE(_socket_id) = SOCKET_ID_ANY;
> 
> This is not always true. Read the control thread documentation.
> If DPDK application is run in a cgroup with cpuset, it maybe limited differently.

The point was more to highlight that control thread NUMA affinity is 
"undefined" (and depends on a lot of factors) rather than necessarily 
"uses all NUMA nodes". IMO the message is OK, even if technically it's 
not 100% accurate.

I mean, we could do some magic and figure out the effective NUMA node of 
a control thread, but do you think this would be worth the effort?

-- 
Thanks,
Anatoly

Re: [PATCH v8] enhance NUMA affinity heuristic

[David Marchand]

On Fri, May 26, 2023 at 11:03 AM Kaisen You <kaisenx.you@intel.com> wrote:
>
> When a DPDK application is started on only one numa node, memory is
> allocated for only one socket. When interrupt threads use memory,
> memory may not be found on the socket where the interrupt thread
> is currently located, and memory has to be reallocated on the hugepage,
> this operation will lead to performance degradation.
>
> Fixes: 705356f0811f ("eal: simplify control thread creation")
> Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
> Cc: stable@dpdk.org

Backporting this kind of change seems risky for a LTS.
Heads up for LTS maintainers.

Anatoly, are we sure we want it backported?


-- 
David Marchand

Re: [PATCH v8] enhance NUMA affinity heuristic

[Thomas Monjalon]

01/06/2023 16:42, David Marchand:
> On Fri, May 26, 2023 at 11:03 AM Kaisen You <kaisenx.you@intel.com> wrote:
> >
> > When a DPDK application is started on only one numa node, memory is
> > allocated for only one socket. When interrupt threads use memory,
> > memory may not be found on the socket where the interrupt thread
> > is currently located, and memory has to be reallocated on the hugepage,
> > this operation will lead to performance degradation.
> >
> > Fixes: 705356f0811f ("eal: simplify control thread creation")
> > Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
> > Cc: stable@dpdk.org
> 
> Backporting this kind of change seems risky for a LTS.
> Heads up for LTS maintainers.
> 
> Anatoly, are we sure we want it backported?

No answer, so I take on me to remove the backport request.

Applied with minor fix suggested by Anatoly.
Hope this patch will have no side effect.

Re: [PATCH v8] enhance NUMA affinity heuristic

[Burakov, Anatoly]

On 6/1/2023 3:42 PM, David Marchand wrote:
> On Fri, May 26, 2023 at 11:03 AM Kaisen You <kaisenx.you@intel.com> wrote:
>>
>> When a DPDK application is started on only one numa node, memory is
>> allocated for only one socket. When interrupt threads use memory,
>> memory may not be found on the socket where the interrupt thread
>> is currently located, and memory has to be reallocated on the hugepage,
>> this operation will lead to performance degradation.
>>
>> Fixes: 705356f0811f ("eal: simplify control thread creation")
>> Fixes: 770d41bf3309 ("malloc: fix allocation with unknown socket ID")
>> Cc: stable@dpdk.org
> 
> Backporting this kind of change seems risky for a LTS.
> Heads up for LTS maintainers.
> 
> Anatoly, are we sure we want it backported?
> 
> 

Yeah, apologies, I was away on leave. Yeah I'd err on the side of not 
backporting anything unless this comes up independently.

-- 
Thanks,
Anatoly