[dpdk-dev] [PATCH] net/null:Different mac address support

[dpdk-dev] [PATCH] net/null:Different mac address support

[Mallesh Koujalagi]

After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac
address for both null devices. Fix this issue, by setting different mac
address.

Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>
---
 drivers/net/null/rte_eth_null.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 9385ffd..98ac115 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -85,8 +85,17 @@ struct pmd_internals {
 	uint8_t rss_key[40];                /**< 40-byte hash key. */
 };
 
+static struct ether_addr base_eth_addr = {
+	.addr_bytes = {
+		0x4E /* N */,
+		0x55 /* U */,
+		0x4C /* L */,
+		0x4C /* L */,
+		0x00,
+		0x00
+	}
+};
 
-static struct ether_addr eth_addr = { .addr_bytes = {0} };
 static struct rte_eth_link pmd_link = {
 	.link_speed = ETH_SPEED_NUM_10G,
 	.link_duplex = ETH_LINK_FULL_DUPLEX,
@@ -492,6 +501,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 	struct rte_eth_dev_data *data = NULL;
 	struct pmd_internals *internals = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
+	struct ether_addr *eth_addr = NULL;
 
 	static const uint8_t default_rss_key[40] = {
 		0x6D, 0x5A, 0x56, 0xDA, 0x25, 0x5B, 0x0E, 0xC2, 0x41, 0x67, 0x25, 0x3D,
@@ -519,6 +529,15 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 		rte_free(data);
 		return -ENOMEM;
 	}
+	eth_addr = rte_zmalloc_socket(rte_vdev_device_name(dev),
+			sizeof(*eth_addr), 0, dev->device.numa_node);
+	if (eth_addr == NULL) {
+		rte_eth_dev_release_port(eth_dev);
+		rte_free(data);
+		return -ENOMEM;
+	}
+	*eth_addr = base_eth_addr;
+	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
 
 	/* now put it all together
 	 * - store queue data in internals,
@@ -543,7 +562,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 	data->nb_rx_queues = (uint16_t)nb_rx_queues;
 	data->nb_tx_queues = (uint16_t)nb_tx_queues;
 	data->dev_link = pmd_link;
-	data->mac_addrs = &eth_addr;
+	data->mac_addrs = eth_addr;
 
 	eth_dev->data = data;
 	eth_dev->dev_ops = &ops;
-- 
2.7.4

Re: [dpdk-dev] [PATCH] net/null:Different mac address support

[Ferruh Yigit]

On 2/3/2018 2:11 AM, Mallesh Koujalagi wrote:
> After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac
> address for both null devices. Fix this issue, by setting different mac
> address.
> 
> Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>
> ---
>  drivers/net/null/rte_eth_null.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
> index 9385ffd..98ac115 100644
> --- a/drivers/net/null/rte_eth_null.c
> +++ b/drivers/net/null/rte_eth_null.c
> @@ -85,8 +85,17 @@ struct pmd_internals {
>  	uint8_t rss_key[40];                /**< 40-byte hash key. */
>  };
>  
> +static struct ether_addr base_eth_addr = {
> +	.addr_bytes = {
> +		0x4E /* N */,
> +		0x55 /* U */,
> +		0x4C /* L */,
> +		0x4C /* L */,
> +		0x00,
> +		0x00
> +	}
> +};
>  
> -static struct ether_addr eth_addr = { .addr_bytes = {0} };
>  static struct rte_eth_link pmd_link = {
>  	.link_speed = ETH_SPEED_NUM_10G,
>  	.link_duplex = ETH_LINK_FULL_DUPLEX,
> @@ -492,6 +501,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
>  	struct rte_eth_dev_data *data = NULL;
>  	struct pmd_internals *internals = NULL;
>  	struct rte_eth_dev *eth_dev = NULL;
> +	struct ether_addr *eth_addr = NULL;
>  
>  	static const uint8_t default_rss_key[40] = {
>  		0x6D, 0x5A, 0x56, 0xDA, 0x25, 0x5B, 0x0E, 0xC2, 0x41, 0x67, 0x25, 0x3D,
> @@ -519,6 +529,15 @@ eth_dev_null_create(struct rte_vdev_device *dev,
>  		rte_free(data);
>  		return -ENOMEM;
>  	}
> +	eth_addr = rte_zmalloc_socket(rte_vdev_device_name(dev),
> +			sizeof(*eth_addr), 0, dev->device.numa_node);

Why not put "struct ether_addr" into "struct pmd_internals" as ring pmd does?
This saves from extra memory allocation and error recovery complexity.

> +	if (eth_addr == NULL) {
> +		rte_eth_dev_release_port(eth_dev);
> +		rte_free(data);

Need to free data->dev_private which has been allocated by rte_eth_vdev_allocate()

And note rte_eth_vdev_allocate() should be done after that since it memset the data.

Also needs to free eth_addr in rte_pmd_null_remove()

> +		return -ENOMEM;
> +	}
> +	*eth_addr = base_eth_addr;
> +	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
>  
>  	/* now put it all together
>  	 * - store queue data in internals,
> @@ -543,7 +562,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
>  	data->nb_rx_queues = (uint16_t)nb_rx_queues;
>  	data->nb_tx_queues = (uint16_t)nb_tx_queues;
>  	data->dev_link = pmd_link;
> -	data->mac_addrs = &eth_addr;
> +	data->mac_addrs = eth_addr;
>  
>  	eth_dev->data = data;
>  	eth_dev->dev_ops = &ops;
> 

[dpdk-dev] [PATCH v2] net/null:Different mac address support

[Mallesh Koujalagi]

After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac
address for both null devices. Fix this issue, by setting different mac
address.

Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>
---
 drivers/net/null/rte_eth_null.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 9385ffd..599b513 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -85,8 +85,17 @@ struct pmd_internals {
 	uint8_t rss_key[40];                /**< 40-byte hash key. */
 };
 
+static struct ether_addr base_eth_addr = {
+	.addr_bytes = {
+		0x4E /* N */,
+		0x55 /* U */,
+		0x4C /* L */,
+		0x4C /* L */,
+		0x00,
+		0x00
+	}
+};
 
-static struct ether_addr eth_addr = { .addr_bytes = {0} };
 static struct rte_eth_link pmd_link = {
 	.link_speed = ETH_SPEED_NUM_10G,
 	.link_duplex = ETH_LINK_FULL_DUPLEX,
@@ -492,6 +501,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 	struct rte_eth_dev_data *data = NULL;
 	struct pmd_internals *internals = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
+	struct ether_addr *eth_addr = NULL;
 
 	static const uint8_t default_rss_key[40] = {
 		0x6D, 0x5A, 0x56, 0xDA, 0x25, 0x5B, 0x0E, 0xC2, 0x41, 0x67, 0x25, 0x3D,
@@ -514,12 +524,21 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 	if (!data)
 		return -ENOMEM;
 
+	eth_addr = rte_zmalloc_socket(rte_vdev_device_name(dev),
+			sizeof(*eth_addr), 0, dev->device.numa_node);
+	if (eth_addr == NULL) {
+		rte_free(data);
+		return -ENOMEM;
+	}
+
 	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internals));
 	if (!eth_dev) {
+		rte_free(eth_addr);
 		rte_free(data);
 		return -ENOMEM;
 	}
-
+	*eth_addr = base_eth_addr;
+	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
 	/* now put it all together
 	 * - store queue data in internals,
 	 * - store numa_node info in ethdev data
@@ -543,7 +562,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 	data->nb_rx_queues = (uint16_t)nb_rx_queues;
 	data->nb_tx_queues = (uint16_t)nb_tx_queues;
 	data->dev_link = pmd_link;
-	data->mac_addrs = &eth_addr;
+	data->mac_addrs = eth_addr;
 
 	eth_dev->data = data;
 	eth_dev->dev_ops = &ops;
@@ -662,6 +681,7 @@ rte_pmd_null_remove(struct rte_vdev_device *dev)
 	if (eth_dev == NULL)
 		return -1;
 
+	rte_free(eth_dev->data->mac_addrs);
 	rte_free(eth_dev->data->dev_private);
 	rte_free(eth_dev->data);
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/null:Different mac address support

[Stephen Hemminger]

On Mon,  5 Mar 2018 19:35:14 -0800
Mallesh Koujalagi <malleshx.koujalagi@intel.com> wrote:

> After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac
> address for both null devices. Fix this issue, by setting different mac
> address.
> 
> Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>
> ---
>  drivers/net/null/rte_eth_null.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
> index 9385ffd..599b513 100644
> --- a/drivers/net/null/rte_eth_null.c
> +++ b/drivers/net/null/rte_eth_null.c
> @@ -85,8 +85,17 @@ struct pmd_internals {
>  	uint8_t rss_key[40];                /**< 40-byte hash key. */
>  };
>  
> +static struct ether_addr base_eth_addr = {
> +	.addr_bytes = {
> +		0x4E /* N */,
> +		0x55 /* U */,
> +		0x4C /* L */,
> +		0x4C /* L */,
> +		0x00,
> +		0x00
> +	}
> +};

Cute, but since first octets of Ethernet are the vendor id (OUI)
it might be confusing. At least 'N' is 4E which does not have
the group address (multicast) set; and it does have the local
admin address bit set.

You really should be using a random locally assigned value.

 
> -static struct ether_addr eth_addr = { .addr_bytes = {0} };
>  static struct rte_eth_link pmd_link = {
>  	.link_speed = ETH_SPEED_NUM_10G,
>  	.link_duplex = ETH_LINK_FULL_DUPLEX,
> @@ -492,6 +501,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
>  	struct rte_eth_dev_data *data = NULL;
>  	struct pmd_internals *internals = NULL;
>  	struct rte_eth_dev *eth_dev = NULL;
> +	struct ether_addr *eth_addr = NULL;
>  
>  	static const uint8_t default_rss_key[40] = {
>  		0x6D, 0x5A, 0x56, 0xDA, 0x25, 0x5B, 0x0E, 0xC2, 0x41, 0x67, 0x25, 0x3D,
> @@ -514,12 +524,21 @@ eth_dev_null_create(struct rte_vdev_device *dev,
>  	if (!data)
>  		return -ENOMEM;
>  
> +	eth_addr = rte_zmalloc_socket(rte_vdev_device_name(dev),
> +			sizeof(*eth_addr), 0, dev->device.numa_node);
> +	if (eth_addr == NULL) {
> +		rte_free(data);
> +		return -ENOMEM;
> +	}
> +
>  	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internals));
>  	if (!eth_dev) {
> +		rte_free(eth_addr);
>  		rte_free(data);
>  		return -ENOMEM;
>  	}
> -
> +	*eth_addr = base_eth_addr;
> +	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
>  	/* now put it all together
>  	 * - store queue data in internals,
>  	 * - store numa_node info in ethdev data
> @@ -543,7 +562,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
>  	data->nb_rx_queues = (uint16_t)nb_rx_queues;
>  	data->nb_tx_queues = (uint16_t)nb_tx_queues;
>  	data->dev_link = pmd_link;
> -	data->mac_addrs = &eth_addr;
> +	data->mac_addrs = eth_addr;
>  
>  	eth_dev->data = data;
>  	eth_dev->dev_ops = &ops;
> @@ -662,6 +681,7 @@ rte_pmd_null_remove(struct rte_vdev_device *dev)
>  	if (eth_dev == NULL)
>  		return -1;
>  
> +	rte_free(eth_dev->data->mac_addrs);
>  	rte_free(eth_dev->data->dev_private);
>  	rte_free(eth_dev->data);
>  

Re: [dpdk-dev] [PATCH v2] net/null:Different mac address support

[Ferruh Yigit]

On 3/6/2018 3:35 AM, Mallesh Koujalagi wrote:
> After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac
> address for both null devices. Fix this issue, by setting different mac
> address.
> 
> Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>

<...>

> @@ -514,12 +524,21 @@ eth_dev_null_create(struct rte_vdev_device *dev,
>  	if (!data)
>  		return -ENOMEM;
>  
> +	eth_addr = rte_zmalloc_socket(rte_vdev_device_name(dev),
> +			sizeof(*eth_addr), 0, dev->device.numa_node);
> +	if (eth_addr == NULL) {
> +		rte_free(data);
> +		return -ENOMEM;
> +	}
> +
>  	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internals));
>  	if (!eth_dev) {
> +		rte_free(eth_addr);
>  		rte_free(data);
>  		return -ENOMEM;
>  	}

Same comment from previous version, why not put "eth_addr" inside "struct
pmd_internals"?

"struct pmd_internals" is already allocated/freed in the code, so you don't need
to manage "eth_addr" if you put it into "struct pmd_internals" it will come free.

<...>

[dpdk-dev] [PATCH v3] net/null:Different mac address support

[Mallesh Koujalagi]

After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac
address for both null devices. Fix this issue, by setting different mac
address.

Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>
---
 drivers/net/null/rte_eth_null.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 9385ffd..42e3a77 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -73,6 +73,7 @@ struct pmd_internals {
 	struct null_queue rx_null_queues[RTE_MAX_QUEUES_PER_PORT];
 	struct null_queue tx_null_queues[RTE_MAX_QUEUES_PER_PORT];
 
+	struct ether_addr eth_addr;
 	/** Bit mask of RSS offloads, the bit offset also means flow type */
 	uint64_t flow_type_rss_offloads;
 
@@ -84,9 +85,6 @@ struct pmd_internals {
 
 	uint8_t rss_key[40];                /**< 40-byte hash key. */
 };
-
-
-static struct ether_addr eth_addr = { .addr_bytes = {0} };
 static struct rte_eth_link pmd_link = {
 	.link_speed = ETH_SPEED_NUM_10G,
 	.link_duplex = ETH_LINK_FULL_DUPLEX,
@@ -519,7 +517,6 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 		rte_free(data);
 		return -ENOMEM;
 	}
-
 	/* now put it all together
 	 * - store queue data in internals,
 	 * - store numa_node info in ethdev data
@@ -533,6 +530,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 	internals->packet_size = packet_size;
 	internals->packet_copy = packet_copy;
 	internals->port_id = eth_dev->data->port_id;
+	eth_random_addr(internals->eth_addr.addr_bytes);
 
 	internals->flow_type_rss_offloads =  ETH_RSS_PROTO_MASK;
 	internals->reta_size = RTE_DIM(internals->reta_conf) * RTE_RETA_GROUP_SIZE;
@@ -543,7 +541,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 	data->nb_rx_queues = (uint16_t)nb_rx_queues;
 	data->nb_tx_queues = (uint16_t)nb_tx_queues;
 	data->dev_link = pmd_link;
-	data->mac_addrs = &eth_addr;
+	data->mac_addrs = &internals->eth_addr;
 
 	eth_dev->data = data;
 	eth_dev->dev_ops = &ops;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] net/null:Different mac address support

[Ferruh Yigit]

On 3/7/2018 3:31 AM, Mallesh Koujalagi wrote:
> After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac
> address for both null devices. Fix this issue, by setting different mac
> address.
> 
> Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

There are some commit formatting issues which I can fix while applying for this
one, but for future can you please run "./devtools/check-git-log.sh" before
sending patches.

Re: [dpdk-dev] [PATCH v3] net/null:Different mac address support

[Koujalagi, MalleshX]

Sure!! Thanks Ferruh.

-----Original Message-----
From: Yigit, Ferruh 
Sent: Wednesday, March 7, 2018 2:45 AM
To: Koujalagi, MalleshX <malleshx.koujalagi@intel.com>; dev@dpdk.org
Cc: mtetsuyah@gmail.com; stephen@networkplumber.org
Subject: Re: [PATCH v3] net/null:Different mac address support

On 3/7/2018 3:31 AM, Mallesh Koujalagi wrote:
> After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac 
> address for both null devices. Fix this issue, by setting different 
> mac address.
> 
> Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

There are some commit formatting issues which I can fix while applying for this one, but for future can you please run "./devtools/check-git-log.sh" before sending patches.

Re: [dpdk-dev] [PATCH v3] net/null:Different mac address support

[Ferruh Yigit]

On 3/7/2018 10:45 AM, Ferruh Yigit wrote:
> On 3/7/2018 3:31 AM, Mallesh Koujalagi wrote:
>> After attaching two Null device to ovs, seeing "00.00.00.00.00.00" mac
>> address for both null devices. Fix this issue, by setting different mac
>> address.
>>
>> Signed-off-by: Mallesh Koujalagi <malleshx.koujalagi@intel.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.