> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Thursday, November 27, 2014 6:00 PM
> To: Liu, Jijiang; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/3] mbuf:add two TX offload flags and change
> three fields
>
> Hi Jijiang,
>
> Please see some comments below.
>
> On 11/27/2014 09:18 AM, Jijiang Liu wrote:
> > In place of removing the PKT_TX_VXLAN_CKSUM, we introduce 2 new flags:
> PKT_TX_OUT_IP_CKSUM, PKT_TX_UDP_TUNNEL_PKT, and a new field:
> l4_tun_len.
> > Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len
> > and
> outer_l3_len field.
> >
> > PKT_TX_OUT_IP_CKSUM: is not used for non-tunnelling packet;hardware outer
> checksum for tunnelling packet.
> > PKT_TX_UDP_TUNNEL_PKT: is used to tell PMD that the transmit packet is a
> UDP tunneling packet.
> > l4_tun_len: for VXLAN packet, it should be udp header length plus VXLAN
> header length.
> >
> > Signed-off-by: Jijiang Liu <jijiang.liu at intel.com>
> > ---
> > lib/librte_mbuf/rte_mbuf.c | 2 +-
> > lib/librte_mbuf/rte_mbuf.h | 23 ++++++++++++++---------
> > 2 files changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 87c2963..e89c310 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -240,7 +240,7 @@ const char *rte_get_tx_ol_flag_name(uint64_t mask)
> > case PKT_TX_SCTP_CKSUM: return "PKT_TX_SCTP_CKSUM";
> > case PKT_TX_UDP_CKSUM: return "PKT_TX_UDP_CKSUM";
> > case PKT_TX_IEEE1588_TMST: return "PKT_TX_IEEE1588_TMST";
> > - case PKT_TX_VXLAN_CKSUM: return "PKT_TX_VXLAN_CKSUM";
> > + case PKT_TX_UDP_TUNNEL_PKT: return "PKT_TX_UDP_TUNNEL_PKT";
> > case PKT_TX_TCP_SEG: return "PKT_TX_TCP_SEG";
> > default: return NULL;
>
> As I said as a reply to the cover letter, I suggest to use
> PKT_TX_OUT_UDP_CKSUM instead of PKT_TX_UDP_TUNNEL_PKT.
>
> Also, the PKT_TX_OUT_IP_CKSUM case is missing here.
Yes.
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 367fc56..48cd8e1 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -99,10 +99,9 @@ extern "C" {
> > #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet with
> IPv6 header. */
> > #define PKT_RX_FDIR_ID (1ULL << 13) /**< FD id reported if FDIR
> > match.
> */
> > #define PKT_RX_FDIR_FLX (1ULL << 14) /**< Flexible bytes reported if
> > FDIR
> match. */
> > -/* add new RX flags here */
> >
>
> We should probably not remove this line.
Why?
There are two lines "/* add new RX flags here */" in rte_mbuf.h file.
>
> > /* add new TX flags here */
> > -#define PKT_TX_VXLAN_CKSUM (1ULL << 50) /**< TX checksum of VXLAN
> computed by NIC */
> > +#define PKT_TX_UDP_TUNNEL_PKT (1ULL << 50) /**< TX packet is an UDP
> > +tunneling packet */
> > #define PKT_TX_IEEE1588_TMST (1ULL << 51) /**< TX IEEE1588 packet to
> > timestamp. */
> >
> > /**
> > @@ -125,13 +124,20 @@ extern "C" {
> > #define PKT_TX_IP_CKSUM (1ULL << 54) /**< IP cksum of TX pkt.
> > computed
> by NIC. */
> > #define PKT_TX_IPV4_CSUM PKT_TX_IP_CKSUM /**< Alias of
> PKT_TX_IP_CKSUM. */
> >
> > +#define PKT_TX_VLAN_PKT (1ULL << 55) /**< TX packet is a 802.1q VLAN
> packet. */
> > +
> > /** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or
> > TSO.
> */
> > -#define PKT_TX_IPV4 PKT_RX_IPV4_HDR
> > +#define PKT_TX_IPV4 (1ULL << 56)
> >
> > /** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or
> > TSO.
> */
> > -#define PKT_TX_IPV6 PKT_RX_IPV6_HDR
> > +#define PKT_TX_IPV6 (1ULL << 57)
>
> The description in comment does not match the description in the cover letter.
Change cover letter.
> Also, I think replacing PKT_RX_IPV[46]_HDR by the value may go in another
> commit.
Ok.
>
> > -#define PKT_TX_VLAN_PKT (1ULL << 55) /**< TX packet is a 802.1q VLAN
> packet. */
> > +/** Outer IP cksum of TX pkt. computed by NIC for tunneling packet */
> > +#define PKT_TX_OUTER_IP_CKSUM (1ULL << 58)
> > +#define PKT_TX_OUTER_IPV4_CSUM PKT_TX_OUTER_IP_CKSUM /**< Alias
> of
> > +PKT_TX_OUTER_IP_CKSUM. */
>
> Why do we need an alias?
Ok, remove it.
> By the way, I think the alias of PKT_TX_IP_CKSUM is also uneeded and can be
> removed. But it's not the topic of your series.
>
> Also, the name PKT_TX_OUTER_IP_CKSUM does not match the name in the cover
> letter and commit logs.
>
>
> > +
> > +/** Tell the NIC it's an outer IPv6 packet for tunneling packet.*/
> > +#define PKT_TX_OUTER_IPV6 (1ULL << 59)
> >
>
> This flag is not in the cover letter or commit log. What is its purpose?
>
With FVL, if outer L3 header is IPv6, to make HW TX checksum offload work , SW
must be responsible to tell hardware this information.
