[dpdk-dev] [PATCH v3 1/6] ethdev: add Tx preparation

[Ananyev, Konstantin]

> -----Original Message-----
> From: Kulasek, TomaszX
> Sent: Thursday, September 29, 2016 2:04 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org
> Subject: RE: [PATCH v3 1/6] ethdev: add Tx preparation
> 
> Hi Konstantin,
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Thursday, September 29, 2016 12:41
> > To: Kulasek, TomaszX <tomaszx.kulasek at intel.com>; dev at dpdk.org
> > Subject: RE: [PATCH v3 1/6] ethdev: add Tx preparation
> >
> > Hi Tomasz,
> >
> > ....
> >
> > > diff --git a/lib/librte_net/rte_pkt.h b/lib/librte_net/rte_pkt.h new file
> > mode 100644 index 0000000..72903ac
> > > --- /dev/null
> > > +++ b/lib/librte_net/rte_pkt.h
> > > @@ -0,0 +1,133 @@
> > > +/*-
> > > + *   BSD LICENSE
> > > + *
> > > + *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> > > + *   All rights reserved.
> > > + *
> > > + *   Redistribution and use in source and binary forms, with or without
> > > + *   modification, are permitted provided that the following conditions
> > > + *   are met:
> > > + *
> > > + *     * Redistributions of source code must retain the above copyright
> > > + *       notice, this list of conditions and the following disclaimer.
> > > + *     * Redistributions in binary form must reproduce the above copyright
> > > + *       notice, this list of conditions and the following disclaimer in
> > > + *       the documentation and/or other materials provided with the
> > > + *       distribution.
> > > + *     * Neither the name of Intel Corporation nor the names of its
> > > + *       contributors may be used to endorse or promote products derived
> > > + *       from this software without specific prior written permission.
> > > + *
> > > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > CONTRIBUTORS
> > > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> > NOT
> > > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> > FITNESS FOR
> > > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> > COPYRIGHT
> > > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> > INCIDENTAL,
> > > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> > BUT NOT
> > > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> > LOSS OF USE,
> > > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> > AND ON ANY
> > > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> > TORT
> > > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> > OF THE USE
> > > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> > DAMAGE.
> > > + */
> > > +
> > > +#ifndef _RTE_PKT_H_
> > > +#define _RTE_PKT_H_
> > > +
> > > +#include <rte_ip.h>
> > > +#include <rte_udp.h>
> > > +#include <rte_tcp.h>
> > > +#include <rte_sctp.h>
> > > +
> > > +/**
> > > + * Validate general requirements for tx offload in packet.
> > > + */
> > > +static inline int
> > > +rte_validate_tx_offload(struct rte_mbuf *m) {
> > > +	uint64_t ol_flags = m->ol_flags;
> > > +
> > > +	/* Does packet set any of available offloads? */
> > > +	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
> > > +		return 0;
> > > +
> > > +	/* IP checksum can be counted only for IPv4 packet */
> > > +	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
> > > +		return -EINVAL;
> > > +
> > > +	if (ol_flags & (PKT_TX_L4_MASK | PKT_TX_TCP_SEG))
> >
> > Not sure what you are trying to test here?
> > Is that PKT_TX_TCP_SEG is set?
> > If so, then the test condition doesn't look correct to me.
> >
> > > +		/* IP type not set */
> > > +		if (!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6)))
> > > +			return -EINVAL;
> > > +
> 
> 	if (ol_flags & (PKT_TX_L4_MASK | PKT_TX_TCP_SEG))
> 		/* IP type not set */
> 		if (!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6)))
> 			return -EINVAL;
> 
> For L4 checksums (L4_MASK == TCP_CSUM|UDP_CSUM|SCTP_CSUM) as well as for TCP_SEG, the flag PKT_TX_IPV4 or PKT_TX_IPV6
> must be set. L4_MASK doesn't include TCP_SEG bit, so I added it to have one condition for all these cases (check if IPV4/6 flag is set
> when required).
> 
> More detailed check, only for TCP_SEG is below (tso_segsz and IP_CSUM flag for IPV4):
> 
> > > +	if (ol_flags & PKT_TX_TCP_SEG)
> > > +		/* PKT_TX_IP_CKSUM offload not set for IPv4 TSO packet */
> > > +		if ((m->tso_segsz == 0) ||
> > > +				((ol_flags & PKT_TX_IPV4) && !(ol_flags &
> > PKT_TX_IP_CKSUM)))
> > > +			return -EINVAL;
> > > +
> >
> > Why not just:
> > If ((ol_flags & PKT_TX_L4_MASK) == PKT_TX_TCP_SEG) {
> 
> PKT_TX_L4_MASK doesn't include PKT_TX_TCP_SEG, so it will always be false.

Ah yes, your right.
By some reason I thought that it does.
Looks good to me then and sorry for the noise.
Konstantin

> 
> >
> >          uint64_t f = ol_flags & PKT_TX_L4_MASK;
> >
> >          if ((f  & (PKT_TX_IPV4 | PKT_TX_IPV6)) == 0 || f == PKT_TX_IPV4 || m-
> > >tso_segsz == 0)
> > 		return -EINVAL;
> > }
> >
> > Instead of 2 ifs around TCP_SEG above?
> >
> > Konstantin
> >
> 
> Tomasz