From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 16BDCA0471 for ; Fri, 21 Jun 2019 14:36:00 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 81F641D5C9; Fri, 21 Jun 2019 14:35:59 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id D5B811D5C8 for ; Fri, 21 Jun 2019 14:35:57 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 72F13100095; Fri, 21 Jun 2019 12:35:55 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 21 Jun 2019 13:35:48 +0100 To: "Ananyev, Konstantin" , Ivan Malov , Olivier Matz CC: "dev@dpdk.org" References: <20190412150542.12026-1-ivan.malov@oktetlabs.ru> <20190621103419.11626-1-ivan.malov@oktetlabs.ru> <2601191342CEEE43887BDE71AB97725801689E31B2@IRSMSX104.ger.corp.intel.com> From: Andrew Rybchenko Message-ID: <4462101f-5250-12f8-db88-0c3af619017a@solarflare.com> Date: Fri, 21 Jun 2019 15:35:45 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.1 MIME-Version: 1.0 In-Reply-To: <2601191342CEEE43887BDE71AB97725801689E31B2@IRSMSX104.ger.corp.intel.com> Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24700.000 X-TM-AS-Result: No-19.783300-8.000000-10 X-TMASE-MatchedRID: yebcs53SkkDmLzc6AOD8DfHkpkyUphL9MHi1Ydy2WEj6eV5+LAaaX3ZQ rrFTqfg/Z+g/9suVejVqnCONYgFlZ9tRKUVZdq2UhUy0TABax1xp4xorO9dSmWz3xnx8b/qRKYK OOxNxwUXlOyb8gIQzWGNQNBwI/n1bjHHp3pfy0nqiAZ3zAhQYgg51SAUUhKsU33Nl3elSfsqlzb GAkBjoug/tuprs4a1fVIKZ9Pa/e15C79cOJbjsnhKQNtfbAzM5EAImHgFYA97PA7gCOGkjyC5V+ T0A+cl73BxkrQlSO91jO2cJJWVbW4JeUXYfLk0BAZ0lncqeHqEyJlZlUbAvWD4tlpFReMb7lCv9 qMQmd99Bgqrqot9+U+HJa+vhvYs5gb/IM5JgEpssIMJqMNlanTyW+6AAWIU/uM5RdaZDc5Z7MoG OLT7cXlx24EY6Ow+8tNvIx77SGgzltmCWilxjDnpRh12Siy9rQZpQRfyCdHxNX9jj1RuwKbo8PQ V15rjgviQqB7I+AfdnTS5MzRK4gXkbhjW2P21N2os3ueKAsFSROPoc+kRA3xHfiujuTbedpTMfR y1HRzk2muOXUVATZkWQd390aC2c0PSnqhxMpGeb73wAA/f83TCeYh/W8m8dH5kZy/EQbgR21/Pb emfc8U7/MzmpaW1fL9hFOEptv6Dqf4K8526DcaXZvcUfKhsoK1bnB+4SMTJHrZN20zVXgvwjyg0 5v+y2JKxndz+L6CybHAuQ1dUnuWJZXQNDzktSDDlsUbcsIPpj0mUvAvV2DXxbHSW75UstoQYqGb 1tpo3y21VZqXynGX6FAoobZbWOcGCkjn3GWnaeAiCmPx4NwLTrdaH1ZWqC1B0Hk1Q1KyLUZxEAl FPo847dS6zvwQvCeeDML7BD4BUBItVcRknU+Bmug2mXg4aGsFzABgYM86E= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--19.783300-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24700.000 X-MDID: 1561120556-BL5iRjZ_pAI4 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] mbuf: outer offsets must be zero for non-tunnel packets X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Konstantin, On 6/21/19 2:10 PM, Ananyev, Konstantin wrote: > Hi Ivan, >> Make sure that outer L2 and L3 header length fields are >> equal to zero for non-tunnel packets in order to ensure >> consistent and predictable behaviour in network drivers. >> Explain this expectation in comments to help developers. >> >> Signed-off-by: Ivan Malov >> Reviewed-by: Andrew Rybchenko >> --- > Not sure it is a good idea: > 1) it is a change in public API behavior (requirements). I would say that it is a clarification. Yes, in terms of rte_validate_tx_offload() behaviour is it is a change. The area looks grey and we just want to make it either black or white. What is the alternative? Say that outer_l2_len and outer_l3_len content is undefined if packet is not tunnelled and drivers must check (ol_flags & PKT_TX_TUNNEL_MASK) != 0 before usage these fields? bnxt, fm10k, i40e, ixgbe (depends on PKT_TX_OUTER_IP_CKSUM in fact, but not PKT_TX_TUNNEL_MASK) and ice use these fields w/o tunnel checks (if I read code correctly). enic, mlx4, mlx5, qede and sfc use them in the case of tunnel packet only. I.e. 5 vs 5. > 2) why these 2 particular tx_offload fields only? > If we'll follow that logic we should enforce same rule for other > tx_offload fileds (tso, l4_len, l3_len, etc.) Because it is about tunnel packets and outer_l2_len and outer_l3_len should be either undefined or 0 for non-tunnel packets. > Personally I don't think there will be much gain from it. > Might be better and easier just to fix offending drivers that make wrong assumptions. We would prefer to define as the patch suggests since it allows to avoid conditions. Other option is to add a comment saying that content of these fields is undefined for non-tunnel packets. Of course, the patch makes it required to care about outer_l2/3_len when mbuf is reused and Tx offloads are requested. So, may be from application point of view it is better to have it undefined for non-tunnel packets. > If we'll still decide to go that way, then I think at least it needs > to be explained in RN, and probably deprecation process has to be followed here. Yes, I agree and would like to understand which way is right (just highlight in release notes or deprecation process). BTW, may I ask you to take a look at two more small patches: [1] https://patches.dpdk.org/patch/53691/ [2] https://patches.dpdk.org/patch/53857/ Many thanks, Andrew. (As Keith said some time ago it looks like almost nobody look at RFC patches. Sad. The main goal of RFC patches is get feedback earlier. RFC for this one was in April and we could start deprecation process in previous release cycle if it is required. Luckily it is not critical in this case.) > Konstantin > >> Notes: >> At the time of writing a couple of network drivers rely on >> the statement (i40e, ice) whilst more drivers have runtime >> conditional checks to guard all references to these fields. >> This patch is likely to relieve datapath checks in drivers. >> >> lib/librte_mbuf/rte_mbuf.h | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h >> index 0d9fef0..cb8b34e 100644 >> --- a/lib/librte_mbuf/rte_mbuf.h >> +++ b/lib/librte_mbuf/rte_mbuf.h >> @@ -702,7 +702,12 @@ struct rte_mbuf { >> uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS; >> /**< TCP TSO segment size */ >> >> - /* fields for TX offloading of tunnels */ >> + /* >> + * Fields for Tx offloading of tunnels. >> + * These fields must be equal to zero in the case >> + * when (ol_flags & PKT_TX_TUNNEL_MASK) == 0, >> + * i.e. for all non-tunnel packets. >> + */ >> uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS; >> /**< Outer L3 (IP) Hdr Length. */ >> uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS; >> @@ -2376,6 +2381,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail >> !(ol_flags & PKT_TX_OUTER_IPV4)) >> return -EINVAL; >> >> + /* Outer L2/L3 offsets must be equal to zero for non-tunnel packets. */ >> + if ((ol_flags & PKT_TX_TUNNEL_MASK) == 0 && >> + m->outer_l2_len + m->outer_l3_len != 0) >> + return -EINVAL; >> + >> return 0; >> } >> >> -- >> 1.8.3.1