DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] i40e: configure MTU
@ 2016-04-23 11:26 Beilei Xing
  2016-04-26  2:00 ` Wu, Jingjing
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Beilei Xing @ 2016-04-23 11:26 UTC (permalink / raw)
  To: jingjing.wu; +Cc: dev, Beilei Xing

This patch enables configuring MTU for i40e.
Since changing MTU needs to reconfigure queue, stop port first
before configuring MTU.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 49 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index bc28d3c..29259b9 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -447,6 +447,8 @@ static int i40e_get_eeprom(struct rte_eth_dev *dev,
 static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 				      struct ether_addr *mac_addr);
 
+static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
+
 static const struct rte_pci_id pci_id_i40e_map[] = {
 #define RTE_PCI_DEV_ID_DECL_I40E(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
 #include "rte_pci_dev_ids.h"
@@ -520,6 +522,7 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.get_eeprom_length            = i40e_get_eeprom_length,
 	.get_eeprom                   = i40e_get_eeprom,
 	.mac_addr_set                 = i40e_set_default_mac_addr,
+	.mtu_set                      = i40e_dev_mtu_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -9104,3 +9107,49 @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 	/* Flags: 0x3 updates port address */
 	i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes, NULL);
 }
+
+static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
+{
+	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct rte_eth_dev_data *dev_data = pf->dev_data;
+	struct rte_eth_dev_info dev_info;
+	struct i40e_rx_queue *rxq;
+	int i;
+	uint16_t len;
+	uint32_t frame_size = mtu + ETHER_HDR_LEN + ETHER_CRC_LEN;
+	int ret = 0;
+
+	i40e_dev_info_get(dev, &dev_info);
+
+	/* check if mtu is within the allowed range */
+	if ((mtu < ETHER_MIN_MTU) || (frame_size > dev_info.max_rx_pktlen))
+		return -EINVAL;
+
+	/* mtu setting is forbidden if port is start */
+	if (dev_data->dev_started) {
+		PMD_DRV_LOG(ERR,
+			    "port %d must be stopped before configuration\n",
+			    dev_data->port_id);
+		return -EBUSY;
+	}
+
+	if (frame_size > ETHER_MAX_LEN)
+		dev_data->dev_conf.rxmode.jumbo_frame = 1;
+	else
+		dev_data->dev_conf.rxmode.jumbo_frame = 0;
+
+	for (i = 0; i < dev_data->nb_rx_queues; i++) {
+		rxq = dev_data->rx_queues[i];
+		if (!rxq || !rxq->q_set)
+			continue;
+
+		dev_data->dev_conf.rxmode.max_rx_pkt_len = frame_size;
+		len = hw->func_caps.rx_buf_chain_len * rxq->rx_buf_len;
+		rxq->max_pkt_len = RTE_MIN(len, frame_size);
+	}
+
+	ret = i40e_dev_rx_init(pf);
+
+	return ret;
+}
-- 
2.5.0

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

* Re: [dpdk-dev] [PATCH] i40e: configure MTU
  2016-04-23 11:26 [dpdk-dev] [PATCH] i40e: configure MTU Beilei Xing
@ 2016-04-26  2:00 ` Wu, Jingjing
  2016-04-27 11:43 ` Julien Meunier
  2016-04-28  3:19 ` [dpdk-dev] [PATCH V2] " Beilei Xing
  2 siblings, 0 replies; 22+ messages in thread
From: Wu, Jingjing @ 2016-04-26  2:00 UTC (permalink / raw)
  To: Xing, Beilei; +Cc: dev



On 4/23/2016 7:26 PM, Xing, Beilei wrote:
> This patch enables configuring MTU for i40e.
> Since changing MTU needs to reconfigure queue, stop port first
> before configuring MTU.
>
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
>   drivers/net/i40e/i40e_ethdev.c | 49 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index bc28d3c..29259b9 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -447,6 +447,8 @@ static int i40e_get_eeprom(struct rte_eth_dev *dev,
>   static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
>   				      struct ether_addr *mac_addr);
>   
> +static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
> +
>   static const struct rte_pci_id pci_id_i40e_map[] = {
>   #define RTE_PCI_DEV_ID_DECL_I40E(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
>   #include "rte_pci_dev_ids.h"
> @@ -520,6 +522,7 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
>   	.get_eeprom_length            = i40e_get_eeprom_length,
>   	.get_eeprom                   = i40e_get_eeprom,
>   	.mac_addr_set                 = i40e_set_default_mac_addr,
> +	.mtu_set                      = i40e_dev_mtu_set,
>   };
>   
>   /* store statistics names and its offset in stats structure */
> @@ -9104,3 +9107,49 @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
>   	/* Flags: 0x3 updates port address */
>   	i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes, NULL);
>   }
> +
> +static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
According to the coding style, at function definition, "The function 
type should be on a line by itself preceding the function."
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> +	struct rte_eth_dev_info dev_info;
> +	struct i40e_rx_queue *rxq;
> +	int i;
> +	uint16_t len;
> +	uint32_t frame_size = mtu + ETHER_HDR_LEN + ETHER_CRC_LEN;
> +	int ret = 0;
> +
> +	i40e_dev_info_get(dev, &dev_info);
> +
> +	/* check if mtu is within the allowed range */
> +	if ((mtu < ETHER_MIN_MTU) || (frame_size > dev_info.max_rx_pktlen))
> +		return -EINVAL;
> +
The dev_info.max_rx_pktlen queried by i40e_dev_info_get is 
"I40E_FRAME_SIZE_MAX".
No need to call the API to get dev info in driver.

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

* Re: [dpdk-dev] [PATCH] i40e: configure MTU
  2016-04-23 11:26 [dpdk-dev] [PATCH] i40e: configure MTU Beilei Xing
  2016-04-26  2:00 ` Wu, Jingjing
@ 2016-04-27 11:43 ` Julien Meunier
  2016-04-27 15:01   ` Xing, Beilei
  2016-04-28  3:19 ` [dpdk-dev] [PATCH V2] " Beilei Xing
  2 siblings, 1 reply; 22+ messages in thread
From: Julien Meunier @ 2016-04-27 11:43 UTC (permalink / raw)
  To: Beilei Xing, jingjing.wu; +Cc: dev

Hello,

On 04/23/2016 01:26 PM, Beilei Xing wrote:
[...]
> +	/* mtu setting is forbidden if port is start */
> +	if (dev_data->dev_started) {
> +		PMD_DRV_LOG(ERR,
> +			    "port %d must be stopped before configuration\n",
> +			    dev_data->port_id);
> +		return -EBUSY;
> +	}

According to rte_ethdev.h, only 4 return codes are supported for
rte_eth_dev_set_mtu:

* - (0) if successful.
* - (-ENOTSUP) if operation is not supported.
* - (-ENODEV) if *port_id* invalid.
* - (-EINVAL) if *mtu* invalid.

EBUSY should not be returned.

> +	for (i = 0; i < dev_data->nb_rx_queues; i++) {
> +		rxq = dev_data->rx_queues[i];
> +		if (!rxq || !rxq->q_set)
> +			continue;
> +
> +		dev_data->dev_conf.rxmode.max_rx_pkt_len = frame_size;
> +		len = hw->func_caps.rx_buf_chain_len * rxq->rx_buf_len;
> +		rxq->max_pkt_len = RTE_MIN(len, frame_size);
> +	}
> +
> +	ret = i40e_dev_rx_init(pf);
> +
> +	return ret;
> +}
> 

Why do want to reconfigure rxq here ? All these operations are already
done when you call i40e_dev_rx_init.
i40e_dev_rx_init => i40e_rx_queue_init (for each queue) =>
i40e_rx_queue_config => redefine rxq->max_pkt_len

Moreover, you should move dev_data->dev_conf.rxmode.max_rx_pkt_len out
of the loop. frame_size is the same for all rx_queues.

-- 
Julien MEUNIER
6WIND

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

* Re: [dpdk-dev] [PATCH] i40e: configure MTU
  2016-04-27 11:43 ` Julien Meunier
@ 2016-04-27 15:01   ` Xing, Beilei
  0 siblings, 0 replies; 22+ messages in thread
From: Xing, Beilei @ 2016-04-27 15:01 UTC (permalink / raw)
  To: Julien Meunier, Wu, Jingjing; +Cc: dev

> -----Original Message-----
> From: Julien Meunier [mailto:julien.meunier@6wind.com]
> Sent: Wednesday, April 27, 2016 7:44 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] i40e: configure MTU
> 
> Hello,
> 
> On 04/23/2016 01:26 PM, Beilei Xing wrote:
> [...]
> > +	/* mtu setting is forbidden if port is start */
> > +	if (dev_data->dev_started) {
> > +		PMD_DRV_LOG(ERR,
> > +			    "port %d must be stopped before configuration\n",
> > +			    dev_data->port_id);
> > +		return -EBUSY;
> > +	}
> 
> According to rte_ethdev.h, only 4 return codes are supported for
> rte_eth_dev_set_mtu:
> 
> * - (0) if successful.
> * - (-ENOTSUP) if operation is not supported.
> * - (-ENODEV) if *port_id* invalid.
> * - (-EINVAL) if *mtu* invalid.
> 
> EBUSY should not be returned.

Hi Meunier,
Thanks for your comments, it shouldn't be EBUSY here.

> 
> > +	for (i = 0; i < dev_data->nb_rx_queues; i++) {
> > +		rxq = dev_data->rx_queues[i];
> > +		if (!rxq || !rxq->q_set)
> > +			continue;
> > +
> > +		dev_data->dev_conf.rxmode.max_rx_pkt_len = frame_size;
> > +		len = hw->func_caps.rx_buf_chain_len * rxq->rx_buf_len;
> > +		rxq->max_pkt_len = RTE_MIN(len, frame_size);
> > +	}
> > +
> > +	ret = i40e_dev_rx_init(pf);
> > +
> > +	return ret;
> > +}
> >
> 
> Why do want to reconfigure rxq here ? All these operations are already done
> when you call i40e_dev_rx_init.
> i40e_dev_rx_init => i40e_rx_queue_init (for each queue) =>
> i40e_rx_queue_config => redefine rxq->max_pkt_len
> 
> Moreover, you should move dev_data->dev_conf.rxmode.max_rx_pkt_len out
> of the loop. frame_size is the same for all rx_queues.

Yes, you're right, needn't reconfigure rxq->max_pkt_len here, I just need reconfigure dev_data->dev_conf.rxmode.max_rx_pkt_len cause i40e_dev_rx_init will cover all. 
Thanks very much.
Beilei

> 
> --
> Julien MEUNIER
> 6WIND

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

* [dpdk-dev] [PATCH V2] i40e: configure MTU
  2016-04-23 11:26 [dpdk-dev] [PATCH] i40e: configure MTU Beilei Xing
  2016-04-26  2:00 ` Wu, Jingjing
  2016-04-27 11:43 ` Julien Meunier
@ 2016-04-28  3:19 ` Beilei Xing
  2016-05-13  8:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
  2 siblings, 1 reply; 22+ messages in thread
From: Beilei Xing @ 2016-04-28  3:19 UTC (permalink / raw)
  To: jingjing.wu; +Cc: dev, Beilei Xing

This patch enables configuring MTU for i40e.
Since changing MTU needs to reconfigure queue, stop port first
before configuring MTU.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d8b6bd7..c56ac70 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -447,6 +447,8 @@ static int i40e_get_eeprom(struct rte_eth_dev *dev,
 static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 				      struct ether_addr *mac_addr);
 
+static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
+
 static const struct rte_pci_id pci_id_i40e_map[] = {
 #define RTE_PCI_DEV_ID_DECL_I40E(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
 #include "rte_pci_dev_ids.h"
@@ -520,6 +522,7 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.get_eeprom_length            = i40e_get_eeprom_length,
 	.get_eeprom                   = i40e_get_eeprom,
 	.mac_addr_set                 = i40e_set_default_mac_addr,
+	.mtu_set                      = i40e_dev_mtu_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -9103,3 +9106,35 @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 	/* Flags: 0x3 updates port address */
 	i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes, NULL);
 }
+
+static int
+i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
+{
+	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	struct rte_eth_dev_data *dev_data = pf->dev_data;
+	uint32_t frame_size = mtu + ETHER_HDR_LEN + ETHER_CRC_LEN;
+	int ret = 0;
+
+	/* check if mtu is within the allowed range */
+	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
+		return -EINVAL;
+
+	/* mtu setting is forbidden if port is start */
+	if (dev_data->dev_started) {
+		PMD_DRV_LOG(ERR,
+			    "port %d must be stopped before configuration\n",
+			    dev_data->port_id);
+		return -ENOTSUP;
+	}
+
+	if (frame_size > ETHER_MAX_LEN)
+		dev_data->dev_conf.rxmode.jumbo_frame = 1;
+	else
+		dev_data->dev_conf.rxmode.jumbo_frame = 0;
+
+	dev_data->dev_conf.rxmode.max_rx_pkt_len = frame_size;
+
+	ret = i40e_dev_rx_init(pf);
+
+	return ret;
+}
-- 
2.5.0

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

* [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-04-28  3:19 ` [dpdk-dev] [PATCH V2] " Beilei Xing
@ 2016-05-13  8:15   ` Beilei Xing
  2016-05-16 12:27     ` Olivier Matz
  2016-05-20 15:17     ` [dpdk-dev] [PATCH v4] " Beilei Xing
  0 siblings, 2 replies; 22+ messages in thread
From: Beilei Xing @ 2016-05-13  8:15 UTC (permalink / raw)
  To: jingjing.wu; +Cc: dev, Beilei Xing

This patch enables configuring MTU for i40e.
Since changing MTU needs to reconfigure queue, stop port first
before configuring MTU.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
v3 changes:
 Add frame size with extra I40E_VLAN_TAG_SIZE.
 Delete i40e_dev_rx_init(pf) cause it will be called when port starts.

v2 changes:
 If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
 Delete rxq reconfigure cause rxq reconfigure will be finished in
 i40e_dev_rx_init.
   
 drivers/net/i40e/i40e_ethdev.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d8b6bd7..96d10c2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -447,6 +447,8 @@ static int i40e_get_eeprom(struct rte_eth_dev *dev,
 static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 				      struct ether_addr *mac_addr);
 
+static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
+
 static const struct rte_pci_id pci_id_i40e_map[] = {
 #define RTE_PCI_DEV_ID_DECL_I40E(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
 #include "rte_pci_dev_ids.h"
@@ -520,6 +522,7 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.get_eeprom_length            = i40e_get_eeprom_length,
 	.get_eeprom                   = i40e_get_eeprom,
 	.mac_addr_set                 = i40e_set_default_mac_addr,
+	.mtu_set                      = i40e_dev_mtu_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -9103,3 +9106,34 @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 	/* Flags: 0x3 updates port address */
 	i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes, NULL);
 }
+
+static int
+i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
+{
+	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	struct rte_eth_dev_data *dev_data = pf->dev_data;
+	uint32_t frame_size = mtu + ETHER_HDR_LEN
+			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
+	int ret = 0;
+
+	/* check if mtu is within the allowed range */
+	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
+		return -EINVAL;
+
+	/* mtu setting is forbidden if port is start */
+	if (dev_data->dev_started) {
+		PMD_DRV_LOG(ERR,
+			    "port %d must be stopped before configuration\n",
+			    dev_data->port_id);
+		return -ENOTSUP;
+	}
+
+	if (frame_size > ETHER_MAX_LEN)
+		dev_data->dev_conf.rxmode.jumbo_frame = 1;
+	else
+		dev_data->dev_conf.rxmode.jumbo_frame = 0;
+
+	dev_data->dev_conf.rxmode.max_rx_pkt_len = frame_size;
+
+	return ret;
+}
-- 
2.5.0

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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-05-13  8:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
@ 2016-05-16 12:27     ` Olivier Matz
  2016-06-16 17:40       ` Yong Wang
  2016-05-20 15:17     ` [dpdk-dev] [PATCH v4] " Beilei Xing
  1 sibling, 1 reply; 22+ messages in thread
From: Olivier Matz @ 2016-05-16 12:27 UTC (permalink / raw)
  To: Beilei Xing, jingjing.wu; +Cc: dev, Julien Meunier, Thomas Monjalon

Hi Beilei,

On 05/13/2016 10:15 AM, Beilei Xing wrote:
> This patch enables configuring MTU for i40e.
> Since changing MTU needs to reconfigure queue, stop port first
> before configuring MTU.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> v3 changes:
>  Add frame size with extra I40E_VLAN_TAG_SIZE.
>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> 
> v2 changes:
>  If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
>  Delete rxq reconfigure cause rxq reconfigure will be finished in
>  i40e_dev_rx_init.
>    
>  drivers/net/i40e/i40e_ethdev.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> [...]
> +static int
> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> +	int ret = 0;
> +
> +	/* check if mtu is within the allowed range */
> +	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
> +		return -EINVAL;
> +
> +	/* mtu setting is forbidden if port is start */
> +	if (dev_data->dev_started) {
> +		PMD_DRV_LOG(ERR,
> +			    "port %d must be stopped before configuration\n",
> +			    dev_data->port_id);
> +		return -ENOTSUP;
> +	}

I'm not convinced that ENOTSUP is the proper return value here.
It is usually returned when a function is not implemented, which
is not the case here: the function is implemented but is forbidden
because the port is running.

I saw that Julien commented on your v1 that the return value should
be one of:
 - (0) if successful.
 - (-ENOTSUP) if operation is not supported.
 - (-ENODEV) if *port_id* invalid.
 - (-EINVAL) if *mtu* invalid.

But I think your initial value (-EBUSY) was fine. Maybe it should be
added in the API instead, with the following description:
  (-EBUSY) if the operation is not allowed when the port is running

This would allow the application to take its dispositions to stop the
port and restart it with the proper jumbo_frame argument.

+CC Thomas which maintains ethdev API.


Regards,
Olivier

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

* [dpdk-dev] [PATCH v4] i40e: configure MTU
  2016-05-13  8:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
  2016-05-16 12:27     ` Olivier Matz
@ 2016-05-20 15:17     ` Beilei Xing
  2016-05-23  1:33       ` Wu, Jingjing
  1 sibling, 1 reply; 22+ messages in thread
From: Beilei Xing @ 2016-05-20 15:17 UTC (permalink / raw)
  To: jingjing.wu; +Cc: dev, Beilei Xing

This patch enables configuring MTU for i40e.
Since changing MTU needs to reconfigure queue, stop port first
before configuring MTU.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
v4 changes:
 Revert v2 change, if the port is running, return -EBUSY.

 doc/guides/rel_notes/release_16_07.rst |  3 +++
 drivers/net/i40e/i40e_ethdev.c         | 34 ++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h          |  1 +
 3 files changed, 38 insertions(+)

diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
index 30e78d4..4b1c176 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -116,6 +116,9 @@ API Changes
   ibadcrc, ibadlen, imcasts, fdirmatch, fdirmiss,
   tx_pause_xon, rx_pause_xon, tx_pause_xoff, rx_pause_xoff.
 
+* The function ``rte_eth_dev_set_mtu`` adds a new return value ``-EBUSY``, which
+  indicates the operation is forbidden because the port is running.
+
 
 ABI Changes
 -----------
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 24777d5..ffccaae 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -447,6 +447,8 @@ static int i40e_get_eeprom(struct rte_eth_dev *dev,
 static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 				      struct ether_addr *mac_addr);
 
+static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
+
 static const struct rte_pci_id pci_id_i40e_map[] = {
 #define RTE_PCI_DEV_ID_DECL_I40E(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
 #include "rte_pci_dev_ids.h"
@@ -520,6 +522,7 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.get_eeprom_length            = i40e_get_eeprom_length,
 	.get_eeprom                   = i40e_get_eeprom,
 	.mac_addr_set                 = i40e_set_default_mac_addr,
+	.mtu_set                      = i40e_dev_mtu_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -9108,3 +9111,34 @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 	/* Flags: 0x3 updates port address */
 	i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes, NULL);
 }
+
+static int
+i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
+{
+	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	struct rte_eth_dev_data *dev_data = pf->dev_data;
+	uint32_t frame_size = mtu + ETHER_HDR_LEN
+			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
+	int ret = 0;
+
+	/* check if mtu is within the allowed range */
+	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
+		return -EINVAL;
+
+	/* mtu setting is forbidden if port is start */
+	if (dev_data->dev_started) {
+		PMD_DRV_LOG(ERR,
+			    "port %d must be stopped before configuration\n",
+			    dev_data->port_id);
+		return -EBUSY;
+	}
+
+	if (frame_size > ETHER_MAX_LEN)
+		dev_data->dev_conf.rxmode.jumbo_frame = 1;
+	else
+		dev_data->dev_conf.rxmode.jumbo_frame = 0;
+
+	dev_data->dev_conf.rxmode.max_rx_pkt_len = frame_size;
+
+	return ret;
+}
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 2757510..a8d9963 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2398,6 +2398,7 @@ int rte_eth_dev_get_mtu(uint8_t port_id, uint16_t *mtu);
  *   - (-ENOTSUP) if operation is not supported.
  *   - (-ENODEV) if *port_id* invalid.
  *   - (-EINVAL) if *mtu* invalid.
+ *   - (-EBUSY) if operation is not allowed when the port is running
  */
 int rte_eth_dev_set_mtu(uint8_t port_id, uint16_t mtu);
 
-- 
2.5.0

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

* Re: [dpdk-dev] [PATCH v4] i40e: configure MTU
  2016-05-20 15:17     ` [dpdk-dev] [PATCH v4] " Beilei Xing
@ 2016-05-23  1:33       ` Wu, Jingjing
  2016-06-09 14:23         ` Bruce Richardson
  0 siblings, 1 reply; 22+ messages in thread
From: Wu, Jingjing @ 2016-05-23  1:33 UTC (permalink / raw)
  To: Xing, Beilei; +Cc: dev



> -----Original Message-----
> From: Xing, Beilei
> Sent: Friday, May 20, 2016 11:17 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org; Xing, Beilei
> Subject: [PATCH v4] i40e: configure MTU
> 
> This patch enables configuring MTU for i40e.
> Since changing MTU needs to reconfigure queue, stop port first before
> configuring MTU.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>

Acked-by: Jingjing Wu <jingjing.wu@intel.com>

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

* Re: [dpdk-dev] [PATCH v4] i40e: configure MTU
  2016-05-23  1:33       ` Wu, Jingjing
@ 2016-06-09 14:23         ` Bruce Richardson
  2016-06-23 10:13           ` Bruce Richardson
  0 siblings, 1 reply; 22+ messages in thread
From: Bruce Richardson @ 2016-06-09 14:23 UTC (permalink / raw)
  To: Wu, Jingjing; +Cc: Xing, Beilei, dev

On Mon, May 23, 2016 at 01:33:42AM +0000, Wu, Jingjing wrote:
> 
> 
> > -----Original Message-----
> > From: Xing, Beilei
> > Sent: Friday, May 20, 2016 11:17 PM
> > To: Wu, Jingjing
> > Cc: dev@dpdk.org; Xing, Beilei
> > Subject: [PATCH v4] i40e: configure MTU
> > 
> > This patch enables configuring MTU for i40e.
> > Since changing MTU needs to reconfigure queue, stop port first before
> > configuring MTU.
> > 
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> 
> Acked-by: Jingjing Wu <jingjing.wu@intel.com>
> 
Applied to dpdk-next-net/rel_16_07

/Bruce

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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-05-16 12:27     ` Olivier Matz
@ 2016-06-16 17:40       ` Yong Wang
  2016-06-16 17:51         ` Yong Wang
  2016-06-20  4:49         ` Xing, Beilei
  0 siblings, 2 replies; 22+ messages in thread
From: Yong Wang @ 2016-06-16 17:40 UTC (permalink / raw)
  To: Olivier Matz, Beilei Xing, jingjing.wu
  Cc: dev, Julien Meunier, Thomas Monjalon

On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz" <dev-bounces@dpdk.org on behalf of olivier.matz@6wind.com> wrote:

>Hi Beilei,
>
>On 05/13/2016 10:15 AM, Beilei Xing wrote:
>> This patch enables configuring MTU for i40e.
>> Since changing MTU needs to reconfigure queue, stop port first
>> before configuring MTU.
>> 
>> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
>> ---
>> v3 changes:
>>  Add frame size with extra I40E_VLAN_TAG_SIZE.
>>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
>> 
>> v2 changes:
>>  If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
>>  Delete rxq reconfigure cause rxq reconfigure will be finished in
>>  i40e_dev_rx_init.
>>    
>>  drivers/net/i40e/i40e_ethdev.c | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>> 
>> [...]
>> +static int
>> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
>> +{
>> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
>> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
>> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
>> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
>> +	int ret = 0;
>> +
>> +	/* check if mtu is within the allowed range */
>> +	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
>> +		return -EINVAL;
>> +
>> +	/* mtu setting is forbidden if port is start */
>> +	if (dev_data->dev_started) {
>> +		PMD_DRV_LOG(ERR,
>> +			    "port %d must be stopped before configuration\n",
>> +			    dev_data->port_id);
>> +		return -ENOTSUP;
>> +	}
>
>I'm not convinced that ENOTSUP is the proper return value here.
>It is usually returned when a function is not implemented, which
>is not the case here: the function is implemented but is forbidden
>because the port is running.
>
>I saw that Julien commented on your v1 that the return value should
>be one of:
> - (0) if successful.
> - (-ENOTSUP) if operation is not supported.
> - (-ENODEV) if *port_id* invalid.
> - (-EINVAL) if *mtu* invalid.
>
>But I think your initial value (-EBUSY) was fine. Maybe it should be
>added in the API instead, with the following description:
>  (-EBUSY) if the operation is not allowed when the port is running

AFAICT, the same check is not done for other drivers that implement
the mac_set op. Wouldn’t it make more sense to have the driver disable
the port, reconfigure and re-enable it in this case, instead of returning
error code?  If the consensus in DPDK is to have the application disable
the port first, we need to enforce this policy across all devices and
clearly document this behavior.

>This would allow the application to take its dispositions to stop the
>port and restart it with the proper jumbo_frame argument.
>
>+CC Thomas which maintains ethdev API.
>
>
>Regards,
>Olivier


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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-06-16 17:40       ` Yong Wang
@ 2016-06-16 17:51         ` Yong Wang
  2016-06-17  0:03           ` Ananyev, Konstantin
  2016-06-20  4:49         ` Xing, Beilei
  1 sibling, 1 reply; 22+ messages in thread
From: Yong Wang @ 2016-06-16 17:51 UTC (permalink / raw)
  To: Olivier Matz, Beilei Xing, jingjing.wu
  Cc: dev, Julien Meunier, Thomas Monjalon

On 6/16/16, 10:40 AM, "dev on behalf of Yong Wang" <dev-bounces@dpdk.org on behalf of yongwang@vmware.com> wrote:

>On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz" <dev-bounces@dpdk.org on behalf of olivier.matz@6wind.com> wrote:
>
>>Hi Beilei,
>>
>>On 05/13/2016 10:15 AM, Beilei Xing wrote:
>>> This patch enables configuring MTU for i40e.
>>> Since changing MTU needs to reconfigure queue, stop port first
>>> before configuring MTU.
>>> 
>>> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
>>> ---
>>> v3 changes:
>>>  Add frame size with extra I40E_VLAN_TAG_SIZE.
>>>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
>>> 
>>> v2 changes:
>>>  If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
>>>  Delete rxq reconfigure cause rxq reconfigure will be finished in
>>>  i40e_dev_rx_init.
>>>    
>>>  drivers/net/i40e/i40e_ethdev.c | 34 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 34 insertions(+)
>>> 
>>> [...]
>>> +static int
>>> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
>>> +{
>>> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
>>> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
>>> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
>>> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
>>> +	int ret = 0;
>>> +
>>> +	/* check if mtu is within the allowed range */
>>> +	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
>>> +		return -EINVAL;
>>> +
>>> +	/* mtu setting is forbidden if port is start */
>>> +	if (dev_data->dev_started) {
>>> +		PMD_DRV_LOG(ERR,
>>> +			    "port %d must be stopped before configuration\n",
>>> +			    dev_data->port_id);
>>> +		return -ENOTSUP;
>>> +	}
>>
>>I'm not convinced that ENOTSUP is the proper return value here.
>>It is usually returned when a function is not implemented, which
>>is not the case here: the function is implemented but is forbidden
>>because the port is running.
>>
>>I saw that Julien commented on your v1 that the return value should
>>be one of:
>> - (0) if successful.
>> - (-ENOTSUP) if operation is not supported.
>> - (-ENODEV) if *port_id* invalid.
>> - (-EINVAL) if *mtu* invalid.
>>
>>But I think your initial value (-EBUSY) was fine. Maybe it should be
>>added in the API instead, with the following description:
>>  (-EBUSY) if the operation is not allowed when the port is running
>
>AFAICT, the same check is not done for other drivers that implement
>the mac_set op. Wouldn’t it make more sense to have the driver disable

Correction: this should read as mtu_set.

>the port, reconfigure and re-enable it in this case, instead of returning
>error code?  If the consensus in DPDK is to have the application disable
>the port first, we need to enforce this policy across all devices and
>clearly document this behavior.
>
>>This would allow the application to take its dispositions to stop the
>>port and restart it with the proper jumbo_frame argument.
>>
>>+CC Thomas which maintains ethdev API.
>>
>>
>>Regards,
>>Olivier
>


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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-06-16 17:51         ` Yong Wang
@ 2016-06-17  0:03           ` Ananyev, Konstantin
  0 siblings, 0 replies; 22+ messages in thread
From: Ananyev, Konstantin @ 2016-06-17  0:03 UTC (permalink / raw)
  To: Yong Wang, Olivier Matz, Xing, Beilei, Wu, Jingjing
  Cc: dev, Julien Meunier, Thomas Monjalon



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yong Wang
> Sent: Thursday, June 16, 2016 6:52 PM
> To: Olivier Matz; Xing, Beilei; Wu, Jingjing
> Cc: dev@dpdk.org; Julien Meunier; Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> 
> On 6/16/16, 10:40 AM, "dev on behalf of Yong Wang" <dev-bounces@dpdk.org on behalf of yongwang@vmware.com> wrote:
> 
> >On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz" <dev-bounces@dpdk.org on behalf of olivier.matz@6wind.com> wrote:
> >
> >>Hi Beilei,
> >>
> >>On 05/13/2016 10:15 AM, Beilei Xing wrote:
> >>> This patch enables configuring MTU for i40e.
> >>> Since changing MTU needs to reconfigure queue, stop port first
> >>> before configuring MTU.
> >>>
> >>> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> >>> ---
> >>> v3 changes:
> >>>  Add frame size with extra I40E_VLAN_TAG_SIZE.
> >>>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> >>>
> >>> v2 changes:
> >>>  If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
> >>>  Delete rxq reconfigure cause rxq reconfigure will be finished in
> >>>  i40e_dev_rx_init.
> >>>
> >>>  drivers/net/i40e/i40e_ethdev.c | 34 ++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 34 insertions(+)
> >>>
> >>> [...]
> >>> +static int
> >>> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
> >>> +{
> >>> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> >>> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> >>> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> >>> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> >>> +	int ret = 0;
> >>> +
> >>> +	/* check if mtu is within the allowed range */
> >>> +	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
> >>> +		return -EINVAL;
> >>> +
> >>> +	/* mtu setting is forbidden if port is start */
> >>> +	if (dev_data->dev_started) {
> >>> +		PMD_DRV_LOG(ERR,
> >>> +			    "port %d must be stopped before configuration\n",
> >>> +			    dev_data->port_id);
> >>> +		return -ENOTSUP;
> >>> +	}
> >>
> >>I'm not convinced that ENOTSUP is the proper return value here.
> >>It is usually returned when a function is not implemented, which
> >>is not the case here: the function is implemented but is forbidden
> >>because the port is running.
> >>
> >>I saw that Julien commented on your v1 that the return value should
> >>be one of:
> >> - (0) if successful.
> >> - (-ENOTSUP) if operation is not supported.
> >> - (-ENODEV) if *port_id* invalid.
> >> - (-EINVAL) if *mtu* invalid.
> >>
> >>But I think your initial value (-EBUSY) was fine. Maybe it should be
> >>added in the API instead, with the following description:
> >>  (-EBUSY) if the operation is not allowed when the port is running
> >
> >AFAICT, the same check is not done for other drivers that implement
> >the mac_set op. Wouldn’t it make more sense to have the driver disable
> 
> Correction: this should read as mtu_set.
> 
> >the port, reconfigure and re-enable it in this case, instead of returning
> >error code?  If the consensus in DPDK is to have the application disable
> >the port first, we need to enforce this policy across all devices and
> >clearly document this behavior.

Other PMDs do allow to change mtu without stopping/reconfiguration the port
(under certain conditions of course).
As I remember that's why that function was introduced at the first place.
It is a bit strange that i40e doesn't allow that, might be the author can
comment why it is necessary.
Again the whole i40e_dev_mtu_set() looks a bit strange to me -
I can't see where the actual change of HW state is happening.
Konstantin

> >
> >>This would allow the application to take its dispositions to stop the
> >>port and restart it with the proper jumbo_frame argument.
> >>
> >>+CC Thomas which maintains ethdev API.
> >>
> >>
> >>Regards,
> >>Olivier
> >


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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-06-16 17:40       ` Yong Wang
  2016-06-16 17:51         ` Yong Wang
@ 2016-06-20  4:49         ` Xing, Beilei
  2016-06-20  4:59           ` Xing, Beilei
  2016-06-20  8:05           ` Ananyev, Konstantin
  1 sibling, 2 replies; 22+ messages in thread
From: Xing, Beilei @ 2016-06-20  4:49 UTC (permalink / raw)
  To: Yong Wang, Olivier Matz, Wu, Jingjing
  Cc: dev, Julien Meunier, Thomas Monjalon


> -----Original Message-----
> From: Yong Wang [mailto:yongwang@vmware.com]
> Sent: Friday, June 17, 2016 1:40 AM
> To: Olivier Matz <olivier.matz@6wind.com>; Xing, Beilei <beilei.xing@intel.com>;
> Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> Monjalon <thomas.monjalon@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> 
> On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz" <dev-bounces@dpdk.org
> on behalf of olivier.matz@6wind.com> wrote:
> 
> >Hi Beilei,
> >
> >On 05/13/2016 10:15 AM, Beilei Xing wrote:
> >> This patch enables configuring MTU for i40e.
> >> Since changing MTU needs to reconfigure queue, stop port first before
> >> configuring MTU.
> >>
> >> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> >> ---
> >> v3 changes:
> >>  Add frame size with extra I40E_VLAN_TAG_SIZE.
> >>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> >>
> >> v2 changes:
> >>  If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
> >>  Delete rxq reconfigure cause rxq reconfigure will be finished in
> >> i40e_dev_rx_init.
> >>
> >>  drivers/net/i40e/i40e_ethdev.c | 34
> >> ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >> [...]
> >> +static int
> >> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> >> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> >> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> >> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> >> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> >> +	int ret = 0;
> >> +
> >> +	/* check if mtu is within the allowed range */
> >> +	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
> >> +		return -EINVAL;
> >> +
> >> +	/* mtu setting is forbidden if port is start */
> >> +	if (dev_data->dev_started) {
> >> +		PMD_DRV_LOG(ERR,
> >> +			    "port %d must be stopped before configuration\n",
> >> +			    dev_data->port_id);
> >> +		return -ENOTSUP;
> >> +	}
> >
> >I'm not convinced that ENOTSUP is the proper return value here.
> >It is usually returned when a function is not implemented, which is not
> >the case here: the function is implemented but is forbidden because the
> >port is running.
> >
> >I saw that Julien commented on your v1 that the return value should be
> >one of:
> > - (0) if successful.
> > - (-ENOTSUP) if operation is not supported.
> > - (-ENODEV) if *port_id* invalid.
> > - (-EINVAL) if *mtu* invalid.
> >
> >But I think your initial value (-EBUSY) was fine. Maybe it should be
> >added in the API instead, with the following description:
> >  (-EBUSY) if the operation is not allowed when the port is running
> 
> AFAICT, the same check is not done for other drivers that implement the mac_set
> op. Wouldn’t it make more sense to have the driver disable the port,
> reconfigure and re-enable it in this case, instead of returning error code?  If the
> consensus in DPDK is to have the application disable the port first, we need to
> enforce this policy across all devices and clearly document this behavior.
> 
Hi,
For ixgbe/igb, there's register for MTU during runtime, but for i40e, setting MTU is finished by firmware during configuration. There'll be some risk when disable the port, reconfigure and re-enable the port, so return error code in this case currently.
Thanks for your comments, we'll improve the document later.

> >This would allow the application to take its dispositions to stop the
> >port and restart it with the proper jumbo_frame argument.
> >
> >+CC Thomas which maintains ethdev API.
> >
> >
> >Regards,
> >Olivier


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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-06-20  4:49         ` Xing, Beilei
@ 2016-06-20  4:59           ` Xing, Beilei
  2016-06-20  8:05           ` Ananyev, Konstantin
  1 sibling, 0 replies; 22+ messages in thread
From: Xing, Beilei @ 2016-06-20  4:59 UTC (permalink / raw)
  To: Ananyev, Konstantin, Yong Wang, Olivier Matz, Wu, Jingjing
  Cc: dev, Julien Meunier, Thomas Monjalon


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> Sent: Monday, June 20, 2016 12:50 PM
> To: Yong Wang <yongwang@vmware.com>; Olivier Matz
> <olivier.matz@6wind.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> Monjalon <thomas.monjalon@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> 
> 
> > -----Original Message-----
> > From: Yong Wang [mailto:yongwang@vmware.com]
> > Sent: Friday, June 17, 2016 1:40 AM
> > To: Olivier Matz <olivier.matz@6wind.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> > Monjalon <thomas.monjalon@6wind.com>
> > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> >
> > On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz"
> > <dev-bounces@dpdk.org on behalf of olivier.matz@6wind.com> wrote:
> >
> > >Hi Beilei,
> > >
> > >On 05/13/2016 10:15 AM, Beilei Xing wrote:
> > >> This patch enables configuring MTU for i40e.
> > >> Since changing MTU needs to reconfigure queue, stop port first
> > >> before configuring MTU.
> > >>
> > >> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > >> ---
> > >> v3 changes:
> > >>  Add frame size with extra I40E_VLAN_TAG_SIZE.
> > >>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> > >>
> > >> v2 changes:
> > >>  If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
> > >>  Delete rxq reconfigure cause rxq reconfigure will be finished in
> > >> i40e_dev_rx_init.
> > >>
> > >>  drivers/net/i40e/i40e_ethdev.c | 34
> > >> ++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 34 insertions(+)
> > >>
> > >> [...]
> > >> +static int
> > >> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> > >> +	struct i40e_pf *pf =
> I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > >> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> > >> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> > >> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> > >> +	int ret = 0;
> > >> +
> > >> +	/* check if mtu is within the allowed range */
> > >> +	if ((mtu < ETHER_MIN_MTU) || (frame_size >
> I40E_FRAME_SIZE_MAX))
> > >> +		return -EINVAL;
> > >> +
> > >> +	/* mtu setting is forbidden if port is start */
> > >> +	if (dev_data->dev_started) {
> > >> +		PMD_DRV_LOG(ERR,
> > >> +			    "port %d must be stopped before configuration\n",
> > >> +			    dev_data->port_id);
> > >> +		return -ENOTSUP;
> > >> +	}
> > >
> > >I'm not convinced that ENOTSUP is the proper return value here.
> > >It is usually returned when a function is not implemented, which is
> > >not the case here: the function is implemented but is forbidden
> > >because the port is running.
> > >
> > >I saw that Julien commented on your v1 that the return value should
> > >be one of:
> > > - (0) if successful.
> > > - (-ENOTSUP) if operation is not supported.
> > > - (-ENODEV) if *port_id* invalid.
> > > - (-EINVAL) if *mtu* invalid.
> > >
> > >But I think your initial value (-EBUSY) was fine. Maybe it should be
> > >added in the API instead, with the following description:
> > >  (-EBUSY) if the operation is not allowed when the port is running
> >
> > AFAICT, the same check is not done for other drivers that implement
> > the mac_set op. Wouldn’t it make more sense to have the driver disable
> > the port, reconfigure and re-enable it in this case, instead of
> > returning error code?  If the consensus in DPDK is to have the
> > application disable the port first, we need to enforce this policy across all
> devices and clearly document this behavior.
> >
> Hi,
> For ixgbe/igb, there's register for MTU during runtime, but for i40e, setting MTU
> is finished by firmware during configuration. There'll be some risk when disable
> the port, reconfigure and re-enable the port, so return error code in this case
> currently.
Correction: register for max packet length.
> Thanks for your comments, we'll improve the document later.
> 
> > >This would allow the application to take its dispositions to stop the
> > >port and restart it with the proper jumbo_frame argument.
> > >
> > >+CC Thomas which maintains ethdev API.
> > >
> > >
> > >Regards,
> > >Olivier


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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-06-20  4:49         ` Xing, Beilei
  2016-06-20  4:59           ` Xing, Beilei
@ 2016-06-20  8:05           ` Ananyev, Konstantin
  2016-06-20 12:04             ` Xing, Beilei
  1 sibling, 1 reply; 22+ messages in thread
From: Ananyev, Konstantin @ 2016-06-20  8:05 UTC (permalink / raw)
  To: Xing, Beilei, Yong Wang, Olivier Matz, Wu, Jingjing
  Cc: dev, Julien Meunier, Thomas Monjalon



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> Sent: Monday, June 20, 2016 5:50 AM
> To: Yong Wang; Olivier Matz; Wu, Jingjing
> Cc: dev@dpdk.org; Julien Meunier; Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> 
> 
> > -----Original Message-----
> > From: Yong Wang [mailto:yongwang@vmware.com]
> > Sent: Friday, June 17, 2016 1:40 AM
> > To: Olivier Matz <olivier.matz@6wind.com>; Xing, Beilei <beilei.xing@intel.com>;
> > Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> > Monjalon <thomas.monjalon@6wind.com>
> > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> >
> > On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz" <dev-bounces@dpdk.org
> > on behalf of olivier.matz@6wind.com> wrote:
> >
> > >Hi Beilei,
> > >
> > >On 05/13/2016 10:15 AM, Beilei Xing wrote:
> > >> This patch enables configuring MTU for i40e.
> > >> Since changing MTU needs to reconfigure queue, stop port first before
> > >> configuring MTU.
> > >>
> > >> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > >> ---
> > >> v3 changes:
> > >>  Add frame size with extra I40E_VLAN_TAG_SIZE.
> > >>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> > >>
> > >> v2 changes:
> > >>  If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
> > >>  Delete rxq reconfigure cause rxq reconfigure will be finished in
> > >> i40e_dev_rx_init.
> > >>
> > >>  drivers/net/i40e/i40e_ethdev.c | 34
> > >> ++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 34 insertions(+)
> > >>
> > >> [...]
> > >> +static int
> > >> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> > >> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > >> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> > >> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> > >> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> > >> +	int ret = 0;
> > >> +
> > >> +	/* check if mtu is within the allowed range */
> > >> +	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
> > >> +		return -EINVAL;
> > >> +
> > >> +	/* mtu setting is forbidden if port is start */
> > >> +	if (dev_data->dev_started) {
> > >> +		PMD_DRV_LOG(ERR,
> > >> +			    "port %d must be stopped before configuration\n",
> > >> +			    dev_data->port_id);
> > >> +		return -ENOTSUP;
> > >> +	}
> > >
> > >I'm not convinced that ENOTSUP is the proper return value here.
> > >It is usually returned when a function is not implemented, which is not
> > >the case here: the function is implemented but is forbidden because the
> > >port is running.
> > >
> > >I saw that Julien commented on your v1 that the return value should be
> > >one of:
> > > - (0) if successful.
> > > - (-ENOTSUP) if operation is not supported.
> > > - (-ENODEV) if *port_id* invalid.
> > > - (-EINVAL) if *mtu* invalid.
> > >
> > >But I think your initial value (-EBUSY) was fine. Maybe it should be
> > >added in the API instead, with the following description:
> > >  (-EBUSY) if the operation is not allowed when the port is running
> >
> > AFAICT, the same check is not done for other drivers that implement the mac_set
> > op. Wouldn’t it make more sense to have the driver disable the port,
> > reconfigure and re-enable it in this case, instead of returning error code?  If the
> > consensus in DPDK is to have the application disable the port first, we need to
> > enforce this policy across all devices and clearly document this behavior.
> >
> Hi,
> For ixgbe/igb, there's register for MTU during runtime, but for i40e, setting MTU is finished by firmware during configuration. There'll
> be some risk when disable the port, reconfigure and re-enable the port, so return error code in this case currently.

Ok, but then what is the point to introduce mtu_set() for i40e at all?
Let's just leave it unimplemented, as it is right now.
Then users would know that for that device they have to do dev_stop/dev_configure().
Konstantin

> Thanks for your comments, we'll improve the document later.
> 
> > >This would allow the application to take its dispositions to stop the
> > >port and restart it with the proper jumbo_frame argument.
> > >
> > >+CC Thomas which maintains ethdev API.
> > >
> > >
> > >Regards,
> > >Olivier


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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-06-20  8:05           ` Ananyev, Konstantin
@ 2016-06-20 12:04             ` Xing, Beilei
  2016-06-20 12:15               ` Ananyev, Konstantin
  0 siblings, 1 reply; 22+ messages in thread
From: Xing, Beilei @ 2016-06-20 12:04 UTC (permalink / raw)
  To: Ananyev, Konstantin, Yong Wang, Olivier Matz, Wu, Jingjing
  Cc: dev, Julien Meunier, Thomas Monjalon


> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, June 20, 2016 4:05 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Yong Wang
> <yongwang@vmware.com>; Olivier Matz <olivier.matz@6wind.com>; Wu,
> Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> Monjalon <thomas.monjalon@6wind.com>
> Subject: RE: [dpdk-dev] [PATCH v3] i40e: configure MTU
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> > Sent: Monday, June 20, 2016 5:50 AM
> > To: Yong Wang; Olivier Matz; Wu, Jingjing
> > Cc: dev@dpdk.org; Julien Meunier; Thomas Monjalon
> > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> >
> >
> > > -----Original Message-----
> > > From: Yong Wang [mailto:yongwang@vmware.com]
> > > Sent: Friday, June 17, 2016 1:40 AM
> > > To: Olivier Matz <olivier.matz@6wind.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > > Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> > > Monjalon <thomas.monjalon@6wind.com>
> > > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> > >
> > > On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz"
> > > <dev-bounces@dpdk.org on behalf of olivier.matz@6wind.com> wrote:
> > >
> > > >Hi Beilei,
> > > >
> > > >On 05/13/2016 10:15 AM, Beilei Xing wrote:
> > > >> This patch enables configuring MTU for i40e.
> > > >> Since changing MTU needs to reconfigure queue, stop port first
> > > >> before configuring MTU.
> > > >>
> > > >> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > > >> ---
> > > >> v3 changes:
> > > >>  Add frame size with extra I40E_VLAN_TAG_SIZE.
> > > >>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> > > >>
> > > >> v2 changes:
> > > >>  If mtu is not within the allowed range, return -EINVAL instead of
> -EBUSY.
> > > >>  Delete rxq reconfigure cause rxq reconfigure will be finished in
> > > >> i40e_dev_rx_init.
> > > >>
> > > >>  drivers/net/i40e/i40e_ethdev.c | 34
> > > >> ++++++++++++++++++++++++++++++++++
> > > >>  1 file changed, 34 insertions(+)
> > > >>
> > > >> [...]
> > > >> +static int
> > > >> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> > > >> +	struct i40e_pf *pf =
> I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > > >> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> > > >> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> > > >> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> > > >> +	int ret = 0;
> > > >> +
> > > >> +	/* check if mtu is within the allowed range */
> > > >> +	if ((mtu < ETHER_MIN_MTU) || (frame_size >
> I40E_FRAME_SIZE_MAX))
> > > >> +		return -EINVAL;
> > > >> +
> > > >> +	/* mtu setting is forbidden if port is start */
> > > >> +	if (dev_data->dev_started) {
> > > >> +		PMD_DRV_LOG(ERR,
> > > >> +			    "port %d must be stopped before configuration\n",
> > > >> +			    dev_data->port_id);
> > > >> +		return -ENOTSUP;
> > > >> +	}
> > > >
> > > >I'm not convinced that ENOTSUP is the proper return value here.
> > > >It is usually returned when a function is not implemented, which is
> > > >not the case here: the function is implemented but is forbidden
> > > >because the port is running.
> > > >
> > > >I saw that Julien commented on your v1 that the return value should
> > > >be one of:
> > > > - (0) if successful.
> > > > - (-ENOTSUP) if operation is not supported.
> > > > - (-ENODEV) if *port_id* invalid.
> > > > - (-EINVAL) if *mtu* invalid.
> > > >
> > > >But I think your initial value (-EBUSY) was fine. Maybe it should
> > > >be added in the API instead, with the following description:
> > > >  (-EBUSY) if the operation is not allowed when the port is running
> > >
> > > AFAICT, the same check is not done for other drivers that implement
> > > the mac_set op. Wouldn’t it make more sense to have the driver
> > > disable the port, reconfigure and re-enable it in this case, instead
> > > of returning error code?  If the consensus in DPDK is to have the
> > > application disable the port first, we need to enforce this policy across all
> devices and clearly document this behavior.
> > >
> > Hi,
> > For ixgbe/igb, there's register for MTU during runtime, but for i40e,
> > setting MTU is finished by firmware during configuration. There'll be some risk
> when disable the port, reconfigure and re-enable the port, so return error code
> in this case currently.
> 
> Ok, but then what is the point to introduce mtu_set() for i40e at all?
> Let's just leave it unimplemented, as it is right now.
> Then users would know that for that device they have to do
> dev_stop/dev_configure().

Hi Konstantin,
The original intention is to reduce ops gap in different PMDs, but unfortunately, setting MTU in i40e couldn’t be finished as in ixgbe, so in the function, we change the configuration and note users that if want to configure MTU, stop ports first. The sequence of configuring MTU for i40e is: port stop->set MTU->port start.
We will add according instruction in document.
Beilei

> Konstantin
> 
> > Thanks for your comments, we'll improve the document later.
> >
> > > >This would allow the application to take its dispositions to stop
> > > >the port and restart it with the proper jumbo_frame argument.
> > > >
> > > >+CC Thomas which maintains ethdev API.
> > > >
> > > >
> > > >Regards,
> > > >Olivier


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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-06-20 12:04             ` Xing, Beilei
@ 2016-06-20 12:15               ` Ananyev, Konstantin
  2016-06-20 12:46                 ` Xing, Beilei
  0 siblings, 1 reply; 22+ messages in thread
From: Ananyev, Konstantin @ 2016-06-20 12:15 UTC (permalink / raw)
  To: Xing, Beilei, Yong Wang, Olivier Matz, Wu, Jingjing
  Cc: dev, Julien Meunier, Thomas Monjalon

Hi Beilei,

> -----Original Message-----
> From: Xing, Beilei
> Sent: Monday, June 20, 2016 1:05 PM
> To: Ananyev, Konstantin; Yong Wang; Olivier Matz; Wu, Jingjing
> Cc: dev@dpdk.org; Julien Meunier; Thomas Monjalon
> Subject: RE: [dpdk-dev] [PATCH v3] i40e: configure MTU
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Monday, June 20, 2016 4:05 PM
> > To: Xing, Beilei <beilei.xing@intel.com>; Yong Wang
> > <yongwang@vmware.com>; Olivier Matz <olivier.matz@6wind.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> > Monjalon <thomas.monjalon@6wind.com>
> > Subject: RE: [dpdk-dev] [PATCH v3] i40e: configure MTU
> >
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> > > Sent: Monday, June 20, 2016 5:50 AM
> > > To: Yong Wang; Olivier Matz; Wu, Jingjing
> > > Cc: dev@dpdk.org; Julien Meunier; Thomas Monjalon
> > > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> > >
> > >
> > > > -----Original Message-----
> > > > From: Yong Wang [mailto:yongwang@vmware.com]
> > > > Sent: Friday, June 17, 2016 1:40 AM
> > > > To: Olivier Matz <olivier.matz@6wind.com>; Xing, Beilei
> > > > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > > > Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> > > > Monjalon <thomas.monjalon@6wind.com>
> > > > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> > > >
> > > > On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz"
> > > > <dev-bounces@dpdk.org on behalf of olivier.matz@6wind.com> wrote:
> > > >
> > > > >Hi Beilei,
> > > > >
> > > > >On 05/13/2016 10:15 AM, Beilei Xing wrote:
> > > > >> This patch enables configuring MTU for i40e.
> > > > >> Since changing MTU needs to reconfigure queue, stop port first
> > > > >> before configuring MTU.
> > > > >>
> > > > >> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > > > >> ---
> > > > >> v3 changes:
> > > > >>  Add frame size with extra I40E_VLAN_TAG_SIZE.
> > > > >>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> > > > >>
> > > > >> v2 changes:
> > > > >>  If mtu is not within the allowed range, return -EINVAL instead of
> > -EBUSY.
> > > > >>  Delete rxq reconfigure cause rxq reconfigure will be finished in
> > > > >> i40e_dev_rx_init.
> > > > >>
> > > > >>  drivers/net/i40e/i40e_ethdev.c | 34
> > > > >> ++++++++++++++++++++++++++++++++++
> > > > >>  1 file changed, 34 insertions(+)
> > > > >>
> > > > >> [...]
> > > > >> +static int
> > > > >> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> > > > >> +	struct i40e_pf *pf =
> > I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > > > >> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> > > > >> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> > > > >> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> > > > >> +	int ret = 0;
> > > > >> +
> > > > >> +	/* check if mtu is within the allowed range */
> > > > >> +	if ((mtu < ETHER_MIN_MTU) || (frame_size >
> > I40E_FRAME_SIZE_MAX))
> > > > >> +		return -EINVAL;
> > > > >> +
> > > > >> +	/* mtu setting is forbidden if port is start */
> > > > >> +	if (dev_data->dev_started) {
> > > > >> +		PMD_DRV_LOG(ERR,
> > > > >> +			    "port %d must be stopped before configuration\n",
> > > > >> +			    dev_data->port_id);
> > > > >> +		return -ENOTSUP;
> > > > >> +	}
> > > > >
> > > > >I'm not convinced that ENOTSUP is the proper return value here.
> > > > >It is usually returned when a function is not implemented, which is
> > > > >not the case here: the function is implemented but is forbidden
> > > > >because the port is running.
> > > > >
> > > > >I saw that Julien commented on your v1 that the return value should
> > > > >be one of:
> > > > > - (0) if successful.
> > > > > - (-ENOTSUP) if operation is not supported.
> > > > > - (-ENODEV) if *port_id* invalid.
> > > > > - (-EINVAL) if *mtu* invalid.
> > > > >
> > > > >But I think your initial value (-EBUSY) was fine. Maybe it should
> > > > >be added in the API instead, with the following description:
> > > > >  (-EBUSY) if the operation is not allowed when the port is running
> > > >
> > > > AFAICT, the same check is not done for other drivers that implement
> > > > the mac_set op. Wouldn’t it make more sense to have the driver
> > > > disable the port, reconfigure and re-enable it in this case, instead
> > > > of returning error code?  If the consensus in DPDK is to have the
> > > > application disable the port first, we need to enforce this policy across all
> > devices and clearly document this behavior.
> > > >
> > > Hi,
> > > For ixgbe/igb, there's register for MTU during runtime, but for i40e,
> > > setting MTU is finished by firmware during configuration. There'll be some risk
> > when disable the port, reconfigure and re-enable the port, so return error code
> > in this case currently.
> >
> > Ok, but then what is the point to introduce mtu_set() for i40e at all?
> > Let's just leave it unimplemented, as it is right now.
> > Then users would know that for that device they have to do
> > dev_stop/dev_configure().
> 
> Hi Konstantin,
> The original intention is to reduce ops gap in different PMDs, but unfortunately, setting MTU in i40e couldn’t be finished as in ixgbe,

Yes, I understand that from your previous explanation.

> so in the function, we change the configuration and note users that if want to configure MTU, stop ports first.

Well, you can do that now with dev_stop(); dev_configure(), right?
As I remember, the original intention of mtu_set() was to give user an ability
to change mtu (whenever possible) without stopping the port.
If the HW (i40e) doesn't support it - that's fine, we just leave mtu_set() for i40e unimplemented. 

> The sequence of > configuring MTU for i40e is: port stop->set MTU->port start.
> We will add according instruction in document.

Sorry, but I don’t agree here.
I think, in that case it is better just to leave mtu_set() unimplemented for i40e.
Konstantin

> Beilei
> 
> > Konstantin
> >
> > > Thanks for your comments, we'll improve the document later.
> > >
> > > > >This would allow the application to take its dispositions to stop
> > > > >the port and restart it with the proper jumbo_frame argument.
> > > > >
> > > > >+CC Thomas which maintains ethdev API.
> > > > >
> > > > >
> > > > >Regards,
> > > > >Olivier


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

* Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
  2016-06-20 12:15               ` Ananyev, Konstantin
@ 2016-06-20 12:46                 ` Xing, Beilei
  0 siblings, 0 replies; 22+ messages in thread
From: Xing, Beilei @ 2016-06-20 12:46 UTC (permalink / raw)
  To: Ananyev, Konstantin, Yong Wang, Olivier Matz, Wu, Jingjing
  Cc: dev, Julien Meunier, Thomas Monjalon

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, June 20, 2016 8:15 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Yong Wang <yongwang@vmware.com>;
> Olivier Matz <olivier.matz@6wind.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> Monjalon <thomas.monjalon@6wind.com>
> Subject: RE: [dpdk-dev] [PATCH v3] i40e: configure MTU
> 
> Hi Beilei,
> 
> > -----Original Message-----
> > From: Xing, Beilei
> > Sent: Monday, June 20, 2016 1:05 PM
> > To: Ananyev, Konstantin; Yong Wang; Olivier Matz; Wu, Jingjing
> > Cc: dev@dpdk.org; Julien Meunier; Thomas Monjalon
> > Subject: RE: [dpdk-dev] [PATCH v3] i40e: configure MTU
> >
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Monday, June 20, 2016 4:05 PM
> > > To: Xing, Beilei <beilei.xing@intel.com>; Yong Wang
> > > <yongwang@vmware.com>; Olivier Matz <olivier.matz@6wind.com>; Wu,
> > > Jingjing <jingjing.wu@intel.com>
> > > Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> > > Monjalon <thomas.monjalon@6wind.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3] i40e: configure MTU
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> > > > Sent: Monday, June 20, 2016 5:50 AM
> > > > To: Yong Wang; Olivier Matz; Wu, Jingjing
> > > > Cc: dev@dpdk.org; Julien Meunier; Thomas Monjalon
> > > > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Yong Wang [mailto:yongwang@vmware.com]
> > > > > Sent: Friday, June 17, 2016 1:40 AM
> > > > > To: Olivier Matz <olivier.matz@6wind.com>; Xing, Beilei
> > > > > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > > > > Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>;
> > > > > Thomas Monjalon <thomas.monjalon@6wind.com>
> > > > > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> > > > >
> > > > > On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz"
> > > > > <dev-bounces@dpdk.org on behalf of olivier.matz@6wind.com> wrote:
> > > > >
> > > > > >Hi Beilei,
> > > > > >
> > > > > >On 05/13/2016 10:15 AM, Beilei Xing wrote:
> > > > > >> This patch enables configuring MTU for i40e.
> > > > > >> Since changing MTU needs to reconfigure queue, stop port
> > > > > >> first before configuring MTU.
> > > > > >>
> > > > > >> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > > > > >> ---
> > > > > >> v3 changes:
> > > > > >>  Add frame size with extra I40E_VLAN_TAG_SIZE.
> > > > > >>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> > > > > >>
> > > > > >> v2 changes:
> > > > > >>  If mtu is not within the allowed range, return -EINVAL
> > > > > >> instead of
> > > -EBUSY.
> > > > > >>  Delete rxq reconfigure cause rxq reconfigure will be
> > > > > >> finished in i40e_dev_rx_init.
> > > > > >>
> > > > > >>  drivers/net/i40e/i40e_ethdev.c | 34
> > > > > >> ++++++++++++++++++++++++++++++++++
> > > > > >>  1 file changed, 34 insertions(+)
> > > > > >>
> > > > > >> [...]
> > > > > >> +static int
> > > > > >> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> > > > > >> +	struct i40e_pf *pf =
> > > I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > > > > >> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> > > > > >> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> > > > > >> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> > > > > >> +	int ret = 0;
> > > > > >> +
> > > > > >> +	/* check if mtu is within the allowed range */
> > > > > >> +	if ((mtu < ETHER_MIN_MTU) || (frame_size >
> > > I40E_FRAME_SIZE_MAX))
> > > > > >> +		return -EINVAL;
> > > > > >> +
> > > > > >> +	/* mtu setting is forbidden if port is start */
> > > > > >> +	if (dev_data->dev_started) {
> > > > > >> +		PMD_DRV_LOG(ERR,
> > > > > >> +			    "port %d must be stopped before
> configuration\n",
> > > > > >> +			    dev_data->port_id);
> > > > > >> +		return -ENOTSUP;
> > > > > >> +	}
> > > > > >
> > > > > >I'm not convinced that ENOTSUP is the proper return value here.
> > > > > >It is usually returned when a function is not implemented,
> > > > > >which is not the case here: the function is implemented but is
> > > > > >forbidden because the port is running.
> > > > > >
> > > > > >I saw that Julien commented on your v1 that the return value
> > > > > >should be one of:
> > > > > > - (0) if successful.
> > > > > > - (-ENOTSUP) if operation is not supported.
> > > > > > - (-ENODEV) if *port_id* invalid.
> > > > > > - (-EINVAL) if *mtu* invalid.
> > > > > >
> > > > > >But I think your initial value (-EBUSY) was fine. Maybe it
> > > > > >should be added in the API instead, with the following description:
> > > > > >  (-EBUSY) if the operation is not allowed when the port is
> > > > > >running
> > > > >
> > > > > AFAICT, the same check is not done for other drivers that
> > > > > implement the mac_set op. Wouldn’t it make more sense to have
> > > > > the driver disable the port, reconfigure and re-enable it in
> > > > > this case, instead of returning error code?  If the consensus in
> > > > > DPDK is to have the application disable the port first, we need
> > > > > to enforce this policy across all
> > > devices and clearly document this behavior.
> > > > >
> > > > Hi,
> > > > For ixgbe/igb, there's register for MTU during runtime, but for
> > > > i40e, setting MTU is finished by firmware during configuration.
> > > > There'll be some risk
> > > when disable the port, reconfigure and re-enable the port, so return
> > > error code in this case currently.
> > >
> > > Ok, but then what is the point to introduce mtu_set() for i40e at all?
> > > Let's just leave it unimplemented, as it is right now.
> > > Then users would know that for that device they have to do
> > > dev_stop/dev_configure().
> >
> > Hi Konstantin,
> > The original intention is to reduce ops gap in different PMDs, but
> > unfortunately, setting MTU in i40e couldn’t be finished as in ixgbe,
> 
> Yes, I understand that from your previous explanation.
> 
> > so in the function, we change the configuration and note users that if want to
> configure MTU, stop ports first.
> 
> Well, you can do that now with dev_stop(); dev_configure(), right?

Yes, that's right.

> As I remember, the original intention of mtu_set() was to give user an ability to
> change mtu (whenever possible) without stopping the port.
> If the HW (i40e) doesn't support it - that's fine, we just leave mtu_set() for i40e
> unimplemented.
> 
> > The sequence of > configuring MTU for i40e is: port stop->set MTU->port start.
> > We will add according instruction in document.
> 
> Sorry, but I don’t agree here.
> I think, in that case it is better just to leave mtu_set() unimplemented for i40e.

OK, I'll discuss with maintainer for this case.
Thanks for all your points:)
Beilei

> Konstantin
> 
> > Beilei
> >
> > > Konstantin
> > >
> > > > Thanks for your comments, we'll improve the document later.
> > > >
> > > > > >This would allow the application to take its dispositions to
> > > > > >stop the port and restart it with the proper jumbo_frame argument.
> > > > > >
> > > > > >+CC Thomas which maintains ethdev API.
> > > > > >
> > > > > >
> > > > > >Regards,
> > > > > >Olivier


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

* Re: [dpdk-dev] [PATCH v4] i40e: configure MTU
  2016-06-09 14:23         ` Bruce Richardson
@ 2016-06-23 10:13           ` Bruce Richardson
  2016-06-23 13:37             ` Xing, Beilei
  0 siblings, 1 reply; 22+ messages in thread
From: Bruce Richardson @ 2016-06-23 10:13 UTC (permalink / raw)
  To: Wu, Jingjing; +Cc: Xing, Beilei, dev

On Thu, Jun 09, 2016 at 03:23:43PM +0100, Bruce Richardson wrote:
> On Mon, May 23, 2016 at 01:33:42AM +0000, Wu, Jingjing wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Xing, Beilei
> > > Sent: Friday, May 20, 2016 11:17 PM
> > > To: Wu, Jingjing
> > > Cc: dev@dpdk.org; Xing, Beilei
> > > Subject: [PATCH v4] i40e: configure MTU
> > > 
> > > This patch enables configuring MTU for i40e.
> > > Since changing MTU needs to reconfigure queue, stop port first before
> > > configuring MTU.
> > > 
> > > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > 
> > Acked-by: Jingjing Wu <jingjing.wu@intel.com>
> > 
> Applied to dpdk-next-net/rel_16_07
> 
> /Bruce

It has been brought to my attention that there were still outstanding comments
and issues with v3 of the patch that were never resolved, therefore this patch
may need to be reverted, as I should not have applied it 

Maintainers, please do not ack patches for your components if there are still
unresolved discussion on them! Once the component maintainer acks a patch I
should not have to go back through the whole patch history to determine if it
can be applied or not.

Regards,
/Bruce

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

* Re: [dpdk-dev] [PATCH v4] i40e: configure MTU
  2016-06-23 10:13           ` Bruce Richardson
@ 2016-06-23 13:37             ` Xing, Beilei
  2016-06-23 13:44               ` Bruce Richardson
  0 siblings, 1 reply; 22+ messages in thread
From: Xing, Beilei @ 2016-06-23 13:37 UTC (permalink / raw)
  To: Richardson, Bruce, Wu, Jingjing; +Cc: dev

Hi Bruce,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, June 23, 2016 6:14 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: Xing, Beilei <beilei.xing@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4] i40e: configure MTU
> 
> On Thu, Jun 09, 2016 at 03:23:43PM +0100, Bruce Richardson wrote:
> > On Mon, May 23, 2016 at 01:33:42AM +0000, Wu, Jingjing wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Xing, Beilei
> > > > Sent: Friday, May 20, 2016 11:17 PM
> > > > To: Wu, Jingjing
> > > > Cc: dev@dpdk.org; Xing, Beilei
> > > > Subject: [PATCH v4] i40e: configure MTU
> > > >
> > > > This patch enables configuring MTU for i40e.
> > > > Since changing MTU needs to reconfigure queue, stop port first
> > > > before configuring MTU.
> > > >
> > > > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > >
> > > Acked-by: Jingjing Wu <jingjing.wu@intel.com>
> > >
> > Applied to dpdk-next-net/rel_16_07
> >
> > /Bruce
> 
> It has been brought to my attention that there were still outstanding comments
> and issues with v3 of the patch that were never resolved, therefore this patch
> may need to be reverted, as I should not have applied it
> 
> Maintainers, please do not ack patches for your components if there are still
> unresolved discussion on them! Once the component maintainer acks a patch I
> should not have to go back through the whole patch history to determine if it can
> be applied or not.
> 

Sorry for inconvenience. But here's one correction, this v4 patch is acked and applied on May 23, but v3 comments are added June 17, I think Jingjing always is a responsible maintainer :)

> Regards,
> /Bruce

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

* Re: [dpdk-dev] [PATCH v4] i40e: configure MTU
  2016-06-23 13:37             ` Xing, Beilei
@ 2016-06-23 13:44               ` Bruce Richardson
  0 siblings, 0 replies; 22+ messages in thread
From: Bruce Richardson @ 2016-06-23 13:44 UTC (permalink / raw)
  To: Xing, Beilei; +Cc: Wu, Jingjing, dev

On Thu, Jun 23, 2016 at 02:37:42PM +0100, Xing, Beilei wrote:
> Hi Bruce,
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Thursday, June 23, 2016 6:14 PM
> > To: Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: Xing, Beilei <beilei.xing@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v4] i40e: configure MTU
> > 
> > On Thu, Jun 09, 2016 at 03:23:43PM +0100, Bruce Richardson wrote:
> > > On Mon, May 23, 2016 at 01:33:42AM +0000, Wu, Jingjing wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Xing, Beilei
> > > > > Sent: Friday, May 20, 2016 11:17 PM
> > > > > To: Wu, Jingjing
> > > > > Cc: dev@dpdk.org; Xing, Beilei
> > > > > Subject: [PATCH v4] i40e: configure MTU
> > > > >
> > > > > This patch enables configuring MTU for i40e.
> > > > > Since changing MTU needs to reconfigure queue, stop port first
> > > > > before configuring MTU.
> > > > >
> > > > > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > > >
> > > > Acked-by: Jingjing Wu <jingjing.wu@intel.com>
> > > >
> > > Applied to dpdk-next-net/rel_16_07
> > >
> > > /Bruce
> > 
> > It has been brought to my attention that there were still outstanding comments
> > and issues with v3 of the patch that were never resolved, therefore this patch
> > may need to be reverted, as I should not have applied it
> > 
> > Maintainers, please do not ack patches for your components if there are still
> > unresolved discussion on them! Once the component maintainer acks a patch I
> > should not have to go back through the whole patch history to determine if it can
> > be applied or not.
> > 
> 
> Sorry for inconvenience. But here's one correction, this v4 patch is acked and applied on May 23, but v3 comments are added June 17, I think Jingjing always is a responsible maintainer :)
> 
You are indeed quite right. My apologies to Jingjing, I didn't notice the dates
on the emails, and she was fully within her rights to ack the patch as she did.
Thanks for pointing this out Beilei.

My bad, and apologies again.

Regards,
/Bruce

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

end of thread, other threads:[~2016-06-23 13:45 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-23 11:26 [dpdk-dev] [PATCH] i40e: configure MTU Beilei Xing
2016-04-26  2:00 ` Wu, Jingjing
2016-04-27 11:43 ` Julien Meunier
2016-04-27 15:01   ` Xing, Beilei
2016-04-28  3:19 ` [dpdk-dev] [PATCH V2] " Beilei Xing
2016-05-13  8:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
2016-05-16 12:27     ` Olivier Matz
2016-06-16 17:40       ` Yong Wang
2016-06-16 17:51         ` Yong Wang
2016-06-17  0:03           ` Ananyev, Konstantin
2016-06-20  4:49         ` Xing, Beilei
2016-06-20  4:59           ` Xing, Beilei
2016-06-20  8:05           ` Ananyev, Konstantin
2016-06-20 12:04             ` Xing, Beilei
2016-06-20 12:15               ` Ananyev, Konstantin
2016-06-20 12:46                 ` Xing, Beilei
2016-05-20 15:17     ` [dpdk-dev] [PATCH v4] " Beilei Xing
2016-05-23  1:33       ` Wu, Jingjing
2016-06-09 14:23         ` Bruce Richardson
2016-06-23 10:13           ` Bruce Richardson
2016-06-23 13:37             ` Xing, Beilei
2016-06-23 13:44               ` Bruce Richardson

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).