Re: [RFC ipsec-next] xfrm: Remove xfrmi interface ID from flowi

[Steffen Klassert]

On Tue, Jul 17, 2018 at 02:40:04PM -0700, Benedict Wong wrote:

> @@ -2301,6 +2322,13 @@ int __xfrm_policy_check(struct sock *sk, int dir, 
> struct sk_buff *skb,
>   int reverse;
>   struct flowi fl;
>   int xerr_idx = -1;
> + const struct xfrm_if_cb *ifcb;
> + struct xfrm_if *xi;
> + u32 if_id = 0;
> +
> + rcu_read_lock();
> + ifcb = xfrm_if_get_cb();
> + rcu_read_unlock();
>  
>   reverse = dir & ~XFRM_POLICY_MASK;
>   dir &= XFRM_POLICY_MASK;
> @@ -2325,10 +2353,16 @@ int __xfrm_policy_check(struct sock *sk, int dir, 
> struct sk_buff *skb,
>   }
>   }
>  
> + if (ifcb) {
> + xi = ifcb->decode_session(skb);
> + if (xi)
> + if_id = xi->p.if_id;
> + }

The usage of the ifcb pointer should go into the
rcu_read_lock section above.

Looks good otherwise, nice improvement.

Please respin and do an official submission of this
patch, I'd like to merge it before I send the pull
request for the ipsec-next tree.

[RFC ipsec-next] xfrm: Remove xfrmi interface ID from flowi

[Benedict Wong]

In order to remove performance impact of having the extra u32 in every
single flowi, this change removes the flowi_xfrm struct, prefering to
take the if_id as a method parameter where needed.

In the inbound direction, if_id is only needed during the
__xfrm_check_policy() function, and the if_id can be determined at that
point based on the skb. As such, xfrmi_decode_session() is only called
with the skb in __xfrm_check_policy().

In the outbound direction, the only place where if_id is needed is the
xfrm_lookup() call in xfrmi_xmit2(). With this change, the if_id is
directly passed into the xfrm_lookup_with_ifid() call. All existing
callers can still call xfrm_lookup(), which uses a default if_id of 0.

This change does not change any behavior of XFRMIs except for improving
overall system performance via flowi size reduction.

This change has been tested against the Android Kernel Networking Tests:

https://android.googlesource.com/kernel/tests/+/master/net/test

Signed-off-by: Benedict Wong 
---
 include/net/dst.h | 14 ++
 include/net/flow.h|  7 ---
 include/net/xfrm.h|  2 +-
 net/xfrm/xfrm_interface.c |  4 +-
 net/xfrm/xfrm_policy.c| 98 ++-
 net/xfrm/xfrm_state.c |  3 +-
 6 files changed, 83 insertions(+), 45 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index b3219cd8a5a1..7f735e76ca73 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -475,6 +475,14 @@ static inline struct dst_entry *xfrm_lookup(struct net 
*net,
return dst_orig;
 }
 
+static inline struct dst_entry *
+xfrm_lookup_with_ifid(struct net *net, struct dst_entry *dst_orig,
+ const struct flowi *fl, const struct sock *sk,
+ int flags, u32 if_id)
+{
+   return dst_orig;
+}
+
 static inline struct dst_entry *xfrm_lookup_route(struct net *net,
  struct dst_entry *dst_orig,
  const struct flowi *fl,
@@ -494,6 +502,12 @@ struct dst_entry *xfrm_lookup(struct net *net, struct 
dst_entry *dst_orig,
  const struct flowi *fl, const struct sock *sk,
  int flags);
 
+struct dst_entry *xfrm_lookup_with_ifid(struct net *net,
+   struct dst_entry *dst_orig,
+   const struct flowi *fl,
+   const struct sock *sk, int flags,
+   u32 if_id);
+
 struct dst_entry *xfrm_lookup_route(struct net *net, struct dst_entry 
*dst_orig,
const struct flowi *fl, const struct sock 
*sk,
int flags);
diff --git a/include/net/flow.h b/include/net/flow.h
index 187c9bef672f..cb1205c526ef 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -26,10 +26,6 @@ struct flowi_tunnel {
__be64  tun_id;
 };
 
-struct flowi_xfrm {
-   __u32   if_id;
-};
-
 struct flowi_common {
int flowic_oif;
int flowic_iif;
@@ -43,7 +39,6 @@ struct flowi_common {
 #define FLOWI_FLAG_SKIP_NH_OIF 0x04
__u32   flowic_secid;
struct flowi_tunnel flowic_tun_key;
-   struct flowi_xfrm xfrm;
kuid_t  flowic_uid;
 };
 
@@ -115,7 +110,6 @@ static inline void flowi4_init_output(struct flowi4 *fl4, 
int oif,
fl4->flowi4_flags = flags;
fl4->flowi4_secid = 0;
fl4->flowi4_tun_key.tun_id = 0;
-   fl4->flowi4_xfrm.if_id = 0;
fl4->flowi4_uid = uid;
fl4->daddr = daddr;
fl4->saddr = saddr;
@@ -193,7 +187,6 @@ struct flowi {
 #define flowi_secidu.__fl_common.flowic_secid
 #define flowi_tun_key  u.__fl_common.flowic_tun_key
 #define flowi_uid  u.__fl_common.flowic_uid
-#define flowi_xfrm u.__fl_common.xfrm
 } __attribute__((__aligned__(BITS_PER_LONG/8)));
 
 static inline struct flowi *flowi4_to_flowi(struct flowi4 *fl4)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index a5378613a49c..0721d899ce00 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1557,7 +1557,7 @@ struct xfrm_state *xfrm_state_find(const xfrm_address_t 
*daddr,
   const struct flowi *fl,
   struct xfrm_tmpl *tmpl,
   struct xfrm_policy *pol, int *err,
-  unsigned short family);
+  unsigned short family, u32 if_id);
 struct xfrm_state *xfrm_stateonly_find(struct net *net, u32 mark, u32 if_id,
   xfrm_address_t *daddr,
   xfrm_address_t *saddr,
diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 31cb1c7e3881..ccfe18d67e98 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -307,10 +307,8 @@ xfrmi