DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Patil, Harish" <Harish.Patil@cavium.com>
To: Olivier Matz <olivier.matz@6wind.com>,
	"Mody, Rasesh" <Rasesh.Mody@cavium.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Dept-Eng DPDK Dev <Dept-EngDPDKDev@cavium.com>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] mbuf: introduce new Tx offload flag for MPLS-in-UDP
Date: Thu, 8 Jun 2017 21:46:00 +0000	[thread overview]
Message-ID: <D55F0C99.1469AE%Harish.Patil@cavium.com> (raw)
In-Reply-To: <20170608142545.10cc8326@platinum>

>Hi Rasesh,
>
>On Wed, 7 Jun 2017 00:43:48 -0700, Rasesh Mody <rasesh.mody@cavium.com>
>wrote:
>> From: Harish Patil <harish.patil@cavium.com>
>> 
>> Some PMDs need to know the tunnel type in order to handle advance TX
>> features. This patch adds a new TX offload flag for MPLS-in-UDP packets.
>> 
>> Signed-off-by: Harish Patil <harish.patil@cavium.com>
>> ---
>>  lib/librte_mbuf/rte_mbuf.c |    2 ++
>>  lib/librte_mbuf/rte_mbuf.h |   17 ++++++++++-------
>>  2 files changed, 12 insertions(+), 7 deletions(-)
>> 
>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>> index 0e3e36a..c2793fb 100644
>> --- a/lib/librte_mbuf/rte_mbuf.c
>> +++ b/lib/librte_mbuf/rte_mbuf.c
>> @@ -410,6 +410,7 @@ const char *rte_get_tx_ol_flag_name(uint64_t mask)
>>  	case PKT_TX_TUNNEL_IPIP: return "PKT_TX_TUNNEL_IPIP";
>>  	case PKT_TX_TUNNEL_GENEVE: return "PKT_TX_TUNNEL_GENEVE";
>>  	case PKT_TX_MACSEC: return "PKT_TX_MACSEC";
>> +	case PKT_TX_TUNNEL_MPLSINUDP: return "PKT_TX_TUNNEL_MPLSINUDP";
>>  	default: return NULL;
>>  	}
>>  }
>> @@ -441,6 +442,7 @@ const char *rte_get_tx_ol_flag_name(uint64_t mask)
>>  		{ PKT_TX_TUNNEL_GENEVE, PKT_TX_TUNNEL_MASK,
>>  		  "PKT_TX_TUNNEL_NONE" },
>>  		{ PKT_TX_MACSEC, PKT_TX_MACSEC, NULL },
>> +		{ PKT_TX_TUNNEL_MPLSINUDP, PKT_TX_TUNNEL_MPLSINUDP, NULL },
>>  	};
>>  	const char *name;
>>  	unsigned int i;
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 1cb0310..27ad421 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -197,19 +197,22 @@
>>   * Offload the MACsec. This flag must be set by the application to
>>enable
>>   * this offload feature for a packet to be transmitted.
>>   */
>> -#define PKT_TX_MACSEC        (1ULL << 44)
>> +#define PKT_TX_MACSEC        (1ULL << 43)
>
>I'm not sure it is suitable to change the value of an existing
>flag, since it breaks the ABI.
>
>
>>  /**
>> - * Bits 45:48 used for the tunnel type.
>> + * Bits 44:48 used for the tunnel type.
>>   * When doing Tx offload like TSO or checksum, the HW needs to
>>configure the
>>   * tunnel type into the HW descriptors.
>>   */
>> -#define PKT_TX_TUNNEL_VXLAN   (0x1ULL << 45)
>> -#define PKT_TX_TUNNEL_GRE     (0x2ULL << 45)
>> -#define PKT_TX_TUNNEL_IPIP    (0x3ULL << 45)
>> -#define PKT_TX_TUNNEL_GENEVE  (0x4ULL << 45)
>> +/**< TX packet with MPLS-in-UDP RFC 7510 header. */
>> +#define PKT_TX_TUNNEL_MPLSINUDP (0x1ULL << 44)
>> +
>> +#define PKT_TX_TUNNEL_VXLAN   (0x2ULL << 44)
>> +#define PKT_TX_TUNNEL_GRE     (0x3ULL << 44)
>> +#define PKT_TX_TUNNEL_IPIP    (0x4ULL << 44)
>> +#define PKT_TX_TUNNEL_GENEVE  (0x5ULL << 45)
>>  /* add new TX TUNNEL type here */
>> -#define PKT_TX_TUNNEL_MASK    (0xFULL << 45)
>> +#define PKT_TX_TUNNEL_MASK    (0x1FULL << 44)
>>  
>>  /**
>>   * Second VLAN insertion (QinQ) flag.
>
>I dont understand why adding
>#define PKT_TX_TUNNEL_MPLSINUDP (0x5ULL << 45)
>wouldn't do the job?
>
>Currently, the tunnel mask is 0xF << 45, which gives 16 possible values.

[Harish]  Hi Olivier,
Not too sure whether I understand your comment.
My understanding is that those are bitmapped values for each Tx tunnel
type in the range [48:45].
They are not values. So defining PKT_TX_TUNNEL_MPLSINUDP (0x5ULL << 45)
won’t work.

Currently bits[48:45] are reserved for Tx tunnel types. Bits[63:49] and
bit 44 are already taken.
Bits [43:18] are free. That’s why we see a code comment there:

/* add new RX flags here */

/* add new TX flags here */

So I could have added MPLSINUDP as:
#define PKT_TX_TUNNEL_MPLSINUDP (1ULL << 18)


But I wanted to group all Tx tunnel types together which is logical and
update PKT_TX_TUNNEL_MASK accordingly. So to accommodate the new MPLSoUDP
flag, I had to move PKT_TX_MACSEC        to one bit position back from 44
to 43 and hence the code comment:
- * Bits 45:48 used for the tunnel type.
+ * Bits 44:48 used for the tunnel type.

But if this would cause a ABI breakage the option is to use bit 18 as
shown above and update the mask accordingly.

But the down side of this is that it will not be grouped together.
Please let me know if this is okay?

Thanks,
Harish
>
>Regards,
>Olivier
>



  reply	other threads:[~2017-06-08 21:46 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-28  8:37 [dpdk-dev] [PATCH 0/7] net/qede/base: update PMD to 2.5.0.1 Rasesh Mody
2017-05-28  8:37 ` [dpdk-dev] [PATCH 1/7] net/qede: refactoring vport handling code Rasesh Mody
2017-05-28  8:37 ` [dpdk-dev] [PATCH 2/7] net/qede: refactoring multi-queue implementation Rasesh Mody
2017-05-28  8:37 ` [dpdk-dev] [PATCH 3/7] net/qede/base: upgrade the FW to 8.20.0.0 Rasesh Mody
2017-05-28  8:37 ` [dpdk-dev] [PATCH 4/7] net/qede: fix VXLAN tunnel Tx offload flag setting Rasesh Mody
2017-05-28  8:37 ` [dpdk-dev] [PATCH 5/7] net/qede: refactor Tx routine Rasesh Mody
2017-05-28  8:37 ` [dpdk-dev] [PATCH 6/7] mbuf: introduce new Tx offload flag for MPLS-in-UDP Rasesh Mody
2017-05-30 12:23   ` Ferruh Yigit
2017-06-01 18:59     ` Patil, Harish
2017-06-01 19:43       ` Ferruh Yigit
2017-06-07  7:49         ` Mody, Rasesh
2017-05-28  8:37 ` [dpdk-dev] [PATCH 7/7] net/qede: add Tx offloads for MPLS-in-UDP packets Rasesh Mody
2017-06-07  7:42 ` [dpdk-dev] [PATCH v2 0/5] net/qede/base: update PMD to 2.5.0.1 Rasesh Mody
2017-06-07  9:38   ` Ferruh Yigit
2017-06-07  7:42 ` [dpdk-dev] [PATCH v2 1/5] net/qede: refactoring vport handling code Rasesh Mody
2017-06-07  7:42 ` [dpdk-dev] [PATCH v2 2/5] net/qede: refactoring multi-queue implementation Rasesh Mody
2017-06-07  7:42 ` [dpdk-dev] [PATCH v2 3/5] net/qede/base: upgrade the FW to 8.20.0.0 Rasesh Mody
2017-06-07  7:42 ` [dpdk-dev] [PATCH v2 4/5] net/qede: fix VXLAN tunnel Tx offload flag setting Rasesh Mody
2017-06-07  7:42 ` [dpdk-dev] [PATCH v2 5/5] net/qede: refactor Tx routine Rasesh Mody
2017-06-07  7:43 ` [dpdk-dev] [PATCH v2 1/2] mbuf: introduce new Tx offload flag for MPLS-in-UDP Rasesh Mody
2017-06-08 12:25   ` Olivier Matz
2017-06-08 21:46     ` Patil, Harish [this message]
     [not found]       ` <20170609091811.0867b1d1@platinum>
2017-06-10  6:17         ` Patil, Harish
2017-06-17 21:57   ` [dpdk-dev] [PATCH v3 " Rasesh Mody
2017-06-27  7:21     ` [dpdk-dev] [PATCH v4 " Rasesh Mody
2017-06-30  8:33       ` Olivier Matz
2017-06-27  7:21     ` [dpdk-dev] [PATCH v4 2/2] net/qede: add Tx offloads for MPLS-in-UDP packets Rasesh Mody
2017-06-17 21:57   ` [dpdk-dev] [PATCH v3 " Rasesh Mody
2017-06-07  7:43 ` [dpdk-dev] [PATCH v2 " Rasesh Mody

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=D55F0C99.1469AE%Harish.Patil@cavium.com \
    --to=harish.patil@cavium.com \
    --cc=Dept-EngDPDKDev@cavium.com \
    --cc=Rasesh.Mody@cavium.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=olivier.matz@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).