From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 3304C2965 for ; Fri, 29 Mar 2019 14:09:54 +0100 (CET) Received: from lfbn-1-5920-128.w90-110.abo.wanadoo.fr ([90.110.126.128] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1h9rIu-0002A1-El; Fri, 29 Mar 2019 14:12:17 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Fri, 29 Mar 2019 14:09:49 +0100 Date: Fri, 29 Mar 2019 14:09:49 +0100 From: Olivier Matz To: Andrew Rybchenko Cc: Tomasz Kulasek , dev@dpdk.org, Konstantin Ananyev , Thomas Monjalon , Ferruh Yigit Message-ID: <20190329130949.tjjo2e5onssvoru4@platinum> References: <1548751746-16030-1-git-send-email-arybchenko@solarflare.com> <1550557852-21882-1-git-send-email-arybchenko@solarflare.com> <9483be25-fc2b-9d0e-81d2-24295d68a07f@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9483be25-fc2b-9d0e-81d2-24295d68a07f@solarflare.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH] mbuf: move headers not fragmented check to checksum 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: , X-List-Received-Date: Fri, 29 Mar 2019 13:09:54 -0000 Hi Andrew, On Thu, Mar 28, 2019 at 08:04:31PM +0300, Andrew Rybchenko wrote: > Ping? (I have a number of net/sfc patches which heavily depend on this > one and must not be applied without this one) > > Andrew. > > On 2/19/19 9:30 AM, Andrew Rybchenko wrote: > > rte_validate_tx_offload() is used in Tx prepare callbacks > > (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency. > > Requirement that packet headers should not be fragmented is not > > documented and unclear where it comes from except > > rte_net_intel_cksum_prepare() functions which relies on it. > > > > It could be NIC vendor specific driver or hardware limitation, but, > > if so, it should be documented and checked in corresponding Tx > > prepare callbacks. > > > > Signed-off-by: Andrew Rybchenko > > Acked-by: Konstantin Ananyev > > --- > > Looks good to me, though extra-testing would be needed. > > Konstantin Ananyev > > > > lib/librte_mbuf/rte_mbuf.h | 12 ------------ > > lib/librte_net/rte_net.h | 17 +++++++++++++++++ > > 2 files changed, 17 insertions(+), 12 deletions(-) > > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > > index d961cca..73daa81 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -2257,23 +2257,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail > > rte_validate_tx_offload(const struct rte_mbuf *m) > > { > > uint64_t ol_flags = m->ol_flags; > > - uint64_t inner_l3_offset = m->l2_len; > > /* Does packet set any of available offloads? */ > > if (!(ol_flags & PKT_TX_OFFLOAD_MASK)) > > return 0; > > - if (ol_flags & PKT_TX_OUTER_IP_CKSUM) > > - /* NB: elaborating the addition like this instead of using > > - * += gives the result uint64_t type instead of int, > > - * avoiding compiler warnings on gcc 8.1 at least */ > > - inner_l3_offset = inner_l3_offset + m->outer_l2_len + > > - m->outer_l3_len; > > - > > - /* Headers are fragmented */ > > - if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len) > > - return -ENOTSUP; > > - > > /* IP checksum can be counted only for IPv4 packet */ > > if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6)) > > return -EINVAL; > > diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h > > index e59760a..bd75aea 100644 > > --- a/lib/librte_net/rte_net.h > > +++ b/lib/librte_net/rte_net.h > > @@ -118,10 +118,27 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m, > > struct udp_hdr *udp_hdr; > > uint64_t inner_l3_offset = m->l2_len; > > + /* > > + * Does packet set any of available offloads? > > + * Mainly it is required to avoid fragmented headers check if > > + * no offloads are requested. > > + */ > > + if (!(ol_flags & PKT_TX_OFFLOAD_MASK)) > > + return 0; > > + > > if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) || > > (ol_flags & PKT_TX_OUTER_IPV6)) > > inner_l3_offset += m->outer_l2_len + m->outer_l3_len; > > + /* > > + * Check if headers are fragmented. > > + * The check could be less strict depending on which offloads are > > + * requested and headers to be used, but let's keep it simple. > > + */ > > + if (unlikely(rte_pktmbuf_data_len(m) < > > + inner_l3_offset + m->l3_len + m->l4_len)) > > + return -ENOTSUP; > > + > > if (ol_flags & PKT_TX_IPV4) { > > ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *, > > inner_l3_offset); > To summarize, the previous code was in a generic part, only enabled if RTE_LIBRTE_ETHDEV_DEBUG is set, and it is moved in an intel-specific part, but always enabled. Am I correct? So it may have a performance impact on intel NICs. Shouldn't it be under a debug option? Regards, Olivier 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 C0242A05D3 for ; Fri, 29 Mar 2019 14:09:55 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C11E82BD3; Fri, 29 Mar 2019 14:09:54 +0100 (CET) Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 3304C2965 for ; Fri, 29 Mar 2019 14:09:54 +0100 (CET) Received: from lfbn-1-5920-128.w90-110.abo.wanadoo.fr ([90.110.126.128] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1h9rIu-0002A1-El; Fri, 29 Mar 2019 14:12:17 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Fri, 29 Mar 2019 14:09:49 +0100 Date: Fri, 29 Mar 2019 14:09:49 +0100 From: Olivier Matz To: Andrew Rybchenko Cc: Tomasz Kulasek , dev@dpdk.org, Konstantin Ananyev , Thomas Monjalon , Ferruh Yigit Message-ID: <20190329130949.tjjo2e5onssvoru4@platinum> References: <1548751746-16030-1-git-send-email-arybchenko@solarflare.com> <1550557852-21882-1-git-send-email-arybchenko@solarflare.com> <9483be25-fc2b-9d0e-81d2-24295d68a07f@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <9483be25-fc2b-9d0e-81d2-24295d68a07f@solarflare.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH] mbuf: move headers not fragmented check to checksum 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" Message-ID: <20190329130949.3gGch0FuOEzkCKvjHaw76d45uQZe-TO1nd_NQQJGCJg@z> Hi Andrew, On Thu, Mar 28, 2019 at 08:04:31PM +0300, Andrew Rybchenko wrote: > Ping? (I have a number of net/sfc patches which heavily depend on this > one and must not be applied without this one) > > Andrew. > > On 2/19/19 9:30 AM, Andrew Rybchenko wrote: > > rte_validate_tx_offload() is used in Tx prepare callbacks > > (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency. > > Requirement that packet headers should not be fragmented is not > > documented and unclear where it comes from except > > rte_net_intel_cksum_prepare() functions which relies on it. > > > > It could be NIC vendor specific driver or hardware limitation, but, > > if so, it should be documented and checked in corresponding Tx > > prepare callbacks. > > > > Signed-off-by: Andrew Rybchenko > > Acked-by: Konstantin Ananyev > > --- > > Looks good to me, though extra-testing would be needed. > > Konstantin Ananyev > > > > lib/librte_mbuf/rte_mbuf.h | 12 ------------ > > lib/librte_net/rte_net.h | 17 +++++++++++++++++ > > 2 files changed, 17 insertions(+), 12 deletions(-) > > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > > index d961cca..73daa81 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -2257,23 +2257,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail > > rte_validate_tx_offload(const struct rte_mbuf *m) > > { > > uint64_t ol_flags = m->ol_flags; > > - uint64_t inner_l3_offset = m->l2_len; > > /* Does packet set any of available offloads? */ > > if (!(ol_flags & PKT_TX_OFFLOAD_MASK)) > > return 0; > > - if (ol_flags & PKT_TX_OUTER_IP_CKSUM) > > - /* NB: elaborating the addition like this instead of using > > - * += gives the result uint64_t type instead of int, > > - * avoiding compiler warnings on gcc 8.1 at least */ > > - inner_l3_offset = inner_l3_offset + m->outer_l2_len + > > - m->outer_l3_len; > > - > > - /* Headers are fragmented */ > > - if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len) > > - return -ENOTSUP; > > - > > /* IP checksum can be counted only for IPv4 packet */ > > if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6)) > > return -EINVAL; > > diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h > > index e59760a..bd75aea 100644 > > --- a/lib/librte_net/rte_net.h > > +++ b/lib/librte_net/rte_net.h > > @@ -118,10 +118,27 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m, > > struct udp_hdr *udp_hdr; > > uint64_t inner_l3_offset = m->l2_len; > > + /* > > + * Does packet set any of available offloads? > > + * Mainly it is required to avoid fragmented headers check if > > + * no offloads are requested. > > + */ > > + if (!(ol_flags & PKT_TX_OFFLOAD_MASK)) > > + return 0; > > + > > if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) || > > (ol_flags & PKT_TX_OUTER_IPV6)) > > inner_l3_offset += m->outer_l2_len + m->outer_l3_len; > > + /* > > + * Check if headers are fragmented. > > + * The check could be less strict depending on which offloads are > > + * requested and headers to be used, but let's keep it simple. > > + */ > > + if (unlikely(rte_pktmbuf_data_len(m) < > > + inner_l3_offset + m->l3_len + m->l4_len)) > > + return -ENOTSUP; > > + > > if (ol_flags & PKT_TX_IPV4) { > > ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *, > > inner_l3_offset); > To summarize, the previous code was in a generic part, only enabled if RTE_LIBRTE_ETHDEV_DEBUG is set, and it is moved in an intel-specific part, but always enabled. Am I correct? So it may have a performance impact on intel NICs. Shouldn't it be under a debug option? Regards, Olivier