[dpdk-dev] [PATCH] bond: inherit maximum rx packet length

[dpdk-dev] [PATCH] bond: inherit maximum rx packet length

[Eric Kinzie]

  Instead of a hard-coded maximum receive length, allow the bond interface
  to inherit this limit from the first slave added.  This allows
  an application that uses jumbo frames to pass realistic values to
  rte_eth_dev_configure without causing an error.

Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
---
 drivers/net/bonding/rte_eth_bond_api.c     |    4 ++++
 drivers/net/bonding/rte_eth_bond_pmd.c     |    2 +-
 drivers/net/bonding/rte_eth_bond_private.h |    2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index e9247b5..b763b37 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -247,6 +247,7 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 	internals->active_slave_count = 0;
 	internals->rx_offload_capa = 0;
 	internals->tx_offload_capa = 0;
+	internals->max_rx_pktlen = (uint32_t)2048;
 
 	/* Initially allow to choose any offload type */
 	internals->flow_type_rss_offloads = ETH_RSS_PROTO_MASK;
@@ -365,6 +366,9 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa = dev_info.tx_offload_capa;
 		internals->flow_type_rss_offloads = dev_info.flow_type_rss_offloads;
 
+		/* Inherit first slave's max rx packet size */
+		internals->max_rx_pktlen = dev_info.max_rx_pktlen;
+
 	} else {
 		/* Check slave link properties are supported if props are set,
 		 * all slaves must be the same */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 54788cf..189fb47 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1650,7 +1650,7 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 
 	dev_info->max_mac_addrs = 1;
 
-	dev_info->max_rx_pktlen = (uint32_t)2048;
+	dev_info->max_rx_pktlen = internals->max_rx_pktlen;
 
 	dev_info->max_rx_queues = (uint16_t)128;
 	dev_info->max_tx_queues = (uint16_t)512;
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 8312397..79ca69d 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -169,6 +169,8 @@ struct bond_dev_private {
 
 	struct rte_kvargs *kvlist;
 	uint8_t slave_update_idx;
+
+	uint32_t max_rx_pktlen;
 };
 
 extern const struct eth_dev_ops default_dev_ops;
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH] bond: inherit maximum rx packet length

[Declan Doherty]

On 14/04/16 18:23, Eric Kinzie wrote:
>    Instead of a hard-coded maximum receive length, allow the bond interface
>    to inherit this limit from the first slave added.  This allows
>    an application that uses jumbo frames to pass realistic values to
>    rte_eth_dev_configure without causing an error.
>
> Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
> ---
...
>

Hey Eric, just one small thing, I think it probably makes sense to 
return the max rx pktlen for all slaves, so as we add each slave just 
check if that the slave being value is larger than the current value.

@@ -385,6 +389,10 @@ __eth_bond_slave_add_lock_free(uint8_t 
bonded_port_id, uint8_t slave_port_id)
                 internals->tx_offload_capa &= dev_info.tx_offload_capa;
                 internals->flow_type_rss_offloads &= 
dev_info.flow_type_rss_offloads;

+               /* If new slave's max rx packet size is larger than 
current value then override */
+               if (dev_info.max_rx_pktlen > internals->max_rx_pktlen)
+                       internals->max_rx_pktlen = dev_info.max_rx_pktlen;
+

Declan

Re: [dpdk-dev] [PATCH] bond: inherit maximum rx packet length

[Eric Kinzie]

On Tue Apr 26 11:51:53 +0100 2016, Declan Doherty wrote:
> On 14/04/16 18:23, Eric Kinzie wrote:
> >   Instead of a hard-coded maximum receive length, allow the bond interface
> >   to inherit this limit from the first slave added.  This allows
> >   an application that uses jumbo frames to pass realistic values to
> >   rte_eth_dev_configure without causing an error.
> >
> >Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
> >---
> ...
> >
> 
> Hey Eric, just one small thing, I think it probably makes sense to
> return the max rx pktlen for all slaves, so as we add each slave
> just check if that the slave being value is larger than the current
> value.
> 
> @@ -385,6 +389,10 @@ __eth_bond_slave_add_lock_free(uint8_t
> bonded_port_id, uint8_t slave_port_id)
>                 internals->tx_offload_capa &= dev_info.tx_offload_capa;
>                 internals->flow_type_rss_offloads &=
> dev_info.flow_type_rss_offloads;
> 
> +               /* If new slave's max rx packet size is larger than
> current value then override */
> +               if (dev_info.max_rx_pktlen > internals->max_rx_pktlen)
> +                       internals->max_rx_pktlen = dev_info.max_rx_pktlen;
> +
> 
> Declan

Declan, I sent an updated patch but now release that I mis-read your
comments.  Is it a good idea to change the value once it's been set?
My patch now refuses to add a slave with a pktlen value that's smaller
than that of the first slave.

Eric

[dpdk-dev] [PATCH v2] bond: inherit maximum rx packet length

[Eric Kinzie]

v2 changes:
 - remove type cast on constant
 - check max_rx_pktlen when adding a slave to make sure it is >= max
   packet length of existing slave interfaces

Eric Kinzie (1):
  bond: inherit maximum rx packet length

 drivers/net/bonding/rte_eth_bond_api.c     | 12 +++++++++++-
 drivers/net/bonding/rte_eth_bond_pmd.c     |  2 +-
 drivers/net/bonding/rte_eth_bond_private.h |  2 ++
 3 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH v2] bond: inherit maximum rx packet length

[Eric Kinzie]

  Instead of a hard-coded maximum receive length, allow the bond interface
  to inherit this limit from the first slave added.  This allows
  an application that uses jumbo frames to pass realistic values to
  rte_eth_dev_configure without causing an error.

Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
---
 drivers/net/bonding/rte_eth_bond_api.c     | 12 +++++++++++-
 drivers/net/bonding/rte_eth_bond_pmd.c     |  2 +-
 drivers/net/bonding/rte_eth_bond_private.h |  2 ++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index e9247b5..acc1c32 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -247,6 +247,7 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 	internals->active_slave_count = 0;
 	internals->rx_offload_capa = 0;
 	internals->tx_offload_capa = 0;
+	internals->max_rx_pktlen = 2048;
 
 	/* Initially allow to choose any offload type */
 	internals->flow_type_rss_offloads = ETH_RSS_PROTO_MASK;
@@ -331,9 +332,15 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 
 	/* Add slave details to bonded device */
 	slave_eth_dev->data->dev_flags |= RTE_ETH_DEV_BONDED_SLAVE;
-	slave_add(internals, slave_eth_dev);
 
 	rte_eth_dev_info_get(slave_port_id, &dev_info);
+	if (dev_info.max_rx_pktlen < internals->max_rx_pktlen) {
+		RTE_BOND_LOG(ERR, "Slave (port %u) max_rx_pktlen too small",
+			     slave_port_id);
+		return -1;
+	}
+
+	slave_add(internals, slave_eth_dev);
 
 	/* We need to store slaves reta_size to be able to synchronize RETA for all
 	 * slave devices even if its sizes are different.
@@ -365,6 +372,9 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa = dev_info.tx_offload_capa;
 		internals->flow_type_rss_offloads = dev_info.flow_type_rss_offloads;
 
+		/* Inherit first slave's max rx packet size */
+		internals->max_rx_pktlen = dev_info.max_rx_pktlen;
+
 	} else {
 		/* Check slave link properties are supported if props are set,
 		 * all slaves must be the same */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 54788cf..189fb47 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1650,7 +1650,7 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 
 	dev_info->max_mac_addrs = 1;
 
-	dev_info->max_rx_pktlen = (uint32_t)2048;
+	dev_info->max_rx_pktlen = internals->max_rx_pktlen;
 
 	dev_info->max_rx_queues = (uint16_t)128;
 	dev_info->max_tx_queues = (uint16_t)512;
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 8312397..79ca69d 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -169,6 +169,8 @@ struct bond_dev_private {
 
 	struct rte_kvargs *kvlist;
 	uint8_t slave_update_idx;
+
+	uint32_t max_rx_pktlen;
 };
 
 extern const struct eth_dev_ops default_dev_ops;
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2] bond: inherit maximum rx packet length

[Declan Doherty]

CC'ing follow up conversation which I accidentally took of list.

-------- Forwarded Message --------
Subject: Re: [dpdk-dev] [PATCH] bond: inherit maximum rx packet length
Date: Wed, 4 May 2016 14:11:53 -0400
From: Eric Kinzie <ehkinzie@gmail.com>
To: Declan Doherty <declan.doherty@intel.com>

On Wed May 04 14:53:26 +0100 2016, Declan Doherty wrote:
 > On 29/04/16 22:36, Eric Kinzie wrote:
 > >On Tue Apr 26 11:51:53 +0100 2016, Declan Doherty wrote:
 > >>On 14/04/16 18:23, Eric Kinzie wrote:
 > >>>  Instead of a hard-coded maximum receive length, allow the bond 
interface
 > >>>  to inherit this limit from the first slave added.  This allows
 > >>>  an application that uses jumbo frames to pass realistic values to
 > >>>  rte_eth_dev_configure without causing an error.
 > >>>
 > >>>Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
 > >>>---
 > >>...
 > >>>
 > >>
 > >>Hey Eric, just one small thing, I think it probably makes sense to
 > >>return the max rx pktlen for all slaves, so as we add each slave
 > >>just check if that the slave being value is larger than the current
 > >>value.
 > >>
 > >>@@ -385,6 +389,10 @@ __eth_bond_slave_add_lock_free(uint8_t
 > >>bonded_port_id, uint8_t slave_port_id)
 > >>                internals->tx_offload_capa &= dev_info.tx_offload_capa;
 > >>                internals->flow_type_rss_offloads &=
 > >>dev_info.flow_type_rss_offloads;
 > >>
 > >>+               /* If new slave's max rx packet size is larger than
 > >>current value then override */
 > >>+               if (dev_info.max_rx_pktlen > internals->max_rx_pktlen)
 > >>+                       internals->max_rx_pktlen = 
dev_info.max_rx_pktlen;
 > >>+
 > >>
 > >>Declan
 > >
 > >Declan, I sent an updated patch but now release that I mis-read your
 > >comments.  Is it a good idea to change the value once it's been set?
 > >My patch now refuses to add a slave with a pktlen value that's smaller
 > >than that of the first slave.
 > >
 > >Eric
 > >
 >
 > Hey Eric,
 >
 > actually I think you're right, we can't change the value dynamically
 > once the bonded device has been configured (maybe gating on
 > start/stop would be sufficient), but I think we shouldn't explicitly
 > gate on the first slave added, as we may be bonding multiple slaves
 > each of which could have different max_rx_pktlens. Prior to calling
 > dev_configure on the bonded device it should be possible to add any
 > slave with any max_rx_pktlen and inherit the minimum value, but once
 > the bonded device has been configured we would refuse a slave with a
 > max_rx_pktlen value which is smaller than the current value. I think
 > this should then satisfy that all slaves meet the minimum
 > requirements published by the bonded device?
 >

Hi, Declan.  I think that would work out ok.  I'll write something to
that effect and send another version of the patch.

[dpdk-dev] [PATCH v3] bond: inherit maximum rx packet length

[Eric Kinzie]

v2 changes:
 - remove type cast on constant
 - check max_rx_pktlen when adding a slave to make sure it is >= max
   packet length of existing slave interfaces

v3 changes:
 - allow slaves with any max rx packet length to be added to the bonding
   interface until it is configured.  After bond_ethdev_configure()
   only slaves with max pktlen greater than that of the bond device
   can be added.  The bond device assumes the smallest of the slave max
   pktlen values.
 - reset bond device's max_rx_pktlen when last slave removed

Eric Kinzie (1):
  bond: inherit maximum rx packet length

 drivers/net/bonding/rte_eth_bond_api.c     | 18 +++++++++++++++++-
 drivers/net/bonding/rte_eth_bond_pmd.c     |  6 +++++-
 drivers/net/bonding/rte_eth_bond_private.h |  3 +++
 3 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH v3] bond: inherit maximum rx packet length

[Eric Kinzie]

  Instead of a hard-coded maximum receive length, allow the bond interface
  to inherit this limit from the slave interfaces.  This allows
  an application that uses jumbo frames to pass realistic values to
  rte_eth_dev_configure without causing an error.

  Before the bond interface is configured, allow slaves with any
  max_rx_pktlen to be added and remember the lowest of these values as
  a candidate value.  During dev_configure, set the bond device's
  max_rx_pktlen to the candidate value.  After this point only slaves
  with a max_rx_pktlen greater or equal to that of the bonding device
  can be added.

  If all slaves are removed, the bond device's pktlen is cleared.

Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
---
 drivers/net/bonding/rte_eth_bond_api.c     | 18 +++++++++++++++++-
 drivers/net/bonding/rte_eth_bond_pmd.c     |  6 +++++-
 drivers/net/bonding/rte_eth_bond_private.h |  3 +++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index e9247b5..aa0be5d 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -247,6 +247,8 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 	internals->active_slave_count = 0;
 	internals->rx_offload_capa = 0;
 	internals->tx_offload_capa = 0;
+	internals->candidate_max_rx_pktlen = 0;
+	internals->max_rx_pktlen = 0;
 
 	/* Initially allow to choose any offload type */
 	internals->flow_type_rss_offloads = ETH_RSS_PROTO_MASK;
@@ -331,9 +333,15 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 
 	/* Add slave details to bonded device */
 	slave_eth_dev->data->dev_flags |= RTE_ETH_DEV_BONDED_SLAVE;
-	slave_add(internals, slave_eth_dev);
 
 	rte_eth_dev_info_get(slave_port_id, &dev_info);
+	if (dev_info.max_rx_pktlen < internals->max_rx_pktlen) {
+		RTE_BOND_LOG(ERR, "Slave (port %u) max_rx_pktlen too small",
+			     slave_port_id);
+		return -1;
+	}
+
+	slave_add(internals, slave_eth_dev);
 
 	/* We need to store slaves reta_size to be able to synchronize RETA for all
 	 * slave devices even if its sizes are different.
@@ -365,6 +373,9 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa = dev_info.tx_offload_capa;
 		internals->flow_type_rss_offloads = dev_info.flow_type_rss_offloads;
 
+		/* Inherit first slave's max rx packet size */
+		internals->candidate_max_rx_pktlen = dev_info.max_rx_pktlen;
+
 	} else {
 		/* Check slave link properties are supported if props are set,
 		 * all slaves must be the same */
@@ -391,6 +402,9 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		if (internals->reta_size > dev_info.reta_size)
 			internals->reta_size = dev_info.reta_size;
 
+		if (!internals->max_rx_pktlen
+		    && dev_info.max_rx_pktlen < internals->candidate_max_rx_pktlen)
+			internals->candidate_max_rx_pktlen = dev_info.max_rx_pktlen;
 	}
 
 	bonded_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf &=
@@ -536,6 +550,8 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa = 0;
 		internals->flow_type_rss_offloads = ETH_RSS_PROTO_MASK;
 		internals->reta_size = 0;
+		internals->candidate_max_rx_pktlen = 0;
+		internals->max_rx_pktlen = 0;
 	}
 	return 0;
 }
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 54788cf..0d39191 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1650,7 +1650,8 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 
 	dev_info->max_mac_addrs = 1;
 
-	dev_info->max_rx_pktlen = (uint32_t)2048;
+	dev_info->max_rx_pktlen = internals->candidate_max_rx_pktlen ?
+				  internals->candidate_max_rx_pktlen : 2048;
 
 	dev_info->max_rx_queues = (uint16_t)128;
 	dev_info->max_tx_queues = (uint16_t)512;
@@ -2294,6 +2295,9 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
 		}
 	}
 
+	/* set the max_rx_pktlen */
+	internals->max_rx_pktlen = internals->candidate_max_rx_pktlen;
+
 	/*
 	 * if no kvlist, it means that this bonded device has been created
 	 * through the bonding api.
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 8312397..2bdc9ef 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -169,6 +169,9 @@ struct bond_dev_private {
 
 	struct rte_kvargs *kvlist;
 	uint8_t slave_update_idx;
+
+	uint32_t candidate_max_rx_pktlen;
+	uint32_t max_rx_pktlen;
 };
 
 extern const struct eth_dev_ops default_dev_ops;
-- 
2.1.4

Re: [dpdk-dev] [PATCH v3] bond: inherit maximum rx packet length

[Declan Doherty]

On 07/05/16 04:45, Eric Kinzie wrote:
>   Instead of a hard-coded maximum receive length, allow the bond interface
>   to inherit this limit from the slave interfaces.  This allows
>   an application that uses jumbo frames to pass realistic values to
>   rte_eth_dev_configure without causing an error.
>
>   Before the bond interface is configured, allow slaves with any
>   max_rx_pktlen to be added and remember the lowest of these values as
>   a candidate value.  During dev_configure, set the bond device's
>   max_rx_pktlen to the candidate value.  After this point only slaves
>   with a max_rx_pktlen greater or equal to that of the bonding device
>   can be added.
>
>   If all slaves are removed, the bond device's pktlen is cleared.
>
> Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
> ---
>
....
>

Acked-by: Declan Dohetry <declan.doherty@intel.com>

Re: [dpdk-dev] [PATCH v3] bond: inherit maximum rx packet length

[Bruce Richardson]

On Fri, Jun 17, 2016 at 03:30:03PM +0100, Declan Doherty wrote:
> On 07/05/16 04:45, Eric Kinzie wrote:
> >  Instead of a hard-coded maximum receive length, allow the bond interface
> >  to inherit this limit from the slave interfaces.  This allows
> >  an application that uses jumbo frames to pass realistic values to
> >  rte_eth_dev_configure without causing an error.
> >
> >  Before the bond interface is configured, allow slaves with any
> >  max_rx_pktlen to be added and remember the lowest of these values as
> >  a candidate value.  During dev_configure, set the bond device's
> >  max_rx_pktlen to the candidate value.  After this point only slaves
> >  with a max_rx_pktlen greater or equal to that of the bonding device
> >  can be added.
> >
> >  If all slaves are removed, the bond device's pktlen is cleared.
> >
> >Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
> >---
> >
> ....
> >
> 
> Acked-by: Declan Dohetry <declan.doherty@intel.com>

Applied to dpdk-next-net/rel_16_07, with one checkpatch fix and with title
corrected to pass check with check-git-log.sh

/Bruce