From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 3C14937B2 for ; Mon, 22 Feb 2016 06:28:09 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 21 Feb 2016 21:28:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,483,1449561600"; d="scan'208";a="656932432" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 21 Feb 2016 21:28:08 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 21 Feb 2016 21:28:07 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.172]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.196]) with mapi id 14.03.0248.002; Mon, 22 Feb 2016 13:28:05 +0800 From: "Lu, Wenzhuo" To: "Ananyev, Konstantin" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] ixgbe: fix x550 VF tx issue Thread-Index: AQHRWmluRZsMk/tR/06KOcrMajbo758wChcAgAF/ekA= Date: Mon, 22 Feb 2016 05:28:05 +0000 Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC09090342DB80@shsmsx102.ccr.corp.intel.com> References: <1454053693-28673-1-git-send-email-wenzhuo.lu@intel.com> <2601191342CEEE43887BDE71AB97725836B078DD@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725836B078DD@irsmsx105.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix x550 VF tx issue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 Feb 2016 05:28:10 -0000 Hi Konstantin, Many thanks for your review and comments. Sorry for the late response. Plea= se see inline :) > -----Original Message----- > From: Ananyev, Konstantin > Sent: Thursday, February 18, 2016 12:45 AM > To: Lu, Wenzhuo ; dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH] ixgbe: fix x550 VF tx issue >=20 > Hi Wenzhuo, >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu > > Sent: Friday, January 29, 2016 7:48 AM > > To: dev@dpdk.org > > Subject: [dpdk-dev] [PATCH] ixgbe: fix x550 VF tx issue > > > > On x550, the basic tx function may not work if we use DPDK VF based on > > kernel PF. The reason is kernel PF enables a new feature "malicious > > driver detection". According to this feature, driver should create TX > > context descriptor and set the "Check Context" bit in advanced data > descriptor. > > This patch add the support of "malicious driver detection" to let > > kernel PF based DPDK VF work. >=20 > Is there any way for VF to determine is MDD is enabled on PF or not? > Mailbox message or something? > If not, then I suppose for x550 VF we always have to set default txq_flag= s in > dev_info to PKT_TX_MALICIOUS_DRIVER_DETECT? > Which means that for X550 VF simple TX path by default will be disabled, > correct? The bad news is now there's no way for VF to know if the MDD is enabled. Bu= t PF will disable MDD by default. And MDD will impact the performance. So, I think VF better disable MDD by d= efault either. > As alternative we probably can change tx_queue_start() for x550 VF to > always setup a dummy TCD and then make changes in TX simple path to > always setup CC bit for x550 VF. > That would allow to keep using TX simple for x550 VF , no matter is MDD i= s > enabled by PF or not. Any suggestion about how to set the dummy TCD? Thanks. I think TCD is used to describe the data, so it should match the packets. S= o, I'm sure how to create a dummy TCD :( >=20 > Another thing - there could be a potential slowdown with that feature: > As now TX has to occupy one extra HW context, even if no HW offload is > requested by SW, correct? > Let say if app sends packets of 3 types: > - TCP with TX HW csum offload > - UDP with TX HW csum offload > - packets with no HW offload requested > Then on 82599 VF no PMD would need to setup TCD only fist 2 times. > But on X550 VF will need to setup new TCD for every packet. > But I suppose that is unavoidable. Yes, I agree. There's performance impact and seems unavoidable. >=20 > > > > Although CC bit should be set in IOV mode, as tested, it's no harm to > > set CC bit in non-IVO mode. So, as PF and VF share the same code, will > > set CC bit anyway. >=20 > I don't think it totally harmless. > As I understand, if CC bit is set HW would try to apply related TC to the > packet. > If this TC was never setup - you don't know what values it will contain. I agree with you, although as tested, the NULL TC doesn't cause any problem= . In this scenario, better only set CC bit for VF. I'll try to do that. >=20 > > > > Please aware there's another way, disabling MDD in PF kernel driver. > > Like this, > > >insmod ixgbe.ko MDD=3D0,0 > > > > Signed-off-by: Wenzhuo Lu > > --- > > doc/guides/rel_notes/release_2_3.rst | 12 ++++++++++++ > > drivers/net/ixgbe/ixgbe_rxtx.c | 16 ++++++++++++++-- > > drivers/net/ixgbe/ixgbe_rxtx.h | 3 ++- > > lib/librte_ether/rte_ethdev.h | 1 + > > lib/librte_mbuf/rte_mbuf.h | 3 ++- > > 5 files changed, 31 insertions(+), 4 deletions(-) > > > > diff --git a/doc/guides/rel_notes/release_2_3.rst > > b/doc/guides/rel_notes/release_2_3.rst > > index 99de186..90f81e0 100644 > > --- a/doc/guides/rel_notes/release_2_3.rst > > +++ b/doc/guides/rel_notes/release_2_3.rst > > @@ -15,6 +15,18 @@ EAL > > Drivers > > ~~~~~~~ > > > > +* **ixgbe: Fixed x550 VF tx issue.** > > + > > + On x550, the basic tx function may not work if we use DPDK VF based > > + on kernel PF. The reason is kernel PF enables a new feature > > + "malicious driver detection". According to this feature, driver > > + should create TX context descriptor and set the "Check Context" bit = in > advanced data descriptor. > > + This patch add the support of "malicious driver detection" to let > > + kernel PF based DPDK VF work. > > + > > + Please aware there's another way, disabling MDD in PF kernel driver. > > + Like this, > > + >insmod ixgbe.ko MDD=3D0,0 > > > > Libraries > > ~~~~~~~~~ > > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c > > b/drivers/net/ixgbe/ixgbe_rxtx.c index 52a263c..c8a7740 100644 > > --- a/drivers/net/ixgbe/ixgbe_rxtx.c > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c > > @@ -85,7 +85,8 @@ > > PKT_TX_VLAN_PKT | \ > > PKT_TX_IP_CKSUM | \ > > PKT_TX_L4_MASK | \ > > - PKT_TX_TCP_SEG) > > + PKT_TX_TCP_SEG | \ > > + PKT_TX_MALICIOUS_DRIVER_DETECT) > > > > static inline struct rte_mbuf * > > rte_rxmbuf_alloc(struct rte_mempool *mp) @@ -564,6 +565,8 @@ > > ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq) > > return (0); > > } > > > > +#define DEFAULT_CTX_L2_LEN 14 >=20 > Use sizeof(strutct eth_hdr) or something. Honestly the 14 comes from datasheet. I think your idea is better :) >=20 > > + > > uint16_t > > ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, > > uint16_t nb_pkts) > > @@ -614,11 +617,19 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf > **tx_pkts, > > * are needed for offload functionality. > > */ > > ol_flags =3D tx_pkt->ol_flags; > > + if (!(txq->txq_flags & ETH_TXQ_FLAGS_NOMDD)) > > + ol_flags |=3D PKT_TX_MALICIOUS_DRIVER_DETECT; >=20 > Probably a bit better introduce new filed inside txq: deaul_ol_flags or > something, Then set it up depending on HW type, and then just: >=20 > ol_flags =3D tx_pkt->ol_flags | txq->default_ol_flags; I think you mean doing this way can decrease the performance impact, right?= Let me figure out how to do it? >=20 >=20 > > > > /* If hardware offload required */ > > tx_ol_req =3D ol_flags & IXGBE_TX_OFFLOAD_MASK; > > if (tx_ol_req) { > > - tx_offload.l2_len =3D tx_pkt->l2_len; > > + /* if l2 len isn't provided by the caller, give it a > > + * default value. > > + */ > > + if (tx_pkt->l2_len =3D=3D 0) > > + tx_offload.l2_len =3D DEFAULT_CTX_L2_LEN; > > + else > > + tx_offload.l2_len =3D tx_pkt->l2_len; >=20 > So with l2_len=3D=3D0 what would happen? > PF would stop the queue? Even worse. As I remember, PF will reset the VF. >=20 >=20 > > tx_offload.l3_len =3D tx_pkt->l3_len; > > tx_offload.l4_len =3D tx_pkt->l4_len; > > tx_offload.vlan_tci =3D tx_pkt->vlan_tci; @@ -801,6 > +812,7 @@ > > ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, > > } > > > > olinfo_status |=3D (pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT); > > + olinfo_status |=3D IXGBE_ADVTXD_CC; >=20 > As I said, probably safer to set CC bit only if (tx_ol_req !=3D 0). Yes, it's better. I thought I need to check if it's VF. But seems maybe I o= nly need to check this ol flag. Thanks for the suggestion:) >=20 > > > > m_seg =3D tx_pkt; > > do { > > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h > > b/drivers/net/ixgbe/ixgbe_rxtx.h index 475a800..bcde785 100644 > > --- a/drivers/net/ixgbe/ixgbe_rxtx.h > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.h > > @@ -255,7 +255,8 @@ struct ixgbe_txq_ops { > > * RTE_PMD_IXGBE_TX_MAX_BURST. > > */ > > #define IXGBE_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | > \ > > - ETH_TXQ_FLAGS_NOOFFLOADS) > > + ETH_TXQ_FLAGS_NOOFFLOADS | \ > > + ETH_TXQ_FLAGS_NOMDD) > > > > /* > > * Populate descriptors with the following info: > > diff --git a/lib/librte_ether/rte_ethdev.h > > b/lib/librte_ether/rte_ethdev.h index bada8ad..a2e32d0 100644 > > --- a/lib/librte_ether/rte_ethdev.h > > +++ b/lib/librte_ether/rte_ethdev.h > > @@ -637,6 +637,7 @@ struct rte_eth_rxconf { #define > > ETH_TXQ_FLAGS_NOXSUMSCTP 0x0200 /**< disable SCTP checksum offload > */ > > #define ETH_TXQ_FLAGS_NOXSUMUDP 0x0400 /**< disable UDP checksum > > offload */ #define ETH_TXQ_FLAGS_NOXSUMTCP 0x0800 /**< disable TCP > > checksum offload */ > > +#define ETH_TXQ_FLAGS_NOMDD 0x1000 /**< disable malicious driver > detection */ > > #define ETH_TXQ_FLAGS_NOOFFLOADS \ > > (ETH_TXQ_FLAGS_NOVLANOFFL | > ETH_TXQ_FLAGS_NOXSUMSCTP | \ > > ETH_TXQ_FLAGS_NOXSUMUDP | > ETH_TXQ_FLAGS_NOXSUMTCP) diff --git > > a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h index > > f234ac9..b5ed25c 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -102,6 +102,7 @@ extern "C" { > > > > /* add new TX flags here */ > > > > +#define PKT_TX_MALICIOUS_DRIVER_DETECT (1ULL << 48) > > /** > > * Second VLAN insertion (QinQ) flag. > > */ > > @@ -824,7 +825,7 @@ struct rte_mbuf { > > struct rte_mempool *pool; /**< Pool from which mbuf was allocated. > */ > > struct rte_mbuf *next; /**< Next segment of scattered packet. */ > > > > - /* fields to support TX offloads */ > > + /* fields to support malicious driver detection or TX offloads */ >=20 > No fields are really updated, so probably no need to update comments here= . It's reminder. As the users may not know if the MDD can work as expected de= pends on these fields. >=20 > > union { > > uint64_t tx_offload; /**< combined for easy fetch */ > > struct { > > -- > > 1.9.3