[dpdk-dev] [PATCH 0/2] Two offloading issues of tep_term

[dpdk-dev] [PATCH 0/2] Two offloading issues of tep_term

[Jianfeng Tan]

This patch set depends on:
 - http://dpdk.org/ml/archives/dev/2016-August/044924.html

Patch 1: fill tunneling type.
Patch 2: inner L4 checksum error.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Jianfeng Tan (2):
  examples/tep_term: fix offload on VXLAN failure
  examples/tep_term: fix inner L4 checksum failure

 examples/tep_termination/vxlan.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/2] examples/tep_term: fix offload on VXLAN failure

[Jianfeng Tan]

Based on previous fix of offload on VXLAN using i40e, applications
need to set proper tunneling type on ol_flags so that i40e driver
can pass it to NIC.

Fixes: a50245ede72a ("examples/tep_term: initialize VXLAN sample")

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 examples/tep_termination/vxlan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/examples/tep_termination/vxlan.c b/examples/tep_termination/vxlan.c
index 5ee1f95..4bad33d 100644
--- a/examples/tep_termination/vxlan.c
+++ b/examples/tep_termination/vxlan.c
@@ -237,6 +237,8 @@ encapsulation(struct rte_mbuf *m, uint8_t queue_id)
 	m->outer_l2_len = sizeof(struct ether_hdr);
 	m->outer_l3_len = sizeof(struct ipv4_hdr);
 
+	ol_flags |= PKT_TX_TUNNEL_VXLAN;
+
 	m->ol_flags |= ol_flags;
 	m->tso_segsz = tx_offload.tso_segsz;
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/2] examples/tep_term: fix offload on VXLAN failure

[Yuanhan Liu]

On Thu, Aug 04, 2016 at 07:58:48AM +0000, Jianfeng Tan wrote:
> Based on previous fix of offload on VXLAN using i40e, applications
> need to set proper tunneling type on ol_flags so that i40e driver
> can pass it to NIC.
> 
> Fixes: a50245ede72a ("examples/tep_term: initialize VXLAN sample")
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>  examples/tep_termination/vxlan.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/examples/tep_termination/vxlan.c b/examples/tep_termination/vxlan.c
> index 5ee1f95..4bad33d 100644
> --- a/examples/tep_termination/vxlan.c
> +++ b/examples/tep_termination/vxlan.c
> @@ -237,6 +237,8 @@ encapsulation(struct rte_mbuf *m, uint8_t queue_id)
>  	m->outer_l2_len = sizeof(struct ether_hdr);
>  	m->outer_l3_len = sizeof(struct ipv4_hdr);
>  
> +	ol_flags |= PKT_TX_TUNNEL_VXLAN;
> +

Hi,

FYI, my testrobot caught some errors when this patch is applied.

	--yliu

---
x86_64-native-linuxapp-clang: config-all-yes
============================================
/root/dpdk/examples/tep_termination/vxlan.c:240:14: error: use of undeclared identifier 'PKT_TX_TUNNEL_VXLAN'
        ol_flags |= PKT_TX_TUNNEL_VXLAN;
                    ^
1 error generated.
make[1]: *** [vxlan.o] Error 1
make: *** [all] Error 2
error: build examples/tep_termination failed
error: build failed

...

Re: [dpdk-dev] [PATCH 1/2] examples/tep_term: fix offload on VXLAN failure

[Tan, Jianfeng]

Hi Yuanhan,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Sunday, September 11, 2016 8:09 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Ananyev, Konstantin; Wu, Jingjing; Kavanagh, Mark B;
> Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH 1/2] examples/tep_term: fix offload on
> VXLAN failure
> 
> On Thu, Aug 04, 2016 at 07:58:48AM +0000, Jianfeng Tan wrote:
> > Based on previous fix of offload on VXLAN using i40e, applications
> > need to set proper tunneling type on ol_flags so that i40e driver
> > can pass it to NIC.
> >
> > Fixes: a50245ede72a ("examples/tep_term: initialize VXLAN sample")
> >
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > ---
> >  examples/tep_termination/vxlan.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/examples/tep_termination/vxlan.c
> b/examples/tep_termination/vxlan.c
> > index 5ee1f95..4bad33d 100644
> > --- a/examples/tep_termination/vxlan.c
> > +++ b/examples/tep_termination/vxlan.c
> > @@ -237,6 +237,8 @@ encapsulation(struct rte_mbuf *m, uint8_t
> queue_id)
> >  	m->outer_l2_len = sizeof(struct ether_hdr);
> >  	m->outer_l3_len = sizeof(struct ipv4_hdr);
> >
> > +	ol_flags |= PKT_TX_TUNNEL_VXLAN;
> > +
> 
> Hi,
> 
> FYI, my testrobot caught some errors when this patch is applied.

It's because this patch set has dependency on a previous patch set, which seems a difficult scenario to handle. There's no standard to state the dependency, right?

Thanks,
Jianfeng

> 
> 	--yliu
> 
> ---
> x86_64-native-linuxapp-clang: config-all-yes
> ============================================
> /root/dpdk/examples/tep_termination/vxlan.c:240:14: error: use of
> undeclared identifier 'PKT_TX_TUNNEL_VXLAN'
>         ol_flags |= PKT_TX_TUNNEL_VXLAN;
>                     ^
> 1 error generated.
> make[1]: *** [vxlan.o] Error 1
> make: *** [all] Error 2
> error: build examples/tep_termination failed
> error: build failed
> 
> ...

Re: [dpdk-dev] [PATCH 1/2] examples/tep_term: fix offload on VXLAN failure

[Thomas Monjalon]

2016-09-12 08:42, Tan, Jianfeng:
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > FYI, my testrobot caught some errors when this patch is applied.
> 
> It's because this patch set has dependency on a previous patch set, which seems a difficult scenario to handle. There's no standard to state the dependency, right?

No there is no standard to state the dependency.
We need one. Actually, there are 3 kinds of dependencies:
	- a well know dependency when sending a patch
	- implicit dependency on the HEAD
	(can fail if a conflicting patch is pushed)
	- dependency on a specific tree (next-*)

I suggest using:
	Depends-on: pw <patchwork-id-of-the-patch>|<tree> <hash>
Examples:
	Depends-on: pw 33000
	Depends-on: master 3643b0f
	Depends-on: next-net f33e00

It won't work well when a patch depends on a pending patch series
because the cover letter has no patchwork identifier.
It will be solved with the next version of patchwork (in few months).
In the meantime, we can point to the first patch of the series.

Comments/ideas?

Re: [dpdk-dev] [PATCH 1/2] examples/tep_term: fix offload on VXLAN failure

[Tan, Jianfeng]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, September 12, 2016 5:36 PM
> To: Tan, Jianfeng; Yuanhan Liu
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] examples/tep_term: fix offload on
> VXLAN failure
> 
> 2016-09-12 08:42, Tan, Jianfeng:
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > FYI, my testrobot caught some errors when this patch is applied.
> >
> > It's because this patch set has dependency on a previous patch set, which
> seems a difficult scenario to handle. There's no standard to state the
> dependency, right?
> 
> No there is no standard to state the dependency.
> We need one. Actually, there are 3 kinds of dependencies:
> 	- a well know dependency when sending a patch
> 	- implicit dependency on the HEAD
> 	(can fail if a conflicting patch is pushed)
> 	- dependency on a specific tree (next-*)
> 
> I suggest using:
> 	Depends-on: pw <patchwork-id-of-the-patch>|<tree> <hash>
> Examples:
> 	Depends-on: pw 33000
> 	Depends-on: master 3643b0f
> 	Depends-on: next-net f33e00
> 
> It won't work well when a patch depends on a pending patch series
> because the cover letter has no patchwork identifier.
> It will be solved with the next version of patchwork (in few months).
> In the meantime, we can point to the first patch of the series.
> 
> Comments/ideas?

I think it's a great idea which can make the work of auto tools much more easier and less false positive errors. Besides, it will improve the experience of code review.

Thanks,
Jianfeng

Re: [dpdk-dev] [PATCH 1/2] examples/tep_term: fix offload on VXLAN failure

[Yuanhan Liu]

On Mon, Sep 12, 2016 at 11:36:01AM +0200, Thomas Monjalon wrote:
> 2016-09-12 08:42, Tan, Jianfeng:
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > FYI, my testrobot caught some errors when this patch is applied.
> > 
> > It's because this patch set has dependency on a previous patch set, which seems a difficult scenario to handle. There's no standard to state the dependency, right?
> 
> No there is no standard to state the dependency.

Yes.

> We need one.

We could.

> Actually, there are 3 kinds of dependencies:
> 	- a well know dependency when sending a patch

Not quite sure what it is.

> 	- implicit dependency on the HEAD

If the HEAD is in one of the DPDK public three (no matter which it is),
no issue. My robot will try to apply a patchset to all available trees,
one by one, until it succeeds.

If the HEAD is a local commit (say, like this case, you made a patch
based on some patches from community that haven't been applied yet),
it's hard. The only way we can do is that we can do some brute force
to get the right combination if no hint given. But I think of no reason
to do that: it just brings complexity. So, we could either ask a hint
from the author (the tag you mentioned below, or provide a git tree
(the alternative I'd suggest, details go below).

> 	(can fail if a conflicting patch is pushed)

It also can be fixed quite easily: when failed, it could go few commits
backwards, until a right base is found.

> 	- dependency on a specific tree (next-*)

Not an issue, as stated above.

> 
> I suggest using:
> 	Depends-on: pw <patchwork-id-of-the-patch>|<tree> <hash>
> Examples:
> 	Depends-on: pw 33000

This one is necessary, both for an auto tools and for some people to have
a try.

> 	Depends-on: master 3643b0f
> 	Depends-on: next-net f33e00

I see no good reason to do that, for that's typically where patches apply.
And since we have introduced sub-trees, it then should be obvious (for
frequent contributors) that if you are making a patch to a PMD driver,
you should grab the next-net tree and make patch there. Vice verse, if
you got a PMD patch from mailing list and want to have a try, you should
try to apply it to the next-net tree. Well, it may fail because the patch
author doesn't know this generic rule, that he made patches based on master,
you then could have a try with master.

Besides, it just adds extra burden to developers: think that we have to
add such tag to every patch.

> 
> It won't work well when a patch depends on a pending patch series
> because the cover letter has no patchwork identifier.
> It will be solved with the next version of patchwork (in few months).
> In the meantime, we can point to the first patch of the series.
> 
> Comments/ideas?

The alternative I'd suggest is to use git trees. I'm going to add the
support of git-tree-based test (hopefully, I could do that this weekend).

I'd suggest all frequent contributors to have its own DPDK tree (publicly,
say at github, or internally, only works for intel). Every one has a local
git, what you need to do is to push your local git to a remote tree. You
then tell me the tree URL, my robot will fetch your tree timely. Once you
have pushed something, it will start the build test. You could even get a
report when it finishes, if you wish.

In such case, there is no dependency at all: because the developer already
fixed that if any.

As stated, it's an alternative. People could choose the one they prefer.

	--yliu

[dpdk-dev] [PATCH 2/2] examples/tep_term: fix inner L4 checksum failure

[Jianfeng Tan]

When sending packets from virtual machine which in need of TSO
by hardware NIC, the inner L4 checksum is not correct on the
other side of the cable.

It's because get_psd_sum() depends on PKT_TX_TCP_SEG to calculate
pseudo-header checksum, but currently this bit is set after the
function get_psd_sum() is called. The fix is straightforward.
Move the bit setting before get_psd_sum() is called.

Fixes: a50245ede72a ("examples/tep_term: initialize VXLAN sample")

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 examples/tep_termination/vxlan.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/examples/tep_termination/vxlan.c b/examples/tep_termination/vxlan.c
index 4bad33d..155415c 100644
--- a/examples/tep_termination/vxlan.c
+++ b/examples/tep_termination/vxlan.c
@@ -141,14 +141,17 @@ process_inner_cksums(struct ether_hdr *eth_hdr, union tunnel_offload_info *info)
 				ethertype, ol_flags);
 	} else if (l4_proto == IPPROTO_TCP) {
 		tcp_hdr = (struct tcp_hdr *)((char *)l3_hdr + info->l3_len);
-		ol_flags |= PKT_TX_TCP_CKSUM;
-		tcp_hdr->cksum = get_psd_sum(l3_hdr, ethertype,
-				ol_flags);
+		/* Put PKT_TX_TCP_SEG bit setting before get_psd_sum(), because
+		 * it depends on PKT_TX_TCP_SEG to calculate pseudo-header
+		 * checksum.
+		 */
 		if (tso_segsz != 0) {
 			ol_flags |= PKT_TX_TCP_SEG;
 			info->tso_segsz = tso_segsz;
 			info->l4_len = sizeof(struct tcp_hdr);
 		}
+		ol_flags |= PKT_TX_TCP_CKSUM;
+		tcp_hdr->cksum = get_psd_sum(l3_hdr, ethertype, ol_flags);
 
 	} else if (l4_proto == IPPROTO_SCTP) {
 		sctp_hdr = (struct sctp_hdr *)((char *)l3_hdr + info->l3_len);
-- 
2.7.4

Re: [dpdk-dev] [PATCH 0/2] Two offloading issues of tep_term

[Thomas Monjalon]

2016-08-04 07:58, Jianfeng Tan:
> This patch set depends on:
>  - http://dpdk.org/ml/archives/dev/2016-August/044924.html
> 
> Patch 1: fill tunneling type.
> Patch 2: inner L4 checksum error.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Applied, thanks