DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
@ 2015-01-13  9:22 Cian Ferriter
  2015-01-13  9:23 ` Ferriter, Cian
  2015-01-13 23:24 ` Stephen Hemminger
  0 siblings, 2 replies; 12+ messages in thread
From: Cian Ferriter @ 2015-01-13  9:22 UTC (permalink / raw)
  To: dev

Change the socket id that is passed to rte_memzone_reserve from
the socket id of current logical core to the socket id of the
master_lcore.
---
 lib/librte_ether/rte_ethdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
 mode change 100644 => 100755 lib/librte_ether/rte_ethdev.c

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
old mode 100644
new mode 100755
index 95f2ceb..835540d
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
 		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
 				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
-				rte_socket_id(), flags);
+				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
 	} else
 		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
 	if (mz == NULL)
-- 
1.7.4.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-13  9:22 [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve Cian Ferriter
@ 2015-01-13  9:23 ` Ferriter, Cian
  2015-01-13 13:55   ` Bruce Richardson
  2015-01-13 23:24 ` Stephen Hemminger
  1 sibling, 1 reply; 12+ messages in thread
From: Ferriter, Cian @ 2015-01-13  9:23 UTC (permalink / raw)
  To: dev

Passing a socket id of "rte_socket_id()" can cause problems in non DPDK applications as there is a dependency on the current logical core we are running on.
Passing " rte_lcore_to_socket_id(rte_get_master_lcore())" as the socket id to rte_memzone_reserve resolves these issues as the master lcore doesn't change.

-----Original Message-----
From: Ferriter, Cian 
Sent: Tuesday, January 13, 2015 9:22 AM
To: dev@dpdk.org
Cc: Ferriter, Cian
Subject: [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve

Change the socket id that is passed to rte_memzone_reserve from the socket id of current logical core to the socket id of the master_lcore.
---
 lib/librte_ether/rte_ethdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)  mode change 100644 => 100755 lib/librte_ether/rte_ethdev.c

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c old mode 100644 new mode 100755 index 95f2ceb..835540d
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
 		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
 				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
-				rte_socket_id(), flags);
+				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
 	} else
 		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
 	if (mz == NULL)
--
1.7.4.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-13  9:23 ` Ferriter, Cian
@ 2015-01-13 13:55   ` Bruce Richardson
  2015-01-13 18:05     ` Ferriter, Cian
  0 siblings, 1 reply; 12+ messages in thread
From: Bruce Richardson @ 2015-01-13 13:55 UTC (permalink / raw)
  To: Ferriter, Cian; +Cc: dev

On Tue, Jan 13, 2015 at 09:23:16AM +0000, Ferriter, Cian wrote:
> Passing a socket id of "rte_socket_id()" can cause problems in non DPDK applications as there is a dependency on the current logical core we are running on.
> Passing " rte_lcore_to_socket_id(rte_get_master_lcore())" as the socket id to rte_memzone_reserve resolves these issues as the master lcore doesn't change.
> 

The only trouble is that when affinitizing the memory for the NICs to the socket
of the master lcore, it gives us no way to correctly configure an app
to use NICs connected to two different sockets on the one system. All memory for
all NICs will end up on the same socket. Two possible alternative solutions:
1) affinitize memory to the socket the NIC is connected to
2) add a socket parameter to the API calls to allow the user complete control
over their memory allocations

Obviously the second one breaks backward compatibility (assume we modify existing
API call), but is more powerful.

Thoughts?

/Bruce

> -----Original Message-----
> From: Ferriter, Cian 
> Sent: Tuesday, January 13, 2015 9:22 AM
> To: dev@dpdk.org
> Cc: Ferriter, Cian
> Subject: [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
> 
> Change the socket id that is passed to rte_memzone_reserve from the socket id of current logical core to the socket id of the master_lcore.
> ---
>  lib/librte_ether/rte_ethdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)  mode change 100644 => 100755 lib/librte_ether/rte_ethdev.c
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c old mode 100644 new mode 100755 index 95f2ceb..835540d
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
>  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
>  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> -				rte_socket_id(), flags);
> +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
>  	} else
>  		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
>  	if (mz == NULL)
> --
> 1.7.4.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-13 13:55   ` Bruce Richardson
@ 2015-01-13 18:05     ` Ferriter, Cian
  2015-01-14 10:09       ` Bruce Richardson
  0 siblings, 1 reply; 12+ messages in thread
From: Ferriter, Cian @ 2015-01-13 18:05 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: dev

Comments on alternative solutions:
1) how would this solution work when there is no NIC present, and "rte_eth_from_rings" is called? Here, could you have an else where the socket id of the master core is passed to the "memzone_reserve"?
2) how would you advise making this change? I have looked at where "rte_eth_dev_allocate" is being called and in all but one case, there is a "numa_id" that could be passed in. This isn't the case for " rte_eth_dev_init" however, is there an easy solution for this? Would there now need to be an "rte_eth_dev_data" struct for each socket that there is a NIC attached to, reserving memory from that socket?

Cian

-----Original Message-----
From: Richardson, Bruce 
Sent: Tuesday, January 13, 2015 1:56 PM
To: Ferriter, Cian
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve

On Tue, Jan 13, 2015 at 09:23:16AM +0000, Ferriter, Cian wrote:
> Passing a socket id of "rte_socket_id()" can cause problems in non DPDK applications as there is a dependency on the current logical core we are running on.
> Passing " rte_lcore_to_socket_id(rte_get_master_lcore())" as the socket id to rte_memzone_reserve resolves these issues as the master lcore doesn't change.
> 

The only trouble is that when affinitizing the memory for the NICs to the socket of the master lcore, it gives us no way to correctly configure an app to use NICs connected to two different sockets on the one system. All memory for all NICs will end up on the same socket. Two possible alternative solutions:
1) affinitize memory to the socket the NIC is connected to
2) add a socket parameter to the API calls to allow the user complete control over their memory allocations

Obviously the second one breaks backward compatibility (assume we modify existing API call), but is more powerful.

Thoughts?

/Bruce

> -----Original Message-----
> From: Ferriter, Cian
> Sent: Tuesday, January 13, 2015 9:22 AM
> To: dev@dpdk.org
> Cc: Ferriter, Cian
> Subject: [PATCH] lib/librte_ether: change socket_id passed to 
> rte_memzone_reserve
> 
> Change the socket id that is passed to rte_memzone_reserve from the socket id of current logical core to the socket id of the master_lcore.
> ---
>  lib/librte_ether/rte_ethdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)  mode change 100644 
> => 100755 lib/librte_ether/rte_ethdev.c
> 
> diff --git a/lib/librte_ether/rte_ethdev.c 
> b/lib/librte_ether/rte_ethdev.c old mode 100644 new mode 100755 index 
> 95f2ceb..835540d
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
>  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
>  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> -				rte_socket_id(), flags);
> +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
>  	} else
>  		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
>  	if (mz == NULL)
> --
> 1.7.4.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-13  9:22 [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve Cian Ferriter
  2015-01-13  9:23 ` Ferriter, Cian
@ 2015-01-13 23:24 ` Stephen Hemminger
  2015-01-14 10:13   ` Bruce Richardson
  1 sibling, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2015-01-13 23:24 UTC (permalink / raw)
  To: Cian Ferriter; +Cc: dev

On Tue, 13 Jan 2015 09:22:00 +0000
Cian Ferriter <cian.ferriter@intel.com> wrote:

> Change the socket id that is passed to rte_memzone_reserve from
> the socket id of current logical core to the socket id of the
> master_lcore.
> ---
>  lib/librte_ether/rte_ethdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>  mode change 100644 => 100755 lib/librte_ether/rte_ethdev.c
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> old mode 100644
> new mode 100755
> index 95f2ceb..835540d
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
>  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
>  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> -				rte_socket_id(), flags);
> +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
>  	} else
>  		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
>  	if (mz == NULL)


Why is this a memzone at all?
Seems like it should be allocated on a per-device basis on the same NUMA node
of the device. Probably with rte_malloc_socket().

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-13 18:05     ` Ferriter, Cian
@ 2015-01-14 10:09       ` Bruce Richardson
  2015-01-19 18:38         ` Ferriter, Cian
  0 siblings, 1 reply; 12+ messages in thread
From: Bruce Richardson @ 2015-01-14 10:09 UTC (permalink / raw)
  To: Ferriter, Cian; +Cc: dev

On Tue, Jan 13, 2015 at 06:05:25PM +0000, Ferriter, Cian wrote:
> Comments on alternative solutions:
> 1) how would this solution work when there is no NIC present, and "rte_eth_from_rings" is called? Here, could you have an else where the socket id of the master core is passed to the "memzone_reserve"?
> 2) how would you advise making this change? I have looked at where "rte_eth_dev_allocate" is being called and in all but one case, there is a "numa_id" that could be passed in. This isn't the case for " rte_eth_dev_init" however, is there an easy solution for this? Would there now need to be an "rte_eth_dev_data" struct for each socket that there is a NIC attached to, reserving memory from that socket?
> 
> Cian

While I think the issues you highlight can probably be overcome, I'm not so
sure any more how much it matters what numa node this is allocated on. The 
ethdev data for any port in use by a port should be in the cache. In that case,
if it doesn't matter, your original suggestion would work fine.

	/Bruce

> 
> -----Original Message-----
> From: Richardson, Bruce 
> Sent: Tuesday, January 13, 2015 1:56 PM
> To: Ferriter, Cian
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
> 
> On Tue, Jan 13, 2015 at 09:23:16AM +0000, Ferriter, Cian wrote:
> > Passing a socket id of "rte_socket_id()" can cause problems in non DPDK applications as there is a dependency on the current logical core we are running on.
> > Passing " rte_lcore_to_socket_id(rte_get_master_lcore())" as the socket id to rte_memzone_reserve resolves these issues as the master lcore doesn't change.
> > 
> 
> The only trouble is that when affinitizing the memory for the NICs to the socket of the master lcore, it gives us no way to correctly configure an app to use NICs connected to two different sockets on the one system. All memory for all NICs will end up on the same socket. Two possible alternative solutions:
> 1) affinitize memory to the socket the NIC is connected to
> 2) add a socket parameter to the API calls to allow the user complete control over their memory allocations
> 
> Obviously the second one breaks backward compatibility (assume we modify existing API call), but is more powerful.
> 
> Thoughts?
> 
> /Bruce
> 
> > -----Original Message-----
> > From: Ferriter, Cian
> > Sent: Tuesday, January 13, 2015 9:22 AM
> > To: dev@dpdk.org
> > Cc: Ferriter, Cian
> > Subject: [PATCH] lib/librte_ether: change socket_id passed to 
> > rte_memzone_reserve
> > 
> > Change the socket id that is passed to rte_memzone_reserve from the socket id of current logical core to the socket id of the master_lcore.
> > ---
> >  lib/librte_ether/rte_ethdev.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)  mode change 100644 
> > => 100755 lib/librte_ether/rte_ethdev.c
> > 
> > diff --git a/lib/librte_ether/rte_ethdev.c 
> > b/lib/librte_ether/rte_ethdev.c old mode 100644 new mode 100755 index 
> > 95f2ceb..835540d
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
> >  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
> >  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
> >  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> > -				rte_socket_id(), flags);
> > +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
> >  	} else
> >  		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
> >  	if (mz == NULL)
> > --
> > 1.7.4.1
> > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-13 23:24 ` Stephen Hemminger
@ 2015-01-14 10:13   ` Bruce Richardson
  0 siblings, 0 replies; 12+ messages in thread
From: Bruce Richardson @ 2015-01-14 10:13 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On Tue, Jan 13, 2015 at 03:24:15PM -0800, Stephen Hemminger wrote:
> On Tue, 13 Jan 2015 09:22:00 +0000
> Cian Ferriter <cian.ferriter@intel.com> wrote:
> 
> > Change the socket id that is passed to rte_memzone_reserve from
> > the socket id of current logical core to the socket id of the
> > master_lcore.
> > ---
> >  lib/librte_ether/rte_ethdev.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >  mode change 100644 => 100755 lib/librte_ether/rte_ethdev.c
> > 
> > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > old mode 100644
> > new mode 100755
> > index 95f2ceb..835540d
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
> >  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
> >  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
> >  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> > -				rte_socket_id(), flags);
> > +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
> >  	} else
> >  		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
> >  	if (mz == NULL)
> 
> 
> Why is this a memzone at all?
> Seems like it should be allocated on a per-device basis on the same NUMA node
> of the device. Probably with rte_malloc_socket().
> 
You can't look up a malloced area of memory in a secondary process, since it
doesn't have a name.
Question is: for normal apps, does the eth_dev_data ever drop out of cache? If
not, the numa node used for memory doesn't matter.

/Bruce

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-14 10:09       ` Bruce Richardson
@ 2015-01-19 18:38         ` Ferriter, Cian
  2015-01-22  9:56           ` Ferriter, Cian
  0 siblings, 1 reply; 12+ messages in thread
From: Ferriter, Cian @ 2015-01-19 18:38 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: dev

I would be happy with the original suggestion. If the ethdev data for a port in use is in cache it removes the performance concern associated the current setup and my fix. The original suggestion also fixes the crash that I was seeing because of memory being reserved from a numa node with no "--socket-mem" allocated.

Cian

-----Original Message-----
From: Richardson, Bruce 
Sent: Wednesday, January 14, 2015 10:10 AM
To: Ferriter, Cian
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve

On Tue, Jan 13, 2015 at 06:05:25PM +0000, Ferriter, Cian wrote:
> Comments on alternative solutions:
> 1) how would this solution work when there is no NIC present, and "rte_eth_from_rings" is called? Here, could you have an else where the socket id of the master core is passed to the "memzone_reserve"?
> 2) how would you advise making this change? I have looked at where "rte_eth_dev_allocate" is being called and in all but one case, there is a "numa_id" that could be passed in. This isn't the case for " rte_eth_dev_init" however, is there an easy solution for this? Would there now need to be an "rte_eth_dev_data" struct for each socket that there is a NIC attached to, reserving memory from that socket?
> 
> Cian

While I think the issues you highlight can probably be overcome, I'm not so sure any more how much it matters what numa node this is allocated on. The ethdev data for any port in use by a port should be in the cache. In that case, if it doesn't matter, your original suggestion would work fine.

	/Bruce

> 
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Tuesday, January 13, 2015 1:56 PM
> To: Ferriter, Cian
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id 
> passed to rte_memzone_reserve
> 
> On Tue, Jan 13, 2015 at 09:23:16AM +0000, Ferriter, Cian wrote:
> > Passing a socket id of "rte_socket_id()" can cause problems in non DPDK applications as there is a dependency on the current logical core we are running on.
> > Passing " rte_lcore_to_socket_id(rte_get_master_lcore())" as the socket id to rte_memzone_reserve resolves these issues as the master lcore doesn't change.
> > 
> 
> The only trouble is that when affinitizing the memory for the NICs to the socket of the master lcore, it gives us no way to correctly configure an app to use NICs connected to two different sockets on the one system. All memory for all NICs will end up on the same socket. Two possible alternative solutions:
> 1) affinitize memory to the socket the NIC is connected to
> 2) add a socket parameter to the API calls to allow the user complete 
> control over their memory allocations
> 
> Obviously the second one breaks backward compatibility (assume we modify existing API call), but is more powerful.
> 
> Thoughts?
> 
> /Bruce
> 
> > -----Original Message-----
> > From: Ferriter, Cian
> > Sent: Tuesday, January 13, 2015 9:22 AM
> > To: dev@dpdk.org
> > Cc: Ferriter, Cian
> > Subject: [PATCH] lib/librte_ether: change socket_id passed to 
> > rte_memzone_reserve
> > 
> > Change the socket id that is passed to rte_memzone_reserve from the socket id of current logical core to the socket id of the master_lcore.
> > ---
> >  lib/librte_ether/rte_ethdev.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)  mode change 
> > 100644 => 100755 lib/librte_ether/rte_ethdev.c
> > 
> > diff --git a/lib/librte_ether/rte_ethdev.c 
> > b/lib/librte_ether/rte_ethdev.c old mode 100644 new mode 100755 
> > index 95f2ceb..835540d
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
> >  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
> >  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
> >  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> > -				rte_socket_id(), flags);
> > +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
> >  	} else
> >  		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
> >  	if (mz == NULL)
> > --
> > 1.7.4.1
> > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-19 18:38         ` Ferriter, Cian
@ 2015-01-22  9:56           ` Ferriter, Cian
  2015-01-22 11:26             ` Bruce Richardson
  0 siblings, 1 reply; 12+ messages in thread
From: Ferriter, Cian @ 2015-01-22  9:56 UTC (permalink / raw)
  To: Richardson, Bruce, dev

Hey guys,

I just wanted to ask is there anything more that can be done with this patch or is it in an acceptable state for pushing?

Cian

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferriter, Cian
Sent: Monday, January 19, 2015 6:39 PM
To: Richardson, Bruce
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve

I would be happy with the original suggestion. If the ethdev data for a port in use is in cache it removes the performance concern associated the current setup and my fix. The original suggestion also fixes the crash that I was seeing because of memory being reserved from a numa node with no "--socket-mem" allocated.

Cian

-----Original Message-----
From: Richardson, Bruce
Sent: Wednesday, January 14, 2015 10:10 AM
To: Ferriter, Cian
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve

On Tue, Jan 13, 2015 at 06:05:25PM +0000, Ferriter, Cian wrote:
> Comments on alternative solutions:
> 1) how would this solution work when there is no NIC present, and "rte_eth_from_rings" is called? Here, could you have an else where the socket id of the master core is passed to the "memzone_reserve"?
> 2) how would you advise making this change? I have looked at where "rte_eth_dev_allocate" is being called and in all but one case, there is a "numa_id" that could be passed in. This isn't the case for " rte_eth_dev_init" however, is there an easy solution for this? Would there now need to be an "rte_eth_dev_data" struct for each socket that there is a NIC attached to, reserving memory from that socket?
> 
> Cian

While I think the issues you highlight can probably be overcome, I'm not so sure any more how much it matters what numa node this is allocated on. The ethdev data for any port in use by a port should be in the cache. In that case, if it doesn't matter, your original suggestion would work fine.

	/Bruce

> 
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Tuesday, January 13, 2015 1:56 PM
> To: Ferriter, Cian
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id 
> passed to rte_memzone_reserve
> 
> On Tue, Jan 13, 2015 at 09:23:16AM +0000, Ferriter, Cian wrote:
> > Passing a socket id of "rte_socket_id()" can cause problems in non DPDK applications as there is a dependency on the current logical core we are running on.
> > Passing " rte_lcore_to_socket_id(rte_get_master_lcore())" as the socket id to rte_memzone_reserve resolves these issues as the master lcore doesn't change.
> > 
> 
> The only trouble is that when affinitizing the memory for the NICs to the socket of the master lcore, it gives us no way to correctly configure an app to use NICs connected to two different sockets on the one system. All memory for all NICs will end up on the same socket. Two possible alternative solutions:
> 1) affinitize memory to the socket the NIC is connected to
> 2) add a socket parameter to the API calls to allow the user complete 
> control over their memory allocations
> 
> Obviously the second one breaks backward compatibility (assume we modify existing API call), but is more powerful.
> 
> Thoughts?
> 
> /Bruce
> 
> > -----Original Message-----
> > From: Ferriter, Cian
> > Sent: Tuesday, January 13, 2015 9:22 AM
> > To: dev@dpdk.org
> > Cc: Ferriter, Cian
> > Subject: [PATCH] lib/librte_ether: change socket_id passed to 
> > rte_memzone_reserve
> > 
> > Change the socket id that is passed to rte_memzone_reserve from the socket id of current logical core to the socket id of the master_lcore.
> > ---
> >  lib/librte_ether/rte_ethdev.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)  mode change
> > 100644 => 100755 lib/librte_ether/rte_ethdev.c
> > 
> > diff --git a/lib/librte_ether/rte_ethdev.c 
> > b/lib/librte_ether/rte_ethdev.c old mode 100644 new mode 100755 
> > index 95f2ceb..835540d
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
> >  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
> >  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
> >  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> > -				rte_socket_id(), flags);
> > +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
> >  	} else
> >  		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
> >  	if (mz == NULL)
> > --
> > 1.7.4.1
> > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-22  9:56           ` Ferriter, Cian
@ 2015-01-22 11:26             ` Bruce Richardson
  0 siblings, 0 replies; 12+ messages in thread
From: Bruce Richardson @ 2015-01-22 11:26 UTC (permalink / raw)
  To: Ferriter, Cian; +Cc: dev

On Thu, Jan 22, 2015 at 09:56:48AM +0000, Ferriter, Cian wrote:
> Hey guys,
> 
> I just wanted to ask is there anything more that can be done with this patch or is it in an acceptable state for pushing?
> 
> Cian

At this stage I think I'm ok with the patch contents, unless anyone else objects.
However, your patch submission is missing the sign-off line needed before it
can be committed. Can you please resubmit with the proper sign-off. [See http://www.dpdk.org/dev]

Regards,
/Bruce

> 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferriter, Cian
> Sent: Monday, January 19, 2015 6:39 PM
> To: Richardson, Bruce
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
> 
> I would be happy with the original suggestion. If the ethdev data for a port in use is in cache it removes the performance concern associated the current setup and my fix. The original suggestion also fixes the crash that I was seeing because of memory being reserved from a numa node with no "--socket-mem" allocated.
> 
> Cian
> 
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, January 14, 2015 10:10 AM
> To: Ferriter, Cian
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
> 
> On Tue, Jan 13, 2015 at 06:05:25PM +0000, Ferriter, Cian wrote:
> > Comments on alternative solutions:
> > 1) how would this solution work when there is no NIC present, and "rte_eth_from_rings" is called? Here, could you have an else where the socket id of the master core is passed to the "memzone_reserve"?
> > 2) how would you advise making this change? I have looked at where "rte_eth_dev_allocate" is being called and in all but one case, there is a "numa_id" that could be passed in. This isn't the case for " rte_eth_dev_init" however, is there an easy solution for this? Would there now need to be an "rte_eth_dev_data" struct for each socket that there is a NIC attached to, reserving memory from that socket?
> > 
> > Cian
> 
> While I think the issues you highlight can probably be overcome, I'm not so sure any more how much it matters what numa node this is allocated on. The ethdev data for any port in use by a port should be in the cache. In that case, if it doesn't matter, your original suggestion would work fine.
> 
> 	/Bruce
> 
> > 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Tuesday, January 13, 2015 1:56 PM
> > To: Ferriter, Cian
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id 
> > passed to rte_memzone_reserve
> > 
> > On Tue, Jan 13, 2015 at 09:23:16AM +0000, Ferriter, Cian wrote:
> > > Passing a socket id of "rte_socket_id()" can cause problems in non DPDK applications as there is a dependency on the current logical core we are running on.
> > > Passing " rte_lcore_to_socket_id(rte_get_master_lcore())" as the socket id to rte_memzone_reserve resolves these issues as the master lcore doesn't change.
> > > 
> > 
> > The only trouble is that when affinitizing the memory for the NICs to the socket of the master lcore, it gives us no way to correctly configure an app to use NICs connected to two different sockets on the one system. All memory for all NICs will end up on the same socket. Two possible alternative solutions:
> > 1) affinitize memory to the socket the NIC is connected to
> > 2) add a socket parameter to the API calls to allow the user complete 
> > control over their memory allocations
> > 
> > Obviously the second one breaks backward compatibility (assume we modify existing API call), but is more powerful.
> > 
> > Thoughts?
> > 
> > /Bruce
> > 
> > > -----Original Message-----
> > > From: Ferriter, Cian
> > > Sent: Tuesday, January 13, 2015 9:22 AM
> > > To: dev@dpdk.org
> > > Cc: Ferriter, Cian
> > > Subject: [PATCH] lib/librte_ether: change socket_id passed to 
> > > rte_memzone_reserve
> > > 
> > > Change the socket id that is passed to rte_memzone_reserve from the socket id of current logical core to the socket id of the master_lcore.
> > > ---
> > >  lib/librte_ether/rte_ethdev.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)  mode change
> > > 100644 => 100755 lib/librte_ether/rte_ethdev.c
> > > 
> > > diff --git a/lib/librte_ether/rte_ethdev.c 
> > > b/lib/librte_ether/rte_ethdev.c old mode 100644 new mode 100755 
> > > index 95f2ceb..835540d
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
> > >  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
> > >  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
> > >  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> > > -				rte_socket_id(), flags);
> > > +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
> > >  	} else
> > >  		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
> > >  	if (mz == NULL)
> > > --
> > > 1.7.4.1
> > > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
  2015-01-22 15:05 Cian Ferriter
@ 2015-01-27  9:29 ` Thomas Monjalon
  0 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2015-01-27  9:29 UTC (permalink / raw)
  To: Cian Ferriter; +Cc: dev

Hi,

2015-01-22 15:05, Cian Ferriter:
> Removes the dependency that this memzone reserve has on the
> socket currently running on. Following the socket of the master
> core will yield more predictable results when calling this
> function after initialisation.

You don't describe what is the problem. In another mail, you say
"The original suggestion also fixes the crash that I was seeing because
of memory being reserved from a numa node with no "--socket-mem" allocated."
Please describe it clearly in the commit log.

You should also explain what this rte_memzone_reserve() is for,
and what are the incidences of your changes.

Thanks
-- 
Thomas

> @@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
>  		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
>  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
> -				rte_socket_id(), flags);
> +				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve
@ 2015-01-22 15:05 Cian Ferriter
  2015-01-27  9:29 ` Thomas Monjalon
  0 siblings, 1 reply; 12+ messages in thread
From: Cian Ferriter @ 2015-01-22 15:05 UTC (permalink / raw)
  To: dev

Removes the dependency that this memzone reserve has on the
socket currently running on. Following the socket of the master
core will yield more predictable results when calling this
function after initialisation.

Signed-off-by: Cian Ferriter <cian.ferriter@intel.com>
Reviewed-by: Maryam Tahhan <maryam.tahhan@intel.com>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_ether/rte_ethdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
 mode change 100644 => 100755 lib/librte_ether/rte_ethdev.c

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
old mode 100644
new mode 100755
index ea3a1fb..088bffc
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -184,7 +184,7 @@ rte_eth_dev_data_alloc(void)
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY){
 		mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
 				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data),
-				rte_socket_id(), flags);
+				rte_lcore_to_socket_id(rte_get_master_lcore()), flags);
 	} else
 		mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
 	if (mz == NULL)
-- 
1.7.4.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-01-27  9:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-13  9:22 [dpdk-dev] [PATCH] lib/librte_ether: change socket_id passed to rte_memzone_reserve Cian Ferriter
2015-01-13  9:23 ` Ferriter, Cian
2015-01-13 13:55   ` Bruce Richardson
2015-01-13 18:05     ` Ferriter, Cian
2015-01-14 10:09       ` Bruce Richardson
2015-01-19 18:38         ` Ferriter, Cian
2015-01-22  9:56           ` Ferriter, Cian
2015-01-22 11:26             ` Bruce Richardson
2015-01-13 23:24 ` Stephen Hemminger
2015-01-14 10:13   ` Bruce Richardson
2015-01-22 15:05 Cian Ferriter
2015-01-27  9:29 ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).