On 3/12/2023 10:54 AM, Ivan Malov wrote: > The driver supports representor functionality. In it, > packets coming from VFs to the dedicated back-end Rx > queue get demultiplexed into front-end Rx queues of > representor ethdevs as per the per-packet metadata > indicating logical HW ingress ports. On transmit, > packets are provided with symmetrical metadata > by front-end Tx queues, and the back-end queue > transforms the data into so-called Tx override > descriptors. These let the packets bypass flow > lookup and go directly to the represented VFs. > > However, in the Rx part, the driver extracts > the said metadata on every HW Rx queue, that > is, not just on the one used by representors. > Doing so leads to a buggy behaviour. It is > revealed by operating testpmd as follows: > > dpdk-testpmd -a 0000:c6:00.0 -a 0000:c6:00.1 -- -i > > testpmd> flow create 0 transfer pattern port_representor \ > port_id is 0 / end actions port_representor port_id 1 / end > Flow rule #0 created > > testpmd> set fwd io > testpmd> start tx_first > > testpmd> flow destroy 0 rule 0 > Flow rule #0 destroyed > > testpmd> stop > > ---------------------- Forward statistics for port 0 ----------------- > RX-packets: 19196498 RX-dropped: 0 RX-total: 19196498 > TX-packets: 19196535 TX-dropped: 0 TX-total: 19196535 > ----------------------------------------------------------------------- > > ---------------------- Forward statistics for port 1 ----------------- > RX-packets: 19196503 RX-dropped: 0 RX-total: 19196503 > TX-packets: 19196530 TX-dropped: 0 TX-total: 19196530 > ----------------------------------------------------------------------- > > In this scenario, two physical functions of the adapter > do not have any corresponding "back-to-back" forwarder > on peer host. Packets transmitted from port 0 can only > be forwarded to port 1 by means of a special flow rule. > > The flow rule indeed works, but destroying it does not > stop forwarding. Port statistics carry on incrementing. > > Also, it is apparent that forwarding in the opposite > direction must not have worked in this case as the > flow is meant to target only one of the directions. > > Because of the bug, the first 32 mbufs received > as a result of the flow rule operation have the > said metadata present. In io mode, testpmd does > not tamper with mbufs and passes them directly > to transmit path, so this data remains in them > instructing the PMD to override destinations > of the packets via Tx option descriptors. > > Expected behaviour is as follows: > > ---------------------- Forward statistics for port 0 ----------------- > RX-packets: 0 RX-dropped: 0 RX-total: 0 > TX-packets: 15787496 TX-dropped: 0 TX-total: 15787496 > ----------------------------------------------------------------------- > > ---------------------- Forward statistics for port 1 ----------------- > RX-packets: 15787464 RX-dropped: 0 RX-total: 15787464 > TX-packets: 32 TX-dropped: 0 TX-total: 32 > ----------------------------------------------------------------------- > > These figures show the rule work only for one direction. > Also, removing the flow shall cause forwarding to cease. > > Provided patch fixes the bug accordingly. > > Fixes: d0f981a3efd8 ("net/sfc: handle ingress mport in EF100 Rx prefix") > Cc: sta...@dpdk.org > > Signed-off-by: Ivan Malov <ivan.ma...@arknetworks.am> > Reviewed-by: Andy Moreton <amore...@xilinx.com> > --- > v3: extra rework after review feedback > v2: address community review notes > > drivers/net/sfc/sfc_dp_rx.h | 1 + > drivers/net/sfc/sfc_ef100_rx.c | 18 ++++++++++++++---- > drivers/net/sfc/sfc_rx.c | 3 +++ > 3 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/sfc/sfc_dp_rx.h b/drivers/net/sfc/sfc_dp_rx.h > index 246adbd87c..8a504bdcf1 100644 > --- a/drivers/net/sfc/sfc_dp_rx.h > +++ b/drivers/net/sfc/sfc_dp_rx.h > @@ -69,6 +69,7 @@ struct sfc_dp_rx_qcreate_info { > /** Receive queue flags initializer */ > unsigned int flags; > #define SFC_RXQ_FLAG_RSS_HASH 0x1 > +#define SFC_RXQ_FLAG_INGRESS_MPORT 0x2 > > /** Rx queue size */ > unsigned int rxq_entries; > diff --git a/drivers/net/sfc/sfc_ef100_rx.c b/drivers/net/sfc/sfc_ef100_rx.c > index b7e3397f77..e323156a26 100644 > --- a/drivers/net/sfc/sfc_ef100_rx.c > +++ b/drivers/net/sfc/sfc_ef100_rx.c > @@ -823,6 +823,9 @@ sfc_ef100_rx_qcreate(uint16_t port_id, uint16_t queue_id, > if (rxq->nic_dma_info->nb_regions > 0) > rxq->flags |= SFC_EF100_RXQ_NIC_DMA_MAP; > > + if (info->flags & SFC_RXQ_FLAG_INGRESS_MPORT) > + rxq->flags |= SFC_EF100_RXQ_INGRESS_MPORT; > + > sfc_ef100_rx_debug(rxq, "RxQ doorbell is %p", rxq->doorbell); > > *dp_rxqp = &rxq->dp; > @@ -889,11 +892,18 @@ sfc_ef100_rx_qstart(struct sfc_dp_rxq *dp_rxq, unsigned > int evq_read_ptr, > else > rxq->flags &= ~SFC_EF100_RXQ_USER_MARK; > > + > + /* > + * At the moment, this feature is used only > + * by the representor proxy Rx queue and is > + * essential for representor support, so if > + * it has been requested but is unsupported, > + * point this inconsistency out to the user. > + */ > if ((unsup_rx_prefix_fields & > - (1U << EFX_RX_PREFIX_FIELD_INGRESS_MPORT)) == 0) > - rxq->flags |= SFC_EF100_RXQ_INGRESS_MPORT; > - else > - rxq->flags &= ~SFC_EF100_RXQ_INGRESS_MPORT; > + (1U << EFX_RX_PREFIX_FIELD_INGRESS_MPORT)) && > + (rxq->flags & SFC_EF100_RXQ_INGRESS_MPORT)) > + return ENOTSUP; > > if ((unsup_rx_prefix_fields & > (1U << EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI)) == 0)
Hi Ivan, The patch doesn't apply cleanly, and I can't find 'EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI' in the repo. Can you please rebase the patch on top of latest 'next-net' and send a new version? So that CI can run. If there won't be any functional change, feel free to keep Andrew's ack in next version.