[PATCH v2 iproute2 net-next] gre6: add collect metadata support

[PATCH v2 iproute2 net-next] gre6: add collect metadata support

[William Tu]

The patch adds 'external' option to support collect metadata
gre6 tunnel. Example of L3 and L2 gre device:
bash:~# ip link add dev ip6gre123 type ip6gre external
bash:~# ip link add dev ip6gretap123 type ip6gretap external

Signed-off-by: William Tu <u9012063@gmail.com>
---
change in v2:
  - remove "noexternal" in man page
---
 ip/link_gre6.c        | 55 ++++++++++++++++++++++++++++++++-------------------
 man/man8/ip-link.8.in |  6 ++++++
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/ip/link_gre6.c b/ip/link_gre6.c
index 0a82eaecf2cd..2cb46ca116d0 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -105,6 +105,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 	__u16 encapflags = TUNNEL_ENCAP_FLAG_CSUM6;
 	__u16 encapsport = 0;
 	__u16 encapdport = 0;
+	__u8 metadata = 0;
 	int len;
 	__u32 fwmark = 0;
 	__u32 erspan_idx = 0;
@@ -178,6 +179,9 @@ get_failed:
 		if (greinfo[IFLA_GRE_ENCAP_SPORT])
 			encapsport = rta_getattr_u16(greinfo[IFLA_GRE_ENCAP_SPORT]);
 
+		if (greinfo[IFLA_GRE_COLLECT_METADATA])
+			metadata = 1;
+
 		if (greinfo[IFLA_GRE_ENCAP_DPORT])
 			encapdport = rta_getattr_u16(greinfo[IFLA_GRE_ENCAP_DPORT]);
 
@@ -355,6 +359,8 @@ get_failed:
 			encapflags |= TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "noencap-remcsum") == 0) {
 			encapflags &= ~TUNNEL_ENCAP_FLAG_REMCSUM;
+		} else if (strcmp(*argv, "external") == 0) {
+			metadata = 1;
 		} else if (strcmp(*argv, "fwmark") == 0) {
 			NEXT_ARG();
 			if (strcmp(*argv, "inherit") == 0) {
@@ -388,26 +394,30 @@ get_failed:
 		argc--; argv++;
 	}
 
-	addattr32(n, 1024, IFLA_GRE_IKEY, ikey);
-	addattr32(n, 1024, IFLA_GRE_OKEY, okey);
-	addattr_l(n, 1024, IFLA_GRE_IFLAGS, &iflags, 2);
-	addattr_l(n, 1024, IFLA_GRE_OFLAGS, &oflags, 2);
-	addattr_l(n, 1024, IFLA_GRE_LOCAL, &laddr, sizeof(laddr));
-	addattr_l(n, 1024, IFLA_GRE_REMOTE, &raddr, sizeof(raddr));
-	if (link)
-		addattr32(n, 1024, IFLA_GRE_LINK, link);
-	addattr_l(n, 1024, IFLA_GRE_TTL, &hop_limit, 1);
-	addattr_l(n, 1024, IFLA_GRE_ENCAP_LIMIT, &encap_limit, 1);
-	addattr_l(n, 1024, IFLA_GRE_FLOWINFO, &flowinfo, 4);
-	addattr32(n, 1024, IFLA_GRE_FLAGS, flags);
-	addattr32(n, 1024, IFLA_GRE_FWMARK, fwmark);
-	if (erspan_idx != 0)
-		addattr32(n, 1024, IFLA_GRE_ERSPAN_INDEX, erspan_idx);
-
-	addattr16(n, 1024, IFLA_GRE_ENCAP_TYPE, encaptype);
-	addattr16(n, 1024, IFLA_GRE_ENCAP_FLAGS, encapflags);
-	addattr16(n, 1024, IFLA_GRE_ENCAP_SPORT, htons(encapsport));
-	addattr16(n, 1024, IFLA_GRE_ENCAP_DPORT, htons(encapdport));
+	if (!metadata) {
+		addattr32(n, 1024, IFLA_GRE_IKEY, ikey);
+		addattr32(n, 1024, IFLA_GRE_OKEY, okey);
+		addattr_l(n, 1024, IFLA_GRE_IFLAGS, &iflags, 2);
+		addattr_l(n, 1024, IFLA_GRE_OFLAGS, &oflags, 2);
+		addattr_l(n, 1024, IFLA_GRE_LOCAL, &laddr, sizeof(laddr));
+		addattr_l(n, 1024, IFLA_GRE_REMOTE, &raddr, sizeof(raddr));
+		if (link)
+			addattr32(n, 1024, IFLA_GRE_LINK, link);
+		addattr_l(n, 1024, IFLA_GRE_TTL, &hop_limit, 1);
+		addattr_l(n, 1024, IFLA_GRE_ENCAP_LIMIT, &encap_limit, 1);
+		addattr_l(n, 1024, IFLA_GRE_FLOWINFO, &flowinfo, 4);
+		addattr32(n, 1024, IFLA_GRE_FLAGS, flags);
+		addattr32(n, 1024, IFLA_GRE_FWMARK, fwmark);
+		if (erspan_idx != 0)
+			addattr32(n, 1024, IFLA_GRE_ERSPAN_INDEX, erspan_idx);
+
+		addattr16(n, 1024, IFLA_GRE_ENCAP_TYPE, encaptype);
+		addattr16(n, 1024, IFLA_GRE_ENCAP_FLAGS, encapflags);
+		addattr16(n, 1024, IFLA_GRE_ENCAP_SPORT, htons(encapsport));
+		addattr16(n, 1024, IFLA_GRE_ENCAP_DPORT, htons(encapdport));
+	} else {
+		addattr_l(n, 1024, IFLA_GRE_COLLECT_METADATA, NULL, 0);
+	}
 
 	return 0;
 }
@@ -426,6 +436,11 @@ static void gre_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (!tb)
 		return;
 
+	if (tb[IFLA_GRE_COLLECT_METADATA]) {
+		print_bool(PRINT_ANY, "collect_metadata", "external", true);
+		return;
+	}
+
 	if (tb[IFLA_GRE_FLAGS])
 		flags = rta_getattr_u32(tb[IFLA_GRE_FLAGS]);
 
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index a6a10e577b1f..eb04f887c940 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -755,6 +755,8 @@ the following additional arguments are supported:
 .BI "dscp inherit"
 ] [
 .BI dev " PHYS_DEV "
+] [
+.RB external
 ]
 
 .in +8
@@ -833,6 +835,10 @@ or
 .IR 00 ".." ff
 when tunneling non-IP packets. The default value is 00.
 
+.sp
+.RB external
+- make this tunnel externally controlled (or not, which is the default).
+
 .in -8
 
 .TP
-- 
2.7.4

Re: [PATCH v2 iproute2 net-next] gre6: add collect metadata support

[Stephen Hemminger]

On Tue,  5 Dec 2017 15:10:37 -0800
William Tu <u9012063@gmail.com> wrote:

> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
> index a6a10e577b1f..eb04f887c940 100644
> --- a/man/man8/ip-link.8.in
> +++ b/man/man8/ip-link.8.in
> @@ -755,6 +755,8 @@ the following additional arguments are supported:
>  .BI "dscp inherit"
>  ] [
>  .BI dev " PHYS_DEV "
> +] [
> +.RB external
>  ]
>  
>  .in +8
> @@ -833,6 +835,10 @@ or
>  .IR 00 ".." ff
>  when tunneling non-IP packets. The default value is 00.
>  
> +.sp
> +.RB external
> +- make this tunnel externally controlled (or not, which is the default).
> +
>  .in -8

I don't have any  direct involvement in offload, so would like some feedback
from others that are.

Not a big fan of opaque "metadata" what exactly does it mean?
Also "external" is already used to mean something else on other parts of
the link command. Also the option, and the value in JSON should be the same.

Please reconsider the naming and resubmit

The wording in the man page here could be better

Re: [PATCH v2 iproute2 net-next] gre6: add collect metadata support

[Daniel Borkmann]

On 12/06/2017 02:07 AM, Stephen Hemminger wrote:
> On Tue,  5 Dec 2017 15:10:37 -0800
> William Tu <u9012063@gmail.com> wrote:
> 
>> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
>> index a6a10e577b1f..eb04f887c940 100644
>> --- a/man/man8/ip-link.8.in
>> +++ b/man/man8/ip-link.8.in
>> @@ -755,6 +755,8 @@ the following additional arguments are supported:
>>  .BI "dscp inherit"
>>  ] [
>>  .BI dev " PHYS_DEV "
>> +] [
>> +.RB external
>>  ]
>>  
>>  .in +8
>> @@ -833,6 +835,10 @@ or
>>  .IR 00 ".." ff
>>  when tunneling non-IP packets. The default value is 00.
>>  
>> +.sp
>> +.RB external
>> +- make this tunnel externally controlled (or not, which is the default).
>> +
>>  .in -8
> 
> I don't have any  direct involvement in offload, so would like some feedback
> from others that are.
> 
> Not a big fan of opaque "metadata" what exactly does it mean?
> Also "external" is already used to mean something else on other parts of
> the link command. Also the option, and the value in JSON should be the same.

The keyword "external" to set the device into collect metadata mode
is already used heavily throughout iproute2, e.g. vxlan, geneve, ipip,
and many other device types. Special casing gre6 to not having it set
up in such way through "external" would seem quite confusing.

> Please reconsider the naming and resubmit
> 
> The wording in the man page here could be better
> 

Re: [PATCH v2 iproute2 net-next] gre6: add collect metadata support

[William Tu]

On Tue, Dec 5, 2017 at 5:40 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 12/06/2017 02:07 AM, Stephen Hemminger wrote:
>> On Tue,  5 Dec 2017 15:10:37 -0800
>> William Tu <u9012063@gmail.com> wrote:
>>
>>> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
>>> index a6a10e577b1f..eb04f887c940 100644
>>> --- a/man/man8/ip-link.8.in
>>> +++ b/man/man8/ip-link.8.in
>>> @@ -755,6 +755,8 @@ the following additional arguments are supported:
>>>  .BI "dscp inherit"
>>>  ] [
>>>  .BI dev " PHYS_DEV "
>>> +] [
>>> +.RB external
>>>  ]
>>>
>>>  .in +8
>>> @@ -833,6 +835,10 @@ or
>>>  .IR 00 ".." ff
>>>  when tunneling non-IP packets. The default value is 00.
>>>
>>> +.sp
>>> +.RB external
>>> +- make this tunnel externally controlled (or not, which is the default).
>>> +
>>>  .in -8
>>
>> I don't have any  direct involvement in offload, so would like some feedback
>> from others that are.
>>
>> Not a big fan of opaque "metadata" what exactly does it mean?
>> Also "external" is already used to mean something else on other parts of
>> the link command. Also the option, and the value in JSON should be the same.
>
> The keyword "external" to set the device into collect metadata mode
> is already used heavily throughout iproute2, e.g. vxlan, geneve, ipip,
> and many other device types. Special casing gre6 to not having it set
> up in such way through "external" would seem quite confusing.
>
>> Please reconsider the naming and resubmit
>>
>> The wording in the man page here could be better
>>
>
Hi Stephen and Daniel,
Thanks for the feedbacks.

For the next version, I will remain using the keyword "external" since
it consists with other tunnels.
As for JSON, since geneve and vxlan is useing "collect_metadata" for
external keyword, should I keep using "collect_metadata" for ip6gre?
root@:~/iproute2# git grep collect_metadata
ip/iplink_geneve.c:             print_bool(PRINT_ANY,
"collect_metadata", "external ", true);
ip/iplink_vxlan.c:              print_bool(PRINT_ANY,
"collect_metadata", "external ", true);

Or I can submit another patch to make them all use "external".
Thanks
William