diff mbox

[net-next,10/17] ipv6: fib: Add offload indication to routes

Message ID 20170719070232.28457-11-jiri@resnulli.us
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Jiri Pirko July 19, 2017, 7:02 a.m. UTC 
From: Ido Schimmel <idosch@mellanox.com>

Allow user space applications to see which routes are offloaded and
which aren't by setting the RTNH_F_OFFLOAD flag when dumping them.

To be consistent with IPv4, a multipath route is marked as offloaded if
one of its nexthops is offloaded. Individual nexthops aren't marked with
the 'offload' flag.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/uapi/linux/ipv6_route.h |  1 +
 net/ipv6/route.c                | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

Comments

David Ahern July 19, 2017, 3:27 p.m. UTC | #1 
On 7/19/17 1:02 AM, Jiri Pirko wrote:
> Allow user space applications to see which routes are offloaded and
> which aren't by setting the RTNH_F_OFFLOAD flag when dumping them.
> 
> To be consistent with IPv4, a multipath route is marked as offloaded if
> one of its nexthops is offloaded. Individual nexthops aren't marked with
> the 'offload' flag.

It is more user friendly to report the offload per nexthop especially
given the implications. There are already flags per nexthop and those
flags are pushed to userspace so not an API change at all.
Ido Schimmel July 19, 2017, 3:49 p.m. UTC | #2 
On Wed, Jul 19, 2017 at 09:27:30AM -0600, David Ahern wrote:
> On 7/19/17 1:02 AM, Jiri Pirko wrote:
> > Allow user space applications to see which routes are offloaded and
> > which aren't by setting the RTNH_F_OFFLOAD flag when dumping them.
> > 
> > To be consistent with IPv4, a multipath route is marked as offloaded if
> > one of its nexthops is offloaded. Individual nexthops aren't marked with
> > the 'offload' flag.
> 
> It is more user friendly to report the offload per nexthop especially
> given the implications. There are already flags per nexthop and those
> flags are pushed to userspace so not an API change at all.

I thought about it, but then just decided to be consistent with IPv4.

I can send a follow-up patchset that aligns both families to the
behavior you requested. Need to teach iproute2 to look for
RTNH_F_OFFLOAD in rtnh_flags as well.
David Ahern July 19, 2017, 3:53 p.m. UTC | #3 
On 7/19/17 9:49 AM, Ido Schimmel wrote:
> On Wed, Jul 19, 2017 at 09:27:30AM -0600, David Ahern wrote:
>> On 7/19/17 1:02 AM, Jiri Pirko wrote:
>>> Allow user space applications to see which routes are offloaded and
>>> which aren't by setting the RTNH_F_OFFLOAD flag when dumping them.
>>>
>>> To be consistent with IPv4, a multipath route is marked as offloaded if
>>> one of its nexthops is offloaded. Individual nexthops aren't marked with
>>> the 'offload' flag.
>>
>> It is more user friendly to report the offload per nexthop especially
>> given the implications. There are already flags per nexthop and those
>> flags are pushed to userspace so not an API change at all.
> 
> I thought about it, but then just decided to be consistent with IPv4.

And the comment stems from just that. I was looking at IPv4 ECMP routes
a few days ago and the existence / lack of offload flag was not intuitive.

> 
> I can send a follow-up patchset that aligns both families to the
> behavior you requested. Need to teach iproute2 to look for
> RTNH_F_OFFLOAD in rtnh_flags as well.
>
Ido Schimmel July 19, 2017, 4:19 p.m. UTC | #4 
On Wed, Jul 19, 2017 at 09:53:28AM -0600, David Ahern wrote:
> On 7/19/17 9:49 AM, Ido Schimmel wrote:
> > On Wed, Jul 19, 2017 at 09:27:30AM -0600, David Ahern wrote:
> >> On 7/19/17 1:02 AM, Jiri Pirko wrote:
> >>> Allow user space applications to see which routes are offloaded and
> >>> which aren't by setting the RTNH_F_OFFLOAD flag when dumping them.
> >>>
> >>> To be consistent with IPv4, a multipath route is marked as offloaded if
> >>> one of its nexthops is offloaded. Individual nexthops aren't marked with
> >>> the 'offload' flag.
> >>
> >> It is more user friendly to report the offload per nexthop especially
> >> given the implications. There are already flags per nexthop and those
> >> flags are pushed to userspace so not an API change at all.
> > 
> > I thought about it, but then just decided to be consistent with IPv4.
> 
> And the comment stems from just that. I was looking at IPv4 ECMP routes
> a few days ago and the existence / lack of offload flag was not intuitive.

Understood. I intend to change that.
diff mbox

Patch

diff --git a/include/uapi/linux/ipv6_route.h b/include/uapi/linux/ipv6_route.h
index d496c02..33e2a57 100644
--- a/include/uapi/linux/ipv6_route.h
+++ b/include/uapi/linux/ipv6_route.h
@@ -35,6 +35,7 @@ 
 #define RTF_PREF(pref)	((pref) << 27)
 #define RTF_PREF_MASK	0x18000000
 
+#define RTF_OFFLOAD	0x20000000	/* offloaded route		*/
 #define RTF_PCPU	0x40000000	/* read-only: can not be set by user */
 #define RTF_LOCAL	0x80000000
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96..924e02d 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1820,6 +1820,11 @@  static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
 		goto out;
 	}
 
+	if (cfg->fc_flags & RTF_OFFLOAD) {
+		NL_SET_ERR_MSG(extack, "Userspace can not set RTF_OFFLOAD");
+		goto out;
+	}
+
 	if (cfg->fc_dst_len > 128) {
 		NL_SET_ERR_MSG(extack, "Invalid prefix length");
 		goto out;
@@ -3327,6 +3332,9 @@  static int rt6_nexthop_info(struct sk_buff *skb, struct rt6_info *rt,
 			goto nla_put_failure;
 	}
 
+	if (rt->rt6i_flags & RTF_OFFLOAD)
+		*flags |= RTNH_F_OFFLOAD;
+
 	/* not needed for multipath encoding b/c it has a rtnexthop struct */
 	if (!skip_oif && rt->dst.dev &&
 	    nla_put_u32(skb, RTA_OIF, rt->dst.dev->ifindex))
@@ -3343,7 +3351,8 @@  static int rt6_nexthop_info(struct sk_buff *skb, struct rt6_info *rt,
 }
 
 /* add multipath next hop */
-static int rt6_add_nexthop(struct sk_buff *skb, struct rt6_info *rt)
+static int rt6_add_nexthop(struct sk_buff *skb, struct rt6_info *rt,
+			   unsigned int *rtm_flags)
 {
 	struct rtnexthop *rtnh;
 	unsigned int flags = 0;
@@ -3359,6 +3368,10 @@  static int rt6_add_nexthop(struct sk_buff *skb, struct rt6_info *rt)
 		goto nla_put_failure;
 
 	rtnh->rtnh_flags = flags;
+	if (rtnh->rtnh_flags & RTNH_F_OFFLOAD) {
+		rtnh->rtnh_flags &= ~RTNH_F_OFFLOAD;
+		*rtm_flags |= RTNH_F_OFFLOAD;
+	}
 
 	/* length of rtnetlink header + attributes */
 	rtnh->rtnh_len = nlmsg_get_pos(skb) - (void *)rtnh;
@@ -3499,12 +3512,12 @@  static int rt6_fill_node(struct net *net,
 		if (!mp)
 			goto nla_put_failure;
 
-		if (rt6_add_nexthop(skb, rt) < 0)
+		if (rt6_add_nexthop(skb, rt, &rtm->rtm_flags) < 0)
 			goto nla_put_failure;
 
 		list_for_each_entry_safe(sibling, next_sibling,
 					 &rt->rt6i_siblings, rt6i_siblings) {
-			if (rt6_add_nexthop(skb, sibling) < 0)
+			if (rt6_add_nexthop(skb, sibling, &rtm->rtm_flags) < 0)
 				goto nla_put_failure;
 		}