From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id BF55A41E7C for ; Sun, 12 Mar 2023 08:59:25 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BB69441611; Sun, 12 Mar 2023 08:59:25 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 24DD540395; Sun, 12 Mar 2023 08:59:23 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 2A1E07B; Sun, 12 Mar 2023 10:59:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 2A1E07B DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1678607962; bh=fVnAhpyPOfOlY5uH927wMxpJhbNy+GJcNGPdiv/okA0=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=gF6nUr9rHJmItddvikHFYrz+C9NcHJQL32ST1Z4fOPx6QUQzCAYjDaklz9l0EWVK2 gDstIyvcW4pu8tsc+BbrjSY3GWg2F9vUptfdT4OsGOG2K3GI4qCvHs2WmniLnM5b/X mxbCV4RT2Je8LbTRnjYe9iGqVx4LTXgVfynCXhJA= Message-ID: <9e929028-935c-cf89-e2a1-414e6985fd5e@oktetlabs.ru> Date: Sun, 12 Mar 2023 10:59:21 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH v2] net/sfc: stop misuse of Rx ingress m-port metadata on EF100 Content-Language: en-US To: Ivan Malov , dev@dpdk.org Cc: Ferruh Yigit , stable@dpdk.org, Andy Moreton References: <20230309041101.8321-1-ivan.malov@arknetworks.am> <20230310170141.7100-1-ivan.malov@arknetworks.am> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20230310170141.7100-1-ivan.malov@arknetworks.am> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org On 3/10/23 20:01, 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: stable@dpdk.org > > Signed-off-by: Ivan Malov > Reviewed-by: Andy Moreton Better now, but I still have a question. See below. > --- > v2: address community review notes > > drivers/net/sfc/sfc_dp_rx.h | 1 + > drivers/net/sfc/sfc_ef100_rx.c | 9 +++++---- > drivers/net/sfc/sfc_rx.c | 3 +++ > 3 files changed, 9 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..09e5a271d5 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,10 +892,8 @@ sfc_ef100_rx_qstart(struct sfc_dp_rxq *dp_rxq, unsigned int evq_read_ptr, > else > rxq->flags &= ~SFC_EF100_RXQ_USER_MARK; > > - if ((unsup_rx_prefix_fields & > - (1U << EFX_RX_PREFIX_FIELD_INGRESS_MPORT)) == 0) > - rxq->flags |= SFC_EF100_RXQ_INGRESS_MPORT; > - else > + if (unsup_rx_prefix_fields & > + (1U << EFX_RX_PREFIX_FIELD_INGRESS_MPORT)) > rxq->flags &= ~SFC_EF100_RXQ_INGRESS_MPORT; Shouldn't we fail here instead? Basically it means that the representors proxy cannot do its job. EFX_RXQ_FLAG_INGRESS_MPORT is requested for representors proxy Rx queue only. > > if ((unsup_rx_prefix_fields & > diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c > index aae815a653..7401615362 100644 > --- a/drivers/net/sfc/sfc_rx.c > +++ b/drivers/net/sfc/sfc_rx.c > @@ -1242,6 +1242,9 @@ sfc_rx_qinit(struct sfc_adapter *sa, sfc_sw_index_t sw_index, > else > rxq_info->rxq_flags = 0; > > + if (rxq_info->type_flags & EFX_RXQ_FLAG_INGRESS_MPORT) > + rxq_info->rxq_flags |= SFC_RXQ_FLAG_INGRESS_MPORT; > + > rxq->buf_size = buf_size; > > rc = sfc_dma_alloc(sa, "rxq", sw_index, EFX_NIC_DMA_ADDR_RX_RING,