[2/4] net/bonding: fix LACP fast queue Rx handler
Checks
Commit Message
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
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;
>
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) &&
>
>
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
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.
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
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!
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.
@@ -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;