From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [RFC PATCH] mbuf: move headers not fragmented check to checksum Date: Wed, 13 Feb 2019 12:50:19 +0300 Message-ID: References: <1548751746-16030-1-git-send-email-arybchenko@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , Konstantin Ananyev , "Thomas Monjalon" , Stephen Hemminger To: Tomasz Kulasek , Olivier Matz Return-path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 206741B137 for ; Wed, 13 Feb 2019 10:50:34 +0100 (CET) In-Reply-To: <1548751746-16030-1-git-send-email-arybchenko@solarflare.com> Content-Language: en-GB List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Ping. Do 2 weeks without reply mean that it looks good and I should send non-RCF version? Andrew. On 1/29/19 11:49 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 > --- > May be the check should be done in rte_net_intel_cksum_prepare() > under RTE_LIBRTE_ETHDEV_DEBUG only. Mainly I'd like to get feedback > on where the limitation comes from and idea to remove it from > rte_validate_tx_offload(). > > 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 a7f67023a..14a3b472b 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -2257,23 +2257,11 @@ static inline int > 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 e59760a0a..bd75aea8e 100644 > --- a/lib/librte_net/rte_net.h > +++ b/lib/librte_net/rte_net.h > @@ -118,10 +118,27 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags) > 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);