Re: [PATCH net-next 2/4] net: mpls: change mpls_route layout

[Eric W. Biederman]

David Ahern  writes:

> diff --git a/net/mpls/internal.h b/net/mpls/internal.h
> index 66f388ba2d49..302d48f54b57 100644
> --- a/net/mpls/internal.h
> +++ b/net/mpls/internal.h
> @@ -64,7 +64,6 @@ struct mpls_dev {
>  struct sk_buff;
>  
>  #define LABEL_NOT_SPECIFIED (1 << 20)
> -#define MAX_NEW_LABELS 2
>  
>  /* This maximum ha length copied from the definition of struct neighbour */
>  #define VIA_ALEN_ALIGN sizeof(unsigned long)
> @@ -84,12 +83,25 @@ enum mpls_payload_type {
>  struct mpls_nh { /* next hop label forwarding entry */
>   struct net_device __rcu *nh_dev;
>   unsigned intnh_flags;
> - u32 nh_label[MAX_NEW_LABELS];
>   u8  nh_labels;
>   u8  nh_via_alen;
>   u8  nh_via_table;
> + /* u8 hole */

This hole probably be better documented with:
u8  nh_reserved1;
> + u32 nh_label[0];
>  };

Eric

[PATCH net-next 2/4] net: mpls: change mpls_route layout

[David Ahern]

Move labels to the end of mpls_nh as a 0-sized array and within mpls_route
move the via for a nexthop after the mpls_nh. The new layout becomes:

   +--+
   | mpls_route   |
   +--+
   | mpls_nh 0|
   +--+
   | alignment padding|   4 bytes for odd number of labels; 0 for even
   +--+
   | via[rt_max_alen] 0   |
   +--+
   | alignment padding|   via's aligned on sizeof(unsigned long)
   +--+
   | ...  |
   +--+
   | mpls_nh n-1  |
   +--+
   | via[rt_max_alen] n-1 |
   +--+

Memory allocated for nexthop + via is constant across all nexthops and
their via is based on the maximum number of labels across all nexthops and
the maximum via length. The size is saved in the mpls_route as rt_nh_size.
Accessing a nexthop becomes rt->rt_nh + index * rt->rt_nh_size.

The offset of the via address from a nexthop is saved as rt_via_offset
so that given an mpls_nh pointer the via for that hop is simply
nh + rt->rt_via_offset.

With prior code, memory allocated per mpls_route with 1 nexthop:
 via is a h/w address   - 64 bytes
 via is an ipv4 address - 64
 via is an ipv6 address - 72

With this patch set, memory allocated per mpls_route with 1 nexthop and
1 or 2 labels:
 via is a h/w address   - 56 bytes
 via is an ipv4 address - 56
 via is an ipv6 address - 64

The 8-byte reduction is due to the previous patch; the change introduced
by this patch has no impact on the size of allocations for 1 or 2 labels.

Performance impact of this change was examined using network namespaces
with veth pairs connecting namespaces. ns0 inserts the packet to the
label-switched path using an lwt route with encap mpls. ns1 adds 1 or 2
labels depending on test, ns2 (and ns3 for 2-label test) pops the label
and forwards. ns3 (or ns4) for a 2-label is the destination. Similar
series of namespaces used for 2-nexthop test.

Intent is to measure changes to latency (overhead in manipulating the
packet) in the forwarding path. Tests used netperf with UDP_RR.

IPv4: current   patches
   1 label, 1 nexthop  29908 30115
   2 label, 1 nexthop  29071 29612
   1 label, 2 nexthop  29582 29776
   2 label, 2 nexthop  29086 29149

IPv6: current   patches
   1 label, 1 nexthop  24502 24960
   2 label, 1 nexthop  24041 24407
   1 label, 2 nexthop  23795 23899
   2 label, 2 nexthop  23074 22959

In short, the change has no effect to a modest increase in performance.
This is expected since this patch does not really have an impact on routes
with 1 or 2 labels (the current limit) and 1 or 2 nexthops.

Signed-off-by: David Ahern 
---
 net/mpls/af_mpls.c  | 37 +
 net/mpls/internal.h | 43 +--
 2 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index d3dc6c43a1d1..e402e5148765 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -24,6 +24,8 @@
 #include 
 #include "internal.h"
 
+#define MAX_NEW_LABELS 2
+
 /* Maximum number of labels to look ahead at when selecting a path of
  * a multipath route
  */
@@ -60,10 +62,7 @@ EXPORT_SYMBOL_GPL(mpls_output_possible);
 
 static u8 *__mpls_nh_via(struct mpls_route *rt, struct mpls_nh *nh)
 {
-   u8 *nh0_via = PTR_ALIGN((u8 *)>rt_nh[rt->rt_nhn], VIA_ALEN_ALIGN);
-   int nh_index = nh - rt->rt_nh;
-
-   return nh0_via + rt->rt_max_alen * nh_index;
+   return (u8 *)nh + rt->rt_via_offset;
 }
 
 static const u8 *mpls_nh_via(const struct mpls_route *rt,
@@ -189,6 +188,11 @@ static u32 mpls_multipath_hash(struct mpls_route *rt, 
struct sk_buff *skb)
return hash;
 }
 
+static struct mpls_nh *mpls_get_nexthop(struct mpls_route *rt, u8 index)
+{
+   return (struct mpls_nh *)((u8 *)rt->rt_nh + index * rt->rt_nh_size);
+}
+
 static struct mpls_nh *mpls_select_multipath(struct mpls_route *rt,
 struct sk_buff *skb)
 {
@@ -201,7 +205,7 @@ static struct mpls_nh *mpls_select_multipath(struct 
mpls_route *rt,
 * one path
 */
if (rt->rt_nhn == 1)
-   goto out;
+   return rt->rt_nh;
 
if (alive <= 0)
return NULL;
@@ -219,7 +223,7 @@ static struct mpls_nh *mpls_select_multipath(struct 
mpls_route *rt,
} endfor_nexthops(rt);
 
 out:
-   return >rt_nh[nh_index];
+   return mpls_get_nexthop(rt, nh_index);
 }
 
 static bool mpls_egress(struct net *net, struct mpls_route *rt,
@@ -458,19 +462,20 @@ struct mpls_route_config {
int rc_mp_len;
 };
 
-static struct mpls_route *mpls_rt_alloc(u8 num_nh, u8 max_alen)
+/* all nexthops within a route have the same