[2/4] net/bonding: fix LACP fast queue Rx handler

Message ID 1554900829-16180-3-git-send-email-david.marchand@redhat.com
State Accepted
Delegated to: Ferruh Yigit
Headers show
Series
  • lacp rx/tx handlers fixes for bonding pmd
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

David Marchand April 10, 2019, 12:53 p.m.
fast queue Rx burst function is missing checks on promisc and the
slave collecting state.
Define an inline wrapper to have a common base.

Fixes: 112891cd27e5 ("net/bonding: add dedicated HW queues for LACP control")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 73 +++++++++++++---------------------
 1 file changed, 27 insertions(+), 46 deletions(-)

Comments

Chas Williams April 12, 2019, 2:01 p.m. | #1
I should have some time this weekend to run these patches through our
regression system.

On 4/10/19 8:53 AM, David Marchand wrote:
> fast queue Rx burst function is missing checks on promisc and the
> slave collecting state.
> Define an inline wrapper to have a common base.
> 
> Fixes: 112891cd27e5 ("net/bonding: add dedicated HW queues for LACP control")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 73 +++++++++++++---------------------
>   1 file changed, 27 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index c193d6d..989be5c 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -256,48 +256,9 @@
>   	return 0;
>   }
>   
> -static uint16_t
> -bond_ethdev_rx_burst_8023ad_fast_queue(void *queue, struct rte_mbuf **bufs,
> -		uint16_t nb_pkts)
> -{
> -	struct bond_rx_queue *bd_rx_q = (struct bond_rx_queue *)queue;
> -	struct bond_dev_private *internals = bd_rx_q->dev_private;
> -	uint16_t num_rx_total = 0;	/* Total number of received packets */
> -	uint16_t slaves[RTE_MAX_ETHPORTS];
> -	uint16_t slave_count;
> -	uint16_t active_slave;
> -	uint16_t i;
> -
> -	/* Copy slave list to protect against slave up/down changes during tx
> -	 * bursting */
> -	slave_count = internals->active_slave_count;
> -	active_slave = internals->active_slave;
> -	memcpy(slaves, internals->active_slaves,
> -			sizeof(internals->active_slaves[0]) * slave_count);
> -
> -	for (i = 0; i < slave_count && nb_pkts; i++) {
> -		uint16_t num_rx_slave;
> -
> -		/* Read packets from this slave */
> -		num_rx_slave = rte_eth_rx_burst(slaves[active_slave],
> -						bd_rx_q->queue_id,
> -						bufs + num_rx_total, nb_pkts);
> -		num_rx_total += num_rx_slave;
> -		nb_pkts -= num_rx_slave;
> -
> -		if (++active_slave == slave_count)
> -			active_slave = 0;
> -	}
> -
> -	if (++internals->active_slave >= slave_count)
> -		internals->active_slave = 0;
> -
> -	return num_rx_total;
> -}
> -
> -static uint16_t
> -bond_ethdev_rx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
> -		uint16_t nb_pkts)
> +static inline uint16_t
> +rx_burst_8023ad(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts,
> +		bool dedicated_rxq)
>   {
>   	/* Cast to structure, containing bonded device's port id and queue id */
>   	struct bond_rx_queue *bd_rx_q = (struct bond_rx_queue *)queue;
> @@ -357,10 +318,16 @@
>   			hdr = rte_pktmbuf_mtod(bufs[j], struct ether_hdr *);
>   			subtype = ((struct slow_protocol_frame *)hdr)->slow_protocol.subtype;
>   
> -			/* Remove packet from array if it is slow packet or slave is not
> -			 * in collecting state or bonding interface is not in promiscuous
> -			 * mode and packet address does not match. */
> -			if (unlikely(is_lacp_packets(hdr->ether_type, subtype, bufs[j]) ||
> +			/* Remove packet from array if:
> +			 * - it is slow packet but no dedicated rxq is present,
> +			 * - slave is not in collecting state,
> +			 * - bonding interface is not in promiscuous mode and
> +			 *   packet is not multicast and address does not match,
> +			 */
> +			if (unlikely(

The coding style checker doesn't like this:

CHECK:OPEN_ENDED_LINE: Lines should not end with a '('

> +				(!dedicated_rxq &&
> +				 is_lacp_packets(hdr->ether_type, subtype,
> +						 bufs[j])) ||
>   				!collecting ||
>   				(!promisc &&
>   				 !is_multicast_ether_addr(&hdr->d_addr) &&
> @@ -392,6 +359,20 @@
>   	return num_rx_total;
>   }
>   
> +static uint16_t
> +bond_ethdev_rx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
> +		uint16_t nb_pkts)
> +{
> +	return rx_burst_8023ad(queue, bufs, nb_pkts, false);
> +}
> +
> +static uint16_t
> +bond_ethdev_rx_burst_8023ad_fast_queue(void *queue, struct rte_mbuf **bufs,
> +		uint16_t nb_pkts)
> +{
> +	return rx_burst_8023ad(queue, bufs, nb_pkts, true);
> +}
> +
>   #if defined(RTE_LIBRTE_BOND_DEBUG_ALB) || defined(RTE_LIBRTE_BOND_DEBUG_ALB_L1)
>   uint32_t burstnumberRX;
>   uint32_t burstnumberTX;
>
David Marchand April 18, 2019, 7:11 a.m. | #2
Hello Chas,

On Fri, Apr 12, 2019 at 4:02 PM Chas Williams <3chas3@gmail.com> wrote:

> I should have some time this weekend to run these patches through our
> regression system.
>

Did you manage to run this series through your tests system ?


> On 4/10/19 8:53 AM, David Marchand wrote:
> > @@ -357,10 +318,16 @@
> >                       hdr = rte_pktmbuf_mtod(bufs[j], struct ether_hdr
> *);
> >                       subtype = ((struct slow_protocol_frame
> *)hdr)->slow_protocol.subtype;
> >
> > -                     /* Remove packet from array if it is slow packet
> or slave is not
> > -                      * in collecting state or bonding interface is not
> in promiscuous
> > -                      * mode and packet address does not match. */
> > -                     if (unlikely(is_lacp_packets(hdr->ether_type,
> subtype, bufs[j]) ||
> > +                     /* Remove packet from array if:
> > +                      * - it is slow packet but no dedicated rxq is
> present,
> > +                      * - slave is not in collecting state,
> > +                      * - bonding interface is not in promiscuous mode
> and
> > +                      *   packet is not multicast and address does not
> match,
> > +                      */
> > +                     if (unlikely(
>
> The coding style checker doesn't like this:
>
> CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
>

Yes, I had seen this warning, just found it easier to read this way.



> > +                             (!dedicated_rxq &&
> > +                              is_lacp_packets(hdr->ether_type, subtype,
> > +                                              bufs[j])) ||
> >                               !collecting ||
> >                               (!promisc &&
> >                                !is_multicast_ether_addr(&hdr->d_addr) &&
>
>
Chas Williams April 18, 2019, 10:50 p.m. | #3
On 4/18/19 3:11 AM, David Marchand wrote:
> Hello Chas,
> 
> On Fri, Apr 12, 2019 at 4:02 PM Chas Williams <3chas3@gmail.com 
> <mailto:3chas3@gmail.com>> wrote:
> 
>     I should have some time this weekend to run these patches through our
>     regression system.
> 
> 
> Did you manage to run this series through your tests system ?

There were some other issues over the weekend. Hopefully this one.

> 
> 
>     On 4/10/19 8:53 AM, David Marchand wrote:
>      > @@ -357,10 +318,16 @@
>      >                       hdr = rte_pktmbuf_mtod(bufs[j], struct
>     ether_hdr *);
>      >                       subtype = ((struct slow_protocol_frame
>     *)hdr)->slow_protocol.subtype;
>      >
>      > -                     /* Remove packet from array if it is slow
>     packet or slave is not
>      > -                      * in collecting state or bonding interface
>     is not in promiscuous
>      > -                      * mode and packet address does not match. */
>      > -                     if
>     (unlikely(is_lacp_packets(hdr->ether_type, subtype, bufs[j]) ||
>      > +                     /* Remove packet from array if:
>      > +                      * - it is slow packet but no dedicated rxq
>     is present,
>      > +                      * - slave is not in collecting state,
>      > +                      * - bonding interface is not in
>     promiscuous mode and
>      > +                      *   packet is not multicast and address
>     does not match,
>      > +                      */
>      > +                     if (unlikely(
> 
>     The coding style checker doesn't like this:
> 
>     CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
> 
> 
> Yes, I had seen this warning, just found it easier to read this way.
> 
> 
> 
>      > +                             (!dedicated_rxq &&
>      > +                              is_lacp_packets(hdr->ether_type,
>     subtype,
>      > +                                              bufs[j])) ||
>      >                               !collecting ||
>      >                               (!promisc &&
>      >                               
>     !is_multicast_ether_addr(&hdr->d_addr) &&
> 
> 
> 
> -- 
> David Marchand
David Marchand May 16, 2019, 9:12 a.m. | #4
Hello Chas,

On Fri, Apr 19, 2019 at 12:50 AM Chas Williams <3chas3@gmail.com> wrote:

> On 4/18/19 3:11 AM, David Marchand wrote:
> > Hello Chas,
> >
> > On Fri, Apr 12, 2019 at 4:02 PM Chas Williams <3chas3@gmail.com
> > <mailto:3chas3@gmail.com>> wrote:
> >
> >     I should have some time this weekend to run these patches through our
> >     regression system.
> >
> >
> > Did you manage to run this series through your tests system ?
>
> There were some other issues over the weekend. Hopefully this one.
>


Any update ?
Thanks.
Ferruh Yigit July 2, 2019, 3:01 p.m. | #5
On 5/16/2019 10:12 AM, David Marchand wrote:
> Hello Chas,
> 
> On Fri, Apr 19, 2019 at 12:50 AM Chas Williams <3chas3@gmail.com> wrote:
> 
>> On 4/18/19 3:11 AM, David Marchand wrote:
>>> Hello Chas,
>>>
>>> On Fri, Apr 12, 2019 at 4:02 PM Chas Williams <3chas3@gmail.com
>>> <mailto:3chas3@gmail.com>> wrote:
>>>
>>>     I should have some time this weekend to run these patches through our
>>>     regression system.
>>>
>>>
>>> Did you manage to run this series through your tests system ?
>>
>> There were some other issues over the weekend. Hopefully this one.
>>
> 
> 
> Any update ?
> Thanks.
> 

Reminder of this patchset, if there is no objection in next a few days I will
merge them.

Thanks,
ferruh
Chas Williams Aug. 14, 2019, 1:43 a.m. | #6
On 7/2/19 11:01 AM, Ferruh Yigit wrote:
> On 5/16/2019 10:12 AM, David Marchand wrote:
>> Hello Chas,
>>
>> On Fri, Apr 19, 2019 at 12:50 AM Chas Williams <3chas3@gmail.com> wrote:
>>
>>> On 4/18/19 3:11 AM, David Marchand wrote:
>>>> Hello Chas,
>>>>
>>>> On Fri, Apr 12, 2019 at 4:02 PM Chas Williams <3chas3@gmail.com
>>>> <mailto:3chas3@gmail.com>> wrote:
>>>>
>>>>      I should have some time this weekend to run these patches through our
>>>>      regression system.
>>>>
>>>>
>>>> Did you manage to run this series through your tests system ?
>>>
>>> There were some other issues over the weekend. Hopefully this one.
>>>
>>
>>
>> Any update ?
>> Thanks.
>>
> 
> Reminder of this patchset, if there is no objection in next a few days I will
> merge them.
> 
> Thanks,
> ferruh
> 

OK, I was able to get a clean run for these patches through our regression
system.  I didn't see any failures with these patches applied. Consider
the following:

David Marchand (4):
   net/bonding: fix oob access in LACP mode when sending many packets
   net/bonding: fix LACP fast queue Rx handler
   net/bonding: fix unicast packets filtering when not in promisc
   net/bonding: prefer allmulti to promisc for LACP

Signed-off-by: Chas Williams <chas3@att.com>

Sorry this took so long!
David Marchand Aug. 19, 2019, 9:41 a.m. | #7
On Wed, Aug 14, 2019 at 3:43 AM Chas Williams <3chas3@gmail.com> wrote:
>
>
>
>
> On 7/2/19 11:01 AM, Ferruh Yigit wrote:
> > On 5/16/2019 10:12 AM, David Marchand wrote:
> >> Hello Chas,
> >>
> >> On Fri, Apr 19, 2019 at 12:50 AM Chas Williams <3chas3@gmail.com> wrote:
> >>
> >>> On 4/18/19 3:11 AM, David Marchand wrote:
> >>>> Hello Chas,
> >>>>
> >>>> On Fri, Apr 12, 2019 at 4:02 PM Chas Williams <3chas3@gmail.com
> >>>> <mailto:3chas3@gmail.com>> wrote:
> >>>>
> >>>>      I should have some time this weekend to run these patches through our
> >>>>      regression system.
> >>>>
> >>>>
> >>>> Did you manage to run this series through your tests system ?
> >>>
> >>> There were some other issues over the weekend. Hopefully this one.
> >>>
> >>
> >>
> >> Any update ?
> >> Thanks.
> >>
> >
> > Reminder of this patchset, if there is no objection in next a few days I will
> > merge them.
> >
> > Thanks,
> > ferruh
> >
>
> OK, I was able to get a clean run for these patches through our regression
> system.  I didn't see any failures with these patches applied. Consider
> the following:
>
> David Marchand (4):
>    net/bonding: fix oob access in LACP mode when sending many packets
>    net/bonding: fix LACP fast queue Rx handler
>    net/bonding: fix unicast packets filtering when not in promisc
>    net/bonding: prefer allmulti to promisc for LACP
>
> Signed-off-by: Chas Williams <chas3@att.com>
>
> Sorry this took so long!

Ah, cool.
Thanks a lot Chas.

Patch

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index c193d6d..989be5c 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -256,48 +256,9 @@ 
 	return 0;
 }
 
-static uint16_t
-bond_ethdev_rx_burst_8023ad_fast_queue(void *queue, struct rte_mbuf **bufs,
-		uint16_t nb_pkts)
-{
-	struct bond_rx_queue *bd_rx_q = (struct bond_rx_queue *)queue;
-	struct bond_dev_private *internals = bd_rx_q->dev_private;
-	uint16_t num_rx_total = 0;	/* Total number of received packets */
-	uint16_t slaves[RTE_MAX_ETHPORTS];
-	uint16_t slave_count;
-	uint16_t active_slave;
-	uint16_t i;
-
-	/* Copy slave list to protect against slave up/down changes during tx
-	 * bursting */
-	slave_count = internals->active_slave_count;
-	active_slave = internals->active_slave;
-	memcpy(slaves, internals->active_slaves,
-			sizeof(internals->active_slaves[0]) * slave_count);
-
-	for (i = 0; i < slave_count && nb_pkts; i++) {
-		uint16_t num_rx_slave;
-
-		/* Read packets from this slave */
-		num_rx_slave = rte_eth_rx_burst(slaves[active_slave],
-						bd_rx_q->queue_id,
-						bufs + num_rx_total, nb_pkts);
-		num_rx_total += num_rx_slave;
-		nb_pkts -= num_rx_slave;
-
-		if (++active_slave == slave_count)
-			active_slave = 0;
-	}
-
-	if (++internals->active_slave >= slave_count)
-		internals->active_slave = 0;
-
-	return num_rx_total;
-}
-
-static uint16_t
-bond_ethdev_rx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
-		uint16_t nb_pkts)
+static inline uint16_t
+rx_burst_8023ad(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts,
+		bool dedicated_rxq)
 {
 	/* Cast to structure, containing bonded device's port id and queue id */
 	struct bond_rx_queue *bd_rx_q = (struct bond_rx_queue *)queue;
@@ -357,10 +318,16 @@ 
 			hdr = rte_pktmbuf_mtod(bufs[j], struct ether_hdr *);
 			subtype = ((struct slow_protocol_frame *)hdr)->slow_protocol.subtype;
 
-			/* Remove packet from array if it is slow packet or slave is not
-			 * in collecting state or bonding interface is not in promiscuous
-			 * mode and packet address does not match. */
-			if (unlikely(is_lacp_packets(hdr->ether_type, subtype, bufs[j]) ||
+			/* Remove packet from array if:
+			 * - it is slow packet but no dedicated rxq is present,
+			 * - slave is not in collecting state,
+			 * - bonding interface is not in promiscuous mode and
+			 *   packet is not multicast and address does not match,
+			 */
+			if (unlikely(
+				(!dedicated_rxq &&
+				 is_lacp_packets(hdr->ether_type, subtype,
+						 bufs[j])) ||
 				!collecting ||
 				(!promisc &&
 				 !is_multicast_ether_addr(&hdr->d_addr) &&
@@ -392,6 +359,20 @@ 
 	return num_rx_total;
 }
 
+static uint16_t
+bond_ethdev_rx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
+		uint16_t nb_pkts)
+{
+	return rx_burst_8023ad(queue, bufs, nb_pkts, false);
+}
+
+static uint16_t
+bond_ethdev_rx_burst_8023ad_fast_queue(void *queue, struct rte_mbuf **bufs,
+		uint16_t nb_pkts)
+{
+	return rx_burst_8023ad(queue, bufs, nb_pkts, true);
+}
+
 #if defined(RTE_LIBRTE_BOND_DEBUG_ALB) || defined(RTE_LIBRTE_BOND_DEBUG_ALB_L1)
 uint32_t burstnumberRX;
 uint32_t burstnumberTX;