[dpdk-dev,13/20] testpmd: support gre tunnels in csum fwd engine
Commit Message
Add support for Ethernet over GRE and IP over GRE tunnels.
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
app/test-pmd/cmdline.c | 6 ++--
app/test-pmd/csumonly.c | 87 +++++++++++++++++++++++++++++++++++++++++++++----
2 files changed, 84 insertions(+), 9 deletions(-)
Comments
Hi Olivier,
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, January 30, 2015 9:16 PM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; Liu, Jijiang; Zhang, Helin; olivier.matz@6wind.com
> Subject: [PATCH 13/20] testpmd: support gre tunnels in csum fwd engine
>
> Add support for Ethernet over GRE and IP over GRE tunnels.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> app/test-pmd/cmdline.c | 6 ++--
> app/test-pmd/csumonly.c | 87
> +++++++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 84 insertions(+), 9 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 98f7a1c..aa5c178 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -321,9 +321,9 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> " checksum with when transmitting a packet using
> the"
> " csum forward engine.\n"
> " ip|udp|tcp|sctp always concern the inner
> layer.\n"
> - " outer-ip concerns the outer IP layer (in"
> - " case the packet is recognized as a vxlan packet by"
> - " the forward engine)\n"
> + " outer-ip concerns the outer IP layer in"
> + " case the packet is recognized as a tunnel packet by"
> + " the forward engine (vxlan and gre are
> supported)\n"
> " Please check the NIC datasheet for HW
> limits.\n\n"
>
> "csum parse-tunnel (on|off) (tx_port_id)\n"
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index
> 52af0e7..02c01f6 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -100,6 +100,12 @@ struct testpmd_offload_info {
> uint16_t tso_segsz;
> };
>
> +/* simplified GRE header (flags must be 0) */ struct simple_gre_hdr {
> + uint16_t flags;
> + uint16_t proto;
> +};
I suggest you to remove the comment ' flags must be 0'，the reason is that we just use the header structure to check what the protocol is.
It is not necessary to require the flag must be 0.
> static uint16_t
> get_psd_sum(void *l3_hdr, uint16_t ethertype, uint64_t ol_flags) { @@ -
> 218,6 +224,60 @@ parse_vxlan(struct udp_hdr *udp_hdr, struct
> testpmd_offload_info *info,
> info->l2_len += ETHER_VXLAN_HLEN; /* add udp + vxlan */ }
>
> +/* Parse a gre header */
> +static void
> +parse_gre(struct simple_gre_hdr *gre_hdr, struct testpmd_offload_info
> +*info) {
> + struct ether_hdr *eth_hdr;
> + struct ipv4_hdr *ipv4_hdr;
> + struct ipv6_hdr *ipv6_hdr;
> +
> + /* if flags != 0; it's not supported */
> + if (gre_hdr->flags != 0)
> + return;
I suggest you remove the check here, you can add some comments in front of this function to explain that actual NVGRE and IP over GRE is not supported now.
For example, when I want to support NVGRE TX checksum offload, I will do the following change.
Or you still keep it here, but anyway, I will change it later.
> +
> + if (gre_hdr->proto == _htons(ETHER_TYPE_IPv4)) {
> + info->is_tunnel = 1;
> + info->outer_ethertype = info->ethertype;
> + info->outer_l2_len = info->l2_len;
> + info->outer_l3_len = info->l3_len;
> +
> + ipv4_hdr = (struct ipv4_hdr *)((char *)gre_hdr +
> + sizeof(struct simple_gre_hdr));
> +
> + parse_ipv4(ipv4_hdr, info);
> + info->ethertype = _htons(ETHER_TYPE_IPv4);
> + info->l2_len = 0;
> +
> + } else if (gre_hdr->proto == _htons(ETHER_TYPE_IPv6)) {
> + info->is_tunnel = 1;
> + info->outer_ethertype = info->ethertype;
> + info->outer_l2_len = info->l2_len;
> + info->outer_l3_len = info->l3_len;
> +
> + ipv6_hdr = (struct ipv6_hdr *)((char *)gre_hdr +
> + sizeof(struct simple_gre_hdr));
> +
> + info->ethertype = _htons(ETHER_TYPE_IPv6);
> + parse_ipv6(ipv6_hdr, info);
> + info->l2_len = 0;
> +
> + } else if (gre_hdr->proto == _htons(0x6558)) { /* ETH_P_TEB in linux
> */
> + info->is_tunnel = 1;
> + info->outer_ethertype = info->ethertype;
> + info->outer_l2_len = info->l2_len;
> + info->outer_l3_len = info->l3_len;
> +
> + eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
> + sizeof(struct simple_gre_hdr));
For NVGRE:
I will do some change here.
eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
sizeof(struct nvgre_hdr)); // replace simple_gre_hdr with nvgre_hdr.
> + parse_ethernet(eth_hdr, info);
> + } else
> + return;
> +
> + info->l2_len += sizeof(struct simple_gre_hdr); }
> +
> /* modify the IPv4 or IPv4 source address of a packet */ static void
> change_ip_addresses(void *l3_hdr, uint16_t ethertype) @@ -368,6 +428,8
> @@ uint16_t testpmd_ol_flags)
> * Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP .
> * Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
> * UDP|TCP|SCTP
> + * Ether / (vlan) / outer IP|IP6 / GRE / Ether / IP|IP6 / UDP|TCP|SCTP
> + * Ether / (vlan) / outer IP|IP6 / GRE / IP|IP6 / UDP|TCP|SCTP
> *
> * The testpmd command line for this forward engine sets the flags
> * TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control @@
> -437,12 +499,25 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> parse_ethernet(eth_hdr, &info);
> l3_hdr = (char *)eth_hdr + info.l2_len;
>
> - /* check if it's a supported tunnel (only vxlan for now) */
> - if ((testpmd_ol_flags &
> TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) &&
> - info.l4_proto == IPPROTO_UDP) {
> - struct udp_hdr *udp_hdr;
> - udp_hdr = (struct udp_hdr *)((char *)l3_hdr +
> info.l3_len);
> - parse_vxlan(udp_hdr, &info, m->ol_flags);
> + /* check if it's a supported tunnel */
> + if (testpmd_ol_flags &
> TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) {
> + if (info.l4_proto == IPPROTO_UDP) {
> + struct udp_hdr *udp_hdr;
> + udp_hdr = (struct udp_hdr *)((char *)l3_hdr
> +
> + info.l3_len);
> + parse_vxlan(udp_hdr, &info, m->ol_flags);
> + } else if (info.l4_proto == IPPROTO_GRE) {
> + struct simple_gre_hdr *gre_hdr;
> + gre_hdr = (struct simple_gre_hdr *)
> + ((char *)l3_hdr + info.l3_len);
> + parse_gre(gre_hdr, &info);
> + }
> + }
> + info.l4_proto == IPPROTO_GRE) {
> + struct simple_gre_hdr *gre_hdr;
> + gre_hdr = (struct simple_gre_hdr *)((char *)l3_hdr +
> + info.l3_len);
> + parse_gre(gre_hdr, &info);
> }
>
> /* update l3_hdr and outer_l3_hdr if a tunnel was parsed */
> --
> 2.1.4
Hi Jijiang,
On 02/02/2015 04:04 AM, Liu, Jijiang wrote:
>> +/* simplified GRE header (flags must be 0) */ struct simple_gre_hdr {
>> + uint16_t flags;
>> + uint16_t proto;
>> +};
> I suggest you to remove the comment ' flags must be 0'，the reason is that we just use the header structure to check what the protocol is.
> It is not necessary to require the flag must be 0.
The signification of this comment is that the only supported header
in the parse_gre() function is the header without optional fields
(checksum, offset, key, ...). These fields are not present when flags
is set to 0.
>
>> static uint16_t
>> get_psd_sum(void *l3_hdr, uint16_t ethertype, uint64_t ol_flags) { @@ -
>> 218,6 +224,60 @@ parse_vxlan(struct udp_hdr *udp_hdr, struct
>> testpmd_offload_info *info,
>> info->l2_len += ETHER_VXLAN_HLEN; /* add udp + vxlan */ }
>>
>> +/* Parse a gre header */
>> +static void
>> +parse_gre(struct simple_gre_hdr *gre_hdr, struct testpmd_offload_info
>> +*info) {
>> + struct ether_hdr *eth_hdr;
>> + struct ipv4_hdr *ipv4_hdr;
>> + struct ipv6_hdr *ipv6_hdr;
>> +
>> + /* if flags != 0; it's not supported */
>> + if (gre_hdr->flags != 0)
>> + return;
> I suggest you remove the check here, you can add some comments in front of this function to explain that actual NVGRE and IP over GRE is not supported now.
>
> For example, when I want to support NVGRE TX checksum offload, I will do the following change.
>
> Or you still keep it here, but anyway, I will change it later.
>
>
>> +
>> + if (gre_hdr->proto == _htons(ETHER_TYPE_IPv4)) {
>> + info->is_tunnel = 1;
>> + info->outer_ethertype = info->ethertype;
>> + info->outer_l2_len = info->l2_len;
>> + info->outer_l3_len = info->l3_len;
>> +
>> + ipv4_hdr = (struct ipv4_hdr *)((char *)gre_hdr +
>> + sizeof(struct simple_gre_hdr));
>> +
>> + parse_ipv4(ipv4_hdr, info);
>> + info->ethertype = _htons(ETHER_TYPE_IPv4);
>> + info->l2_len = 0;
>> +
>> + } else if (gre_hdr->proto == _htons(ETHER_TYPE_IPv6)) {
>> + info->is_tunnel = 1;
>> + info->outer_ethertype = info->ethertype;
>> + info->outer_l2_len = info->l2_len;
>> + info->outer_l3_len = info->l3_len;
>> +
>> + ipv6_hdr = (struct ipv6_hdr *)((char *)gre_hdr +
>> + sizeof(struct simple_gre_hdr));
>> +
>> + info->ethertype = _htons(ETHER_TYPE_IPv6);
>> + parse_ipv6(ipv6_hdr, info);
>> + info->l2_len = 0;
>> +
>> + } else if (gre_hdr->proto == _htons(0x6558)) { /* ETH_P_TEB in linux
>> */
>> + info->is_tunnel = 1;
>> + info->outer_ethertype = info->ethertype;
>> + info->outer_l2_len = info->l2_len;
>> + info->outer_l3_len = info->l3_len;
>> +
>> + eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
>> + sizeof(struct simple_gre_hdr));
>
> For NVGRE:
> I will do some change here.
> eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
> sizeof(struct nvgre_hdr)); // replace simple_gre_hdr with nvgre_hdr.
>
I think replacing gre_hdr by nvgre_hdr here would break the gre.
If we want to support NVGRE, I think we should check the value
of gre_hdr->flags to determine the length of the gre header.
I'm not a NVGRE expert, but from what I've seen NVGRE is just a
GRE header with the key flag present.
Regards,
Olivier
Hi Olivier,
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, February 2, 2015 8:55 PM
> To: Liu, Jijiang
> Cc: Ananyev, Konstantin; Zhang, Helin; dev@dpdk.org
> Subject: Re: [PATCH 13/20] testpmd: support gre tunnels in csum fwd engine
>
> Hi Jijiang,
>
> On 02/02/2015 04:04 AM, Liu, Jijiang wrote:
> >> +/* simplified GRE header (flags must be 0) */ struct simple_gre_hdr {
> >> + uint16_t flags;
> >> + uint16_t proto;
> >> +};
> > I suggest you to remove the comment ' flags must be 0'，the reason is that we
> just use the header structure to check what the protocol is.
> > It is not necessary to require the flag must be 0.
>
> The signification of this comment is that the only supported header in the
> parse_gre() function is the header without optional fields (checksum, offset,
> key, ...). These fields are not present when flags is set to 0.
>
> >
> >> static uint16_t
> >> get_psd_sum(void *l3_hdr, uint16_t ethertype, uint64_t ol_flags) {
> >> @@ -
> >> 218,6 +224,60 @@ parse_vxlan(struct udp_hdr *udp_hdr, struct
> >> testpmd_offload_info *info,
> >> info->l2_len += ETHER_VXLAN_HLEN; /* add udp + vxlan */ }
> >>
> >> +/* Parse a gre header */
> >> +static void
> >> +parse_gre(struct simple_gre_hdr *gre_hdr, struct
> >> +testpmd_offload_info
> >> +*info) {
> >> + struct ether_hdr *eth_hdr;
> >> + struct ipv4_hdr *ipv4_hdr;
> >> + struct ipv6_hdr *ipv6_hdr;
> >> +
> >> + /* if flags != 0; it's not supported */
> >> + if (gre_hdr->flags != 0)
> >> + return;
> > I suggest you remove the check here, you can add some comments in front of
> this function to explain that actual NVGRE and IP over GRE is not supported now.
> >
> > For example, when I want to support NVGRE TX checksum offload, I will do the
> following change.
> >
> > Or you still keep it here, but anyway, I will change it later.
> >
> >
> >> +
> >> + if (gre_hdr->proto == _htons(ETHER_TYPE_IPv4)) {
> >> + info->is_tunnel = 1;
> >> + info->outer_ethertype = info->ethertype;
> >> + info->outer_l2_len = info->l2_len;
> >> + info->outer_l3_len = info->l3_len;
> >> +
> >> + ipv4_hdr = (struct ipv4_hdr *)((char *)gre_hdr +
> >> + sizeof(struct simple_gre_hdr));
> >> +
> >> + parse_ipv4(ipv4_hdr, info);
> >> + info->ethertype = _htons(ETHER_TYPE_IPv4);
> >> + info->l2_len = 0;
> >> +
> >> + } else if (gre_hdr->proto == _htons(ETHER_TYPE_IPv6)) {
> >> + info->is_tunnel = 1;
> >> + info->outer_ethertype = info->ethertype;
> >> + info->outer_l2_len = info->l2_len;
> >> + info->outer_l3_len = info->l3_len;
> >> +
> >> + ipv6_hdr = (struct ipv6_hdr *)((char *)gre_hdr +
> >> + sizeof(struct simple_gre_hdr));
> >> +
> >> + info->ethertype = _htons(ETHER_TYPE_IPv6);
> >> + parse_ipv6(ipv6_hdr, info);
> >> + info->l2_len = 0;
> >> +
> >> + } else if (gre_hdr->proto == _htons(0x6558)) { /* ETH_P_TEB in
> >> +linux
> >> */
> >> + info->is_tunnel = 1;
> >> + info->outer_ethertype = info->ethertype;
> >> + info->outer_l2_len = info->l2_len;
> >> + info->outer_l3_len = info->l3_len;
> >> +
> >> + eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
> >> + sizeof(struct simple_gre_hdr));
> >
> > For NVGRE:
> > I will do some change here.
> > eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
> > sizeof(struct nvgre_hdr)); // replace simple_gre_hdr with
> nvgre_hdr.
> >
>
> I think replacing gre_hdr by nvgre_hdr here would break the gre.
> If we want to support NVGRE, I think we should check the value of gre_hdr-
> >flags to determine the length of the gre header.
>
> I'm not a NVGRE expert, but from what I've seen NVGRE is just a GRE header
> with the key flag present.
NVGRE frame format in section 3.2
http://datatracker.ietf.org/doc/draft-sridharan-virtualization-nvgre/?include_text=1
When gre_hdr->proto = htons (0x6558), which means this is NVGRE protocol( MAC over GRE), the key flag must be present, so gre_hdr->flags != 0.
Anyway, the following check should be removed when we support NVGRE TX checksum offload later, or it will failed.
/* if flags != 0; it's not supported */
if (gre_hdr->flags != 0)
return;
> Regards,
> Olivier
Hi Jijiang,
On 02/02/2015 02:16 PM, Liu, Jijiang wrote:
>>> For NVGRE:
>>> I will do some change here.
>>> eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
>>> sizeof(struct nvgre_hdr)); // replace simple_gre_hdr with
>> nvgre_hdr.
>>>
>>
>> I think replacing gre_hdr by nvgre_hdr here would break the gre.
>> If we want to support NVGRE, I think we should check the value of gre_hdr-
>>> flags to determine the length of the gre header.
>>
>> I'm not a NVGRE expert, but from what I've seen NVGRE is just a GRE header
>> with the key flag present.
>
> NVGRE frame format in section 3.2
> http://datatracker.ietf.org/doc/draft-sridharan-virtualization-nvgre/?include_text=1
>
> When gre_hdr->proto = htons (0x6558), which means this is NVGRE protocol( MAC over GRE), the key flag must be present, so gre_hdr->flags != 0.
>
> Anyway, the following check should be removed when we support NVGRE TX checksum offload later, or it will failed.
>
> /* if flags != 0; it's not supported */
> if (gre_hdr->flags != 0)
> return;
Yes, what I'm saying is that we need to replace it by something like:
gre_len = sizeof(*gre);
/* replaces "if (gre->flags != 0)" */
if ((gre->flags & UNSUPPORTED_FLAGS) != 0)
return;
if (gre->flags & KEY)
gre_len += 4;
If the key flag is mandatory in key flag is mandatory in case of
Ethernet over GRE, this should be checked after.
Regards,
Olivier
@@ -321,9 +321,9 @@ static void cmd_help_long_parsed(void *parsed_result,
" checksum with when transmitting a packet using the"
" csum forward engine.\n"
" ip|udp|tcp|sctp always concern the inner layer.\n"
- " outer-ip concerns the outer IP layer (in"
- " case the packet is recognized as a vxlan packet by"
- " the forward engine)\n"
+ " outer-ip concerns the outer IP layer in"
+ " case the packet is recognized as a tunnel packet by"
+ " the forward engine (vxlan and gre are supported)\n"
" Please check the NIC datasheet for HW limits.\n\n"
"csum parse-tunnel (on|off) (tx_port_id)\n"
@@ -100,6 +100,12 @@ struct testpmd_offload_info {
uint16_t tso_segsz;
};
+/* simplified GRE header (flags must be 0) */
+struct simple_gre_hdr {
+ uint16_t flags;
+ uint16_t proto;
+};
+
static uint16_t
get_psd_sum(void *l3_hdr, uint16_t ethertype, uint64_t ol_flags)
{
@@ -218,6 +224,60 @@ parse_vxlan(struct udp_hdr *udp_hdr, struct testpmd_offload_info *info,
info->l2_len += ETHER_VXLAN_HLEN; /* add udp + vxlan */
}
+/* Parse a gre header */
+static void
+parse_gre(struct simple_gre_hdr *gre_hdr, struct testpmd_offload_info *info)
+{
+ struct ether_hdr *eth_hdr;
+ struct ipv4_hdr *ipv4_hdr;
+ struct ipv6_hdr *ipv6_hdr;
+
+ /* if flags != 0; it's not supported */
+ if (gre_hdr->flags != 0)
+ return;
+
+ if (gre_hdr->proto == _htons(ETHER_TYPE_IPv4)) {
+ info->is_tunnel = 1;
+ info->outer_ethertype = info->ethertype;
+ info->outer_l2_len = info->l2_len;
+ info->outer_l3_len = info->l3_len;
+
+ ipv4_hdr = (struct ipv4_hdr *)((char *)gre_hdr +
+ sizeof(struct simple_gre_hdr));
+
+ parse_ipv4(ipv4_hdr, info);
+ info->ethertype = _htons(ETHER_TYPE_IPv4);
+ info->l2_len = 0;
+
+ } else if (gre_hdr->proto == _htons(ETHER_TYPE_IPv6)) {
+ info->is_tunnel = 1;
+ info->outer_ethertype = info->ethertype;
+ info->outer_l2_len = info->l2_len;
+ info->outer_l3_len = info->l3_len;
+
+ ipv6_hdr = (struct ipv6_hdr *)((char *)gre_hdr +
+ sizeof(struct simple_gre_hdr));
+
+ info->ethertype = _htons(ETHER_TYPE_IPv6);
+ parse_ipv6(ipv6_hdr, info);
+ info->l2_len = 0;
+
+ } else if (gre_hdr->proto == _htons(0x6558)) { /* ETH_P_TEB in linux */
+ info->is_tunnel = 1;
+ info->outer_ethertype = info->ethertype;
+ info->outer_l2_len = info->l2_len;
+ info->outer_l3_len = info->l3_len;
+
+ eth_hdr = (struct ether_hdr *)((char *)gre_hdr +
+ sizeof(struct simple_gre_hdr));
+
+ parse_ethernet(eth_hdr, info);
+ } else
+ return;
+
+ info->l2_len += sizeof(struct simple_gre_hdr);
+}
+
/* modify the IPv4 or IPv4 source address of a packet */
static void
change_ip_addresses(void *l3_hdr, uint16_t ethertype)
@@ -368,6 +428,8 @@ uint16_t testpmd_ol_flags)
* Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP .
* Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
* UDP|TCP|SCTP
+ * Ether / (vlan) / outer IP|IP6 / GRE / Ether / IP|IP6 / UDP|TCP|SCTP
+ * Ether / (vlan) / outer IP|IP6 / GRE / IP|IP6 / UDP|TCP|SCTP
*
* The testpmd command line for this forward engine sets the flags
* TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control
@@ -437,12 +499,25 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
parse_ethernet(eth_hdr, &info);
l3_hdr = (char *)eth_hdr + info.l2_len;
- /* check if it's a supported tunnel (only vxlan for now) */
- if ((testpmd_ol_flags & TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) &&
- info.l4_proto == IPPROTO_UDP) {
- struct udp_hdr *udp_hdr;
- udp_hdr = (struct udp_hdr *)((char *)l3_hdr + info.l3_len);
- parse_vxlan(udp_hdr, &info, m->ol_flags);
+ /* check if it's a supported tunnel */
+ if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) {
+ if (info.l4_proto == IPPROTO_UDP) {
+ struct udp_hdr *udp_hdr;
+ udp_hdr = (struct udp_hdr *)((char *)l3_hdr +
+ info.l3_len);
+ parse_vxlan(udp_hdr, &info, m->ol_flags);
+ } else if (info.l4_proto == IPPROTO_GRE) {
+ struct simple_gre_hdr *gre_hdr;
+ gre_hdr = (struct simple_gre_hdr *)
+ ((char *)l3_hdr + info.l3_len);
+ parse_gre(gre_hdr, &info);
+ }
+ }
+ info.l4_proto == IPPROTO_GRE) {
+ struct simple_gre_hdr *gre_hdr;
+ gre_hdr = (struct simple_gre_hdr *)((char *)l3_hdr +
+ info.l3_len);
+ parse_gre(gre_hdr, &info);
}
/* update l3_hdr and outer_l3_hdr if a tunnel was parsed */