* [PATCH net-next 0/3] Mitigate the two-reallocations issue for iptunnels
@ 2024-10-25 13:37 Justin Iurman
2024-10-25 13:37 ` [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue Justin Iurman
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Justin Iurman @ 2024-10-25 13:37 UTC (permalink / raw)
To: netdev
Cc: davem, dsahern, edumazet, kuba, pabeni, horms, linux-kernel,
justin.iurman
The same pattern is found in ioam6, rpl6, and seg6. Basically, it first
makes sure there is enough room for inserting a new header:
(1) err = skb_cow_head(skb, len + skb->mac_len);
Then, when the insertion (encap or inline) is performed, the input and
output handlers respectively make sure there is enough room for layer 2:
(2) err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
skb_cow_head() does nothing when there is enough room. Otherwise, it
reallocates more room, which depends on the architecture. Briefly,
skb_cow_head() calls __skb_cow() which then calls pskb_expand_head() as
follows:
pskb_expand_head(skb, ALIGN(delta, NET_SKB_PAD), 0, GFP_ATOMIC);
"delta" represents the number of bytes to be added. This value is
aligned with NET_SKB_PAD, which is defined as follows:
NET_SKB_PAD = max(32, L1_CACHE_BYTES)
... where L1_CACHE_BYTES also depends on the architecture. In our case
(x86), it is defined as follows:
L1_CACHE_BYTES = (1 << CONFIG_X86_L1_CACHE_SHIFT)
... where (again, in our case) CONFIG_X86_L1_CACHE_SHIFT equals 6
(=X86_GENERIC).
All this to say, skb_cow_head() would reallocate to the next multiple of
NET_SKB_PAD (in our case a 64-byte multiple) when there is not enough
room.
Back to the main issue with the pattern: in some cases, two
reallocations are triggered, resulting in a performance drop (i.e.,
lines (1) and (2) would both trigger an implicit reallocation). How's
that possible? Well, this is kind of bad luck as we hit an exact
NET_SKB_PAD boundary and when skb->mac_len (=14) is smaller than
LL_RESERVED_SPACE(dst->dev) (=16 in our case). For an x86 arch, it
happens in the following cases (with the default needed_headroom):
- ioam6:
- (inline mode) pre-allocated data trace of 236 or 240 bytes
- (encap mode) pre-allocated data trace of 196 or 200 bytes
- seg6:
- (encap mode) for 13, 17, 21, 25, 29, 33, ...(+4)... prefixes
Let's illustrate the problem, i.e., when we fall on the exact
NET_SKB_PAD boundary. In the case of ioam6, for the above problematic
values, the total overhead is 256 bytes for both modes. Based on line
(1), skb->mac_len (=14) is added, therefore passing 270 bytes to
skb_cow_head(). At that moment, the headroom has 206 bytes available (in
our case). Since 270 > 206, skb_cow_head() performs a reallocation and
the new headroom is now 206 + 64 (NET_SKB_PAD) = 270. Which is exactly
the room we needed. After the insertion, the headroom has 0 byte
available. But, there's line (2) where 16 bytes are still needed. Which,
again, triggers another reallocation.
The same logic is applied to seg6 (although it does not happen with the
inline mode, i.e., -40 bytes). It happens with other L1 cache shifts too
(the larger the cache shift, the less often it happens). For example,
with a +32 cache shift (instead of +64), the following number of
segments would trigger two reallocations: 11, 15, 19, ... With a +128
cache shift, the following number of segments would trigger two
reallocations: 17, 25, 33, ... And so on and so forth. Note that it is
the same for both the "encap" and "l2encap" modes. For the "encap.red"
and "l2encap.red" modes, it is the same logic but with "segs+1" (e.g.,
14, 18, 22, 26, etc for a +64 cache shift). Note also that it may happen
with rpl6 (based on some calculations), although it did not in our case.
This series provides a solution to mitigate the aforementioned issue for
ioam6, seg6, and rpl6. It provides the dst_entry (in the cache) to
skb_cow_head() **before** the insertion (line (1)). As a result, the
very first iteration would still trigger two reallocations (i.e., empty
cache), while next iterations would only trigger a single reallocation.
Justin Iurman (3):
net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue
net: ipv6: seg6_iptunnel: mitigate 2-realloc issue
net: ipv6: rpl_iptunnel: mitigate 2-realloc issue
net/ipv6/ioam6_iptunnel.c | 84 ++++++++++++++++---------------
net/ipv6/rpl_iptunnel.c | 60 +++++++++++-----------
net/ipv6/seg6_iptunnel.c | 103 +++++++++++++++++++++-----------------
3 files changed, 132 insertions(+), 115 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue
2024-10-25 13:37 [PATCH net-next 0/3] Mitigate the two-reallocations issue for iptunnels Justin Iurman
@ 2024-10-25 13:37 ` Justin Iurman
2024-10-25 15:12 ` Alexander Lobakin
2024-10-25 13:37 ` [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: " Justin Iurman
2024-10-25 13:37 ` [PATCH net-next 3/3] net: ipv6: rpl_iptunnel: " Justin Iurman
2 siblings, 1 reply; 9+ messages in thread
From: Justin Iurman @ 2024-10-25 13:37 UTC (permalink / raw)
To: netdev
Cc: davem, dsahern, edumazet, kuba, pabeni, horms, linux-kernel,
justin.iurman
This patch mitigates the two-reallocations issue with ioam6_iptunnel by
providing the dst_entry (in the cache) to the first call to
skb_cow_head(). As a result, the very first iteration would still
trigger two reallocations (i.e., empty cache), while next iterations
would only trigger a single reallocation.
Performance tests before/after applying this patch, which clearly shows
the improvement:
- inline mode:
- before: https://ibb.co/LhQ8V63
- after: https://ibb.co/x5YT2bS
- encap mode:
- before: https://ibb.co/3Cjm5m0
- after: https://ibb.co/TwpsxTC
- encap mode with tunsrc:
- before: https://ibb.co/Gpy9QPg
- after: https://ibb.co/PW1bZFT
This patch also fixes an incorrect behavior: after the insertion, the
second call to skb_cow_head() makes sure that the dev has enough
headroom in the skb for layer 2 and stuff. In that case, the "old"
dst_entry was used, which is now fixed. After discussing with Paolo, it
appears that both patches can be merged into a single one -this one-
(for the sake of readability) and target net-next.
Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
---
net/ipv6/ioam6_iptunnel.c | 84 ++++++++++++++++++++-------------------
1 file changed, 43 insertions(+), 41 deletions(-)
diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
index beb6b4cfc551..da79055fc02a 100644
--- a/net/ipv6/ioam6_iptunnel.c
+++ b/net/ipv6/ioam6_iptunnel.c
@@ -255,14 +255,16 @@ static int ioam6_do_fill(struct net *net, struct sk_buff *skb)
}
static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
- struct ioam6_lwt_encap *tuninfo)
+ struct ioam6_lwt_encap *tuninfo,
+ struct dst_entry *dst)
{
struct ipv6hdr *oldhdr, *hdr;
int hdrlen, err;
hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
- err = skb_cow_head(skb, hdrlen + skb->mac_len);
+ err = skb_cow_head(skb, hdrlen + (!dst ? skb->mac_len
+ : LL_RESERVED_SPACE(dst->dev)));
if (unlikely(err))
return err;
@@ -293,16 +295,17 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
struct ioam6_lwt_encap *tuninfo,
bool has_tunsrc,
struct in6_addr *tunsrc,
- struct in6_addr *tundst)
+ struct in6_addr *tundst,
+ struct dst_entry *dst)
{
- struct dst_entry *dst = skb_dst(skb);
struct ipv6hdr *hdr, *inner_hdr;
int hdrlen, len, err;
hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
len = sizeof(*hdr) + hdrlen;
- err = skb_cow_head(skb, len + skb->mac_len);
+ err = skb_cow_head(skb, len + (!dst ? skb->mac_len
+ : LL_RESERVED_SPACE(dst->dev)));
if (unlikely(err))
return err;
@@ -326,7 +329,7 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
if (has_tunsrc)
memcpy(&hdr->saddr, tunsrc, sizeof(*tunsrc));
else
- ipv6_dev_get_saddr(net, dst->dev, &hdr->daddr,
+ ipv6_dev_get_saddr(net, skb_dst(skb)->dev, &hdr->daddr,
IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
skb_postpush_rcsum(skb, hdr, len);
@@ -336,7 +339,7 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
- struct dst_entry *dst = skb_dst(skb);
+ struct dst_entry *dst, *orig_dst = skb_dst(skb);
struct in6_addr orig_daddr;
struct ioam6_lwt *ilwt;
int err = -EINVAL;
@@ -345,7 +348,7 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
if (skb->protocol != htons(ETH_P_IPV6))
goto drop;
- ilwt = ioam6_lwt_state(dst->lwtstate);
+ ilwt = ioam6_lwt_state(orig_dst->lwtstate);
/* Check for insertion frequency (i.e., "k over n" insertions) */
pkt_cnt = atomic_fetch_inc(&ilwt->pkt_cnt);
@@ -354,6 +357,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
orig_daddr = ipv6_hdr(skb)->daddr;
+ local_bh_disable();
+ dst = dst_cache_get(&ilwt->cache);
+ local_bh_enable();
+
switch (ilwt->mode) {
case IOAM6_IPTUNNEL_MODE_INLINE:
do_inline:
@@ -361,7 +368,7 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
if (ipv6_hdr(skb)->nexthdr == NEXTHDR_HOP)
goto out;
- err = ioam6_do_inline(net, skb, &ilwt->tuninfo);
+ err = ioam6_do_inline(net, skb, &ilwt->tuninfo, dst);
if (unlikely(err))
goto drop;
@@ -371,7 +378,7 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
/* Encapsulation (ip6ip6) */
err = ioam6_do_encap(net, skb, &ilwt->tuninfo,
ilwt->has_tunsrc, &ilwt->tunsrc,
- &ilwt->tundst);
+ &ilwt->tundst, dst);
if (unlikely(err))
goto drop;
@@ -389,45 +396,40 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
goto drop;
}
- err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
- if (unlikely(err))
- goto drop;
+ if (unlikely(!dst)) {
+ struct ipv6hdr *hdr = ipv6_hdr(skb);
+ struct flowi6 fl6;
+
+ memset(&fl6, 0, sizeof(fl6));
+ fl6.daddr = hdr->daddr;
+ fl6.saddr = hdr->saddr;
+ fl6.flowlabel = ip6_flowinfo(hdr);
+ fl6.flowi6_mark = skb->mark;
+ fl6.flowi6_proto = hdr->nexthdr;
+
+ dst = ip6_route_output(net, NULL, &fl6);
+ if (dst->error) {
+ err = dst->error;
+ dst_release(dst);
+ goto drop;
+ }
- if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr)) {
local_bh_disable();
- dst = dst_cache_get(&ilwt->cache);
+ dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
local_bh_enable();
- if (unlikely(!dst)) {
- struct ipv6hdr *hdr = ipv6_hdr(skb);
- struct flowi6 fl6;
-
- memset(&fl6, 0, sizeof(fl6));
- fl6.daddr = hdr->daddr;
- fl6.saddr = hdr->saddr;
- fl6.flowlabel = ip6_flowinfo(hdr);
- fl6.flowi6_mark = skb->mark;
- fl6.flowi6_proto = hdr->nexthdr;
-
- dst = ip6_route_output(net, NULL, &fl6);
- if (dst->error) {
- err = dst->error;
- dst_release(dst);
- goto drop;
- }
-
- local_bh_disable();
- dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
- local_bh_enable();
- }
+ err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+ if (unlikely(err))
+ goto drop;
+ }
- skb_dst_drop(skb);
- skb_dst_set(skb, dst);
+ skb_dst_drop(skb);
+ skb_dst_set(skb, dst);
+ if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr))
return dst_output(net, sk, skb);
- }
out:
- return dst->lwtstate->orig_output(net, sk, skb);
+ return orig_dst->lwtstate->orig_output(net, sk, skb);
drop:
kfree_skb(skb);
return err;
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: mitigate 2-realloc issue
2024-10-25 13:37 [PATCH net-next 0/3] Mitigate the two-reallocations issue for iptunnels Justin Iurman
2024-10-25 13:37 ` [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue Justin Iurman
@ 2024-10-25 13:37 ` Justin Iurman
2024-10-26 8:38 ` kernel test robot
2024-10-26 10:00 ` kernel test robot
2024-10-25 13:37 ` [PATCH net-next 3/3] net: ipv6: rpl_iptunnel: " Justin Iurman
2 siblings, 2 replies; 9+ messages in thread
From: Justin Iurman @ 2024-10-25 13:37 UTC (permalink / raw)
To: netdev
Cc: davem, dsahern, edumazet, kuba, pabeni, horms, linux-kernel,
justin.iurman
This patch mitigates the two-reallocations issue with seg6_iptunnel by
providing the dst_entry (in the cache) to the first call to
skb_cow_head(). As a result, the very first iteration would still
trigger two reallocations (i.e., empty cache), while next iterations
would only trigger a single reallocation.
Performance tests before/after applying this patch, which clearly shows
the improvement:
- before: https://ibb.co/3Cg4sNH
- after: https://ibb.co/8rQ350r
Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
---
net/ipv6/seg6_iptunnel.c | 103 ++++++++++++++++++++++-----------------
1 file changed, 58 insertions(+), 45 deletions(-)
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
index 098632adc9b5..7789a31db355 100644
--- a/net/ipv6/seg6_iptunnel.c
+++ b/net/ipv6/seg6_iptunnel.c
@@ -127,8 +127,14 @@ static __be32 seg6_make_flowlabel(struct net *net, struct sk_buff *skb,
/* encapsulate an IPv6 packet within an outer IPv6 header with a given SRH */
int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
{
- struct dst_entry *dst = skb_dst(skb);
- struct net *net = dev_net(dst->dev);
+ return __seg6_do_srh_encap(skb, osrh, proto, NULL);
+}
+EXPORT_SYMBOL_GPL(seg6_do_srh_encap);
+
+int __seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
+ int proto, struct dst_entry *dst)
+{
+ struct net *net = dev_net(skb_dst(skb)->dev);
struct ipv6hdr *hdr, *inner_hdr;
struct ipv6_sr_hdr *isrh;
int hdrlen, tot_len, err;
@@ -137,7 +143,8 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
hdrlen = (osrh->hdrlen + 1) << 3;
tot_len = hdrlen + sizeof(*hdr);
- err = skb_cow_head(skb, tot_len + skb->mac_len);
+ err = skb_cow_head(skb, tot_len + (!dst ? skb->mac_len
+ : LL_RESERVED_SPACE(dst->dev)));
if (unlikely(err))
return err;
@@ -181,7 +188,7 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
isrh->nexthdr = proto;
hdr->daddr = isrh->segments[isrh->first_segment];
- set_tun_src(net, dst->dev, &hdr->daddr, &hdr->saddr);
+ set_tun_src(net, skb_dst(skb)->dev, &hdr->daddr, &hdr->saddr);
#ifdef CONFIG_IPV6_SEG6_HMAC
if (sr_has_hmac(isrh)) {
@@ -197,15 +204,14 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
return 0;
}
-EXPORT_SYMBOL_GPL(seg6_do_srh_encap);
/* encapsulate an IPv6 packet within an outer IPv6 header with reduced SRH */
static int seg6_do_srh_encap_red(struct sk_buff *skb,
- struct ipv6_sr_hdr *osrh, int proto)
+ struct ipv6_sr_hdr *osrh, int proto,
+ struct dst_entry *dst)
{
__u8 first_seg = osrh->first_segment;
- struct dst_entry *dst = skb_dst(skb);
- struct net *net = dev_net(dst->dev);
+ struct net *net = dev_net(skb_dst(skb)->dev);
struct ipv6hdr *hdr, *inner_hdr;
int hdrlen = ipv6_optlen(osrh);
int red_tlv_offset, tlv_offset;
@@ -230,7 +236,8 @@ static int seg6_do_srh_encap_red(struct sk_buff *skb,
tot_len = red_hdrlen + sizeof(struct ipv6hdr);
- err = skb_cow_head(skb, tot_len + skb->mac_len);
+ err = skb_cow_head(skb, tot_len + (!dst ? skb->mac_len
+ : LL_RESERVED_SPACE(dst->dev)));
if (unlikely(err))
return err;
@@ -263,7 +270,7 @@ static int seg6_do_srh_encap_red(struct sk_buff *skb,
if (skip_srh) {
hdr->nexthdr = proto;
- set_tun_src(net, dst->dev, &hdr->daddr, &hdr->saddr);
+ set_tun_src(net, skb_dst(skb)->dev, &hdr->daddr, &hdr->saddr);
goto out;
}
@@ -299,7 +306,7 @@ static int seg6_do_srh_encap_red(struct sk_buff *skb,
srcaddr:
isrh->nexthdr = proto;
- set_tun_src(net, dst->dev, &hdr->daddr, &hdr->saddr);
+ set_tun_src(net, skb_dst(skb)->dev, &hdr->daddr, &hdr->saddr);
#ifdef CONFIG_IPV6_SEG6_HMAC
if (unlikely(!skip_srh && sr_has_hmac(isrh))) {
@@ -319,6 +326,13 @@ static int seg6_do_srh_encap_red(struct sk_buff *skb,
/* insert an SRH within an IPv6 packet, just after the IPv6 header */
int seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh)
+{
+ return __seg6_do_srh_inline(skb, osrh, NULL);
+}
+EXPORT_SYMBOL_GPL(seg6_do_srh_inline);
+
+int __seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
+ struct dst_entry *dst)
{
struct ipv6hdr *hdr, *oldhdr;
struct ipv6_sr_hdr *isrh;
@@ -326,7 +340,8 @@ int seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh)
hdrlen = (osrh->hdrlen + 1) << 3;
- err = skb_cow_head(skb, hdrlen + skb->mac_len);
+ err = skb_cow_head(skb, hdrlen + (!dst ? skb->mac_len
+ : LL_RESERVED_SPACE(dst->dev)));
if (unlikely(err))
return err;
@@ -369,22 +384,20 @@ int seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh)
return 0;
}
-EXPORT_SYMBOL_GPL(seg6_do_srh_inline);
-static int seg6_do_srh(struct sk_buff *skb)
+static int seg6_do_srh(struct sk_buff *skb, struct dst_entry *dst)
{
- struct dst_entry *dst = skb_dst(skb);
struct seg6_iptunnel_encap *tinfo;
int proto, err = 0;
- tinfo = seg6_encap_lwtunnel(dst->lwtstate);
+ tinfo = seg6_encap_lwtunnel(skb_dst(skb)->lwtstate);
switch (tinfo->mode) {
case SEG6_IPTUN_MODE_INLINE:
if (skb->protocol != htons(ETH_P_IPV6))
return -EINVAL;
- err = seg6_do_srh_inline(skb, tinfo->srh);
+ err = __seg6_do_srh_inline(skb, tinfo->srh, dst);
if (err)
return err;
break;
@@ -402,9 +415,9 @@ static int seg6_do_srh(struct sk_buff *skb)
return -EINVAL;
if (tinfo->mode == SEG6_IPTUN_MODE_ENCAP)
- err = seg6_do_srh_encap(skb, tinfo->srh, proto);
+ err = __seg6_do_srh_encap(skb, tinfo->srh, proto, dst);
else
- err = seg6_do_srh_encap_red(skb, tinfo->srh, proto);
+ err = seg6_do_srh_encap_red(skb, tinfo->srh, proto, dst);
if (err)
return err;
@@ -425,11 +438,11 @@ static int seg6_do_srh(struct sk_buff *skb)
skb_push(skb, skb->mac_len);
if (tinfo->mode == SEG6_IPTUN_MODE_L2ENCAP)
- err = seg6_do_srh_encap(skb, tinfo->srh,
- IPPROTO_ETHERNET);
+ err = __seg6_do_srh_encap(skb, tinfo->srh,
+ IPPROTO_ETHERNET, dst);
else
err = seg6_do_srh_encap_red(skb, tinfo->srh,
- IPPROTO_ETHERNET);
+ IPPROTO_ETHERNET, dst);
if (err)
return err;
@@ -453,36 +466,37 @@ static int seg6_input_finish(struct net *net, struct sock *sk,
static int seg6_input_core(struct net *net, struct sock *sk,
struct sk_buff *skb)
{
- struct dst_entry *orig_dst = skb_dst(skb);
- struct dst_entry *dst = NULL;
+ struct dst_entry *dst;
struct seg6_lwt *slwt;
int err;
- err = seg6_do_srh(skb);
- if (unlikely(err))
- goto drop;
-
- slwt = seg6_lwt_lwtunnel(orig_dst->lwtstate);
+ slwt = seg6_lwt_lwtunnel(skb_dst(skb)->lwtstate);
local_bh_disable();
dst = dst_cache_get(&slwt->cache);
+ local_bh_enable();
+
+ err = seg6_do_srh(skb, dst);
+ if (unlikely(err))
+ goto drop;
if (!dst) {
ip6_route_input(skb);
dst = skb_dst(skb);
if (!dst->error) {
+ local_bh_disable();
dst_cache_set_ip6(&slwt->cache, dst,
&ipv6_hdr(skb)->saddr);
+ local_bh_enable();
}
+
+ err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+ if (unlikely(err))
+ goto drop;
} else {
skb_dst_drop(skb);
skb_dst_set(skb, dst);
}
- local_bh_enable();
-
- err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
- if (unlikely(err))
- goto drop;
if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled))
return NF_HOOK(NFPROTO_IPV6, NF_INET_LOCAL_OUT,
@@ -523,21 +537,20 @@ static int seg6_input(struct sk_buff *skb)
static int seg6_output_core(struct net *net, struct sock *sk,
struct sk_buff *skb)
{
- struct dst_entry *orig_dst = skb_dst(skb);
- struct dst_entry *dst = NULL;
+ struct dst_entry *dst;
struct seg6_lwt *slwt;
int err;
- err = seg6_do_srh(skb);
- if (unlikely(err))
- goto drop;
-
- slwt = seg6_lwt_lwtunnel(orig_dst->lwtstate);
+ slwt = seg6_lwt_lwtunnel(skb_dst(skb)->lwtstate);
local_bh_disable();
dst = dst_cache_get(&slwt->cache);
local_bh_enable();
+ err = seg6_do_srh(skb, dst);
+ if (unlikely(err))
+ goto drop;
+
if (unlikely(!dst)) {
struct ipv6hdr *hdr = ipv6_hdr(skb);
struct flowi6 fl6;
@@ -559,15 +572,15 @@ static int seg6_output_core(struct net *net, struct sock *sk,
local_bh_disable();
dst_cache_set_ip6(&slwt->cache, dst, &fl6.saddr);
local_bh_enable();
+
+ err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+ if (unlikely(err))
+ goto drop;
}
skb_dst_drop(skb);
skb_dst_set(skb, dst);
- err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
- if (unlikely(err))
- goto drop;
-
if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled))
return NF_HOOK(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net, sk, skb,
NULL, skb_dst(skb)->dev, dst_output);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 3/3] net: ipv6: rpl_iptunnel: mitigate 2-realloc issue
2024-10-25 13:37 [PATCH net-next 0/3] Mitigate the two-reallocations issue for iptunnels Justin Iurman
2024-10-25 13:37 ` [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue Justin Iurman
2024-10-25 13:37 ` [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: " Justin Iurman
@ 2024-10-25 13:37 ` Justin Iurman
2 siblings, 0 replies; 9+ messages in thread
From: Justin Iurman @ 2024-10-25 13:37 UTC (permalink / raw)
To: netdev
Cc: davem, dsahern, edumazet, kuba, pabeni, horms, linux-kernel,
justin.iurman
This patch mitigates the two-reallocations issue with rpl_iptunnel by
providing the dst_entry (in the cache) to the first call to
skb_cow_head(). As a result, the very first iteration would still
trigger two reallocations (i.e., empty cache), while next iterations
would only trigger a single reallocation.
Performance tests before/after applying this patch, which clearly shows
there is no impact (it even shows improvement):
- before: https://ibb.co/nQJhqwc
- after: https://ibb.co/4ZvW6wV
Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
---
net/ipv6/rpl_iptunnel.c | 60 +++++++++++++++++++++--------------------
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c
index db3c19a42e1c..ce722d9ec711 100644
--- a/net/ipv6/rpl_iptunnel.c
+++ b/net/ipv6/rpl_iptunnel.c
@@ -125,7 +125,8 @@ static void rpl_destroy_state(struct lwtunnel_state *lwt)
}
static int rpl_do_srh_inline(struct sk_buff *skb, const struct rpl_lwt *rlwt,
- const struct ipv6_rpl_sr_hdr *srh)
+ const struct ipv6_rpl_sr_hdr *srh,
+ struct dst_entry *dst)
{
struct ipv6_rpl_sr_hdr *isrh, *csrh;
const struct ipv6hdr *oldhdr;
@@ -153,7 +154,8 @@ static int rpl_do_srh_inline(struct sk_buff *skb, const struct rpl_lwt *rlwt,
hdrlen = ((csrh->hdrlen + 1) << 3);
- err = skb_cow_head(skb, hdrlen + skb->mac_len);
+ err = skb_cow_head(skb, hdrlen + (!dst ? skb->mac_len
+ : LL_RESERVED_SPACE(dst->dev)));
if (unlikely(err)) {
kfree(buf);
return err;
@@ -186,36 +188,35 @@ static int rpl_do_srh_inline(struct sk_buff *skb, const struct rpl_lwt *rlwt,
return 0;
}
-static int rpl_do_srh(struct sk_buff *skb, const struct rpl_lwt *rlwt)
+static int rpl_do_srh(struct sk_buff *skb, const struct rpl_lwt *rlwt,
+ struct dst_entry *dst)
{
- struct dst_entry *dst = skb_dst(skb);
struct rpl_iptunnel_encap *tinfo;
if (skb->protocol != htons(ETH_P_IPV6))
return -EINVAL;
- tinfo = rpl_encap_lwtunnel(dst->lwtstate);
+ tinfo = rpl_encap_lwtunnel(skb_dst(skb)->lwtstate);
- return rpl_do_srh_inline(skb, rlwt, tinfo->srh);
+ return rpl_do_srh_inline(skb, rlwt, tinfo->srh, dst);
}
static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
- struct dst_entry *orig_dst = skb_dst(skb);
- struct dst_entry *dst = NULL;
+ struct dst_entry *dst;
struct rpl_lwt *rlwt;
int err;
- rlwt = rpl_lwt_lwtunnel(orig_dst->lwtstate);
-
- err = rpl_do_srh(skb, rlwt);
- if (unlikely(err))
- goto drop;
+ rlwt = rpl_lwt_lwtunnel(skb_dst(skb)->lwtstate);
local_bh_disable();
dst = dst_cache_get(&rlwt->cache);
local_bh_enable();
+ err = rpl_do_srh(skb, rlwt, dst);
+ if (unlikely(err))
+ goto drop;
+
if (unlikely(!dst)) {
struct ipv6hdr *hdr = ipv6_hdr(skb);
struct flowi6 fl6;
@@ -237,15 +238,15 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb)
local_bh_disable();
dst_cache_set_ip6(&rlwt->cache, dst, &fl6.saddr);
local_bh_enable();
+
+ err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+ if (unlikely(err))
+ goto drop;
}
skb_dst_drop(skb);
skb_dst_set(skb, dst);
- err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
- if (unlikely(err))
- goto drop;
-
return dst_output(net, sk, skb);
drop:
@@ -255,36 +256,37 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb)
static int rpl_input(struct sk_buff *skb)
{
- struct dst_entry *orig_dst = skb_dst(skb);
- struct dst_entry *dst = NULL;
+ struct dst_entry *dst;
struct rpl_lwt *rlwt;
int err;
- rlwt = rpl_lwt_lwtunnel(orig_dst->lwtstate);
-
- err = rpl_do_srh(skb, rlwt);
- if (unlikely(err))
- goto drop;
+ rlwt = rpl_lwt_lwtunnel(skb_dst(skb)->lwtstate);
local_bh_disable();
dst = dst_cache_get(&rlwt->cache);
+ local_bh_enable();
+
+ err = rpl_do_srh(skb, rlwt, dst);
+ if (unlikely(err))
+ goto drop;
if (!dst) {
ip6_route_input(skb);
dst = skb_dst(skb);
if (!dst->error) {
+ local_bh_disable();
dst_cache_set_ip6(&rlwt->cache, dst,
&ipv6_hdr(skb)->saddr);
+ local_bh_enable();
}
+
+ err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+ if (unlikely(err))
+ goto drop;
} else {
skb_dst_drop(skb);
skb_dst_set(skb, dst);
}
- local_bh_enable();
-
- err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
- if (unlikely(err))
- goto drop;
return dst_input(skb);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue
2024-10-25 13:37 ` [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue Justin Iurman
@ 2024-10-25 15:12 ` Alexander Lobakin
2024-10-25 21:06 ` Justin Iurman
0 siblings, 1 reply; 9+ messages in thread
From: Alexander Lobakin @ 2024-10-25 15:12 UTC (permalink / raw)
To: Justin Iurman
Cc: netdev, davem, dsahern, edumazet, kuba, pabeni, horms,
linux-kernel
From: Justin Iurman <justin.iurman@uliege.be>
Date: Fri, 25 Oct 2024 15:37:25 +0200
> This patch mitigates the two-reallocations issue with ioam6_iptunnel by
> providing the dst_entry (in the cache) to the first call to
> skb_cow_head(). As a result, the very first iteration would still
> trigger two reallocations (i.e., empty cache), while next iterations
> would only trigger a single reallocation.
[...]
> static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
> - struct ioam6_lwt_encap *tuninfo)
> + struct ioam6_lwt_encap *tuninfo,
> + struct dst_entry *dst)
> {
> struct ipv6hdr *oldhdr, *hdr;
> int hdrlen, err;
>
> hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
>
> - err = skb_cow_head(skb, hdrlen + skb->mac_len);
> + err = skb_cow_head(skb, hdrlen + (!dst ? skb->mac_len
> + : LL_RESERVED_SPACE(dst->dev)));
You use this pattern a lot throughout the series. I believe you should
make a static inline or a macro from it.
static inline u32 some_name(const *dst, const *skb)
{
return dst ? LL_RESERVED_SPACE(dst->dev) : skb->mac_len;
}
BTW why do you check for `!dst`, not `dst`? Does changing this affects
performance?
> if (unlikely(err))
> return err;
Thanks,
Olek
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue
2024-10-25 15:12 ` Alexander Lobakin
@ 2024-10-25 21:06 ` Justin Iurman
2024-10-29 15:08 ` Alexander Lobakin
0 siblings, 1 reply; 9+ messages in thread
From: Justin Iurman @ 2024-10-25 21:06 UTC (permalink / raw)
To: Alexander Lobakin
Cc: netdev, davem, dsahern, edumazet, kuba, pabeni, horms,
linux-kernel
On 10/25/24 17:12, Alexander Lobakin wrote:
> From: Justin Iurman <justin.iurman@uliege.be>
> Date: Fri, 25 Oct 2024 15:37:25 +0200
>
>> This patch mitigates the two-reallocations issue with ioam6_iptunnel by
>> providing the dst_entry (in the cache) to the first call to
>> skb_cow_head(). As a result, the very first iteration would still
>> trigger two reallocations (i.e., empty cache), while next iterations
>> would only trigger a single reallocation.
>
> [...]
>
>> static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
>> - struct ioam6_lwt_encap *tuninfo)
>> + struct ioam6_lwt_encap *tuninfo,
>> + struct dst_entry *dst)
>> {
>> struct ipv6hdr *oldhdr, *hdr;
>> int hdrlen, err;
>>
>> hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
>>
>> - err = skb_cow_head(skb, hdrlen + skb->mac_len);
>> + err = skb_cow_head(skb, hdrlen + (!dst ? skb->mac_len
>> + : LL_RESERVED_SPACE(dst->dev)));
>
> You use this pattern a lot throughout the series. I believe you should
> make a static inline or a macro from it.
>
> static inline u32 some_name(const *dst, const *skb)
> {
> return dst ? LL_RESERVED_SPACE(dst->dev) : skb->mac_len;
> }
>
> BTW why do you check for `!dst`, not `dst`? Does changing this affects
> performance?
Not at all, you're right... even the opposite actually. Regarding the
static inline suggestion, it could be a good idea and may even look like
this as an optimization:
static inline u32 dev_overhead(struct dst_entry *dst, struct sk_buff *skb)
{
if (likely(dst))
return LL_RESERVED_SPACE(dst->dev);
return skb->mac_len;
}
The question is... where should it go then? A static inline function per
file (i.e., ioam6_iptunnel.c, seg6_iptunnel.c, and rpl_iptunnel.c)? In
that case, it would still be repeated 3 times. Or in a header file
somewhere, to have it defined only once? If so, what location do you
think would be best?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: mitigate 2-realloc issue
2024-10-25 13:37 ` [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: " Justin Iurman
@ 2024-10-26 8:38 ` kernel test robot
2024-10-26 10:00 ` kernel test robot
1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-10-26 8:38 UTC (permalink / raw)
To: Justin Iurman, netdev
Cc: oe-kbuild-all, davem, dsahern, edumazet, kuba, pabeni, horms,
linux-kernel, justin.iurman
Hi Justin,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Justin-Iurman/net-ipv6-ioam6_iptunnel-mitigate-2-realloc-issue/20241025-214849
base: net-next/main
patch link: https://lore.kernel.org/r/20241025133727.27742-3-justin.iurman%40uliege.be
patch subject: [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: mitigate 2-realloc issue
config: arc-randconfig-001-20241026 (https://download.01.org/0day-ci/archive/20241026/202410261651.MiKpheOT-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241026/202410261651.MiKpheOT-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410261651.MiKpheOT-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
net/ipv6/seg6_iptunnel.c: In function 'seg6_do_srh_encap':
>> net/ipv6/seg6_iptunnel.c:130:16: error: implicit declaration of function '__seg6_do_srh_encap'; did you mean 'seg6_do_srh_encap'? [-Werror=implicit-function-declaration]
130 | return __seg6_do_srh_encap(skb, osrh, proto, NULL);
| ^~~~~~~~~~~~~~~~~~~
| seg6_do_srh_encap
net/ipv6/seg6_iptunnel.c: At top level:
>> net/ipv6/seg6_iptunnel.c:134:5: warning: no previous prototype for '__seg6_do_srh_encap' [-Wmissing-prototypes]
134 | int __seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
| ^~~~~~~~~~~~~~~~~~~
net/ipv6/seg6_iptunnel.c: In function 'seg6_do_srh_inline':
>> net/ipv6/seg6_iptunnel.c:330:16: error: implicit declaration of function '__seg6_do_srh_inline'; did you mean 'seg6_do_srh_inline'? [-Werror=implicit-function-declaration]
330 | return __seg6_do_srh_inline(skb, osrh, NULL);
| ^~~~~~~~~~~~~~~~~~~~
| seg6_do_srh_inline
net/ipv6/seg6_iptunnel.c: At top level:
>> net/ipv6/seg6_iptunnel.c:334:5: warning: no previous prototype for '__seg6_do_srh_inline' [-Wmissing-prototypes]
334 | int __seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
| ^~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +130 net/ipv6/seg6_iptunnel.c
126
127 /* encapsulate an IPv6 packet within an outer IPv6 header with a given SRH */
128 int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
129 {
> 130 return __seg6_do_srh_encap(skb, osrh, proto, NULL);
131 }
132 EXPORT_SYMBOL_GPL(seg6_do_srh_encap);
133
> 134 int __seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
135 int proto, struct dst_entry *dst)
136 {
137 struct net *net = dev_net(skb_dst(skb)->dev);
138 struct ipv6hdr *hdr, *inner_hdr;
139 struct ipv6_sr_hdr *isrh;
140 int hdrlen, tot_len, err;
141 __be32 flowlabel;
142
143 hdrlen = (osrh->hdrlen + 1) << 3;
144 tot_len = hdrlen + sizeof(*hdr);
145
146 err = skb_cow_head(skb, tot_len + (!dst ? skb->mac_len
147 : LL_RESERVED_SPACE(dst->dev)));
148 if (unlikely(err))
149 return err;
150
151 inner_hdr = ipv6_hdr(skb);
152 flowlabel = seg6_make_flowlabel(net, skb, inner_hdr);
153
154 skb_push(skb, tot_len);
155 skb_reset_network_header(skb);
156 skb_mac_header_rebuild(skb);
157 hdr = ipv6_hdr(skb);
158
159 /* inherit tc, flowlabel and hlim
160 * hlim will be decremented in ip6_forward() afterwards and
161 * decapsulation will overwrite inner hlim with outer hlim
162 */
163
164 if (skb->protocol == htons(ETH_P_IPV6)) {
165 ip6_flow_hdr(hdr, ip6_tclass(ip6_flowinfo(inner_hdr)),
166 flowlabel);
167 hdr->hop_limit = inner_hdr->hop_limit;
168 } else {
169 ip6_flow_hdr(hdr, 0, flowlabel);
170 hdr->hop_limit = ip6_dst_hoplimit(skb_dst(skb));
171
172 memset(IP6CB(skb), 0, sizeof(*IP6CB(skb)));
173
174 /* the control block has been erased, so we have to set the
175 * iif once again.
176 * We read the receiving interface index directly from the
177 * skb->skb_iif as it is done in the IPv4 receiving path (i.e.:
178 * ip_rcv_core(...)).
179 */
180 IP6CB(skb)->iif = skb->skb_iif;
181 }
182
183 hdr->nexthdr = NEXTHDR_ROUTING;
184
185 isrh = (void *)hdr + sizeof(*hdr);
186 memcpy(isrh, osrh, hdrlen);
187
188 isrh->nexthdr = proto;
189
190 hdr->daddr = isrh->segments[isrh->first_segment];
191 set_tun_src(net, skb_dst(skb)->dev, &hdr->daddr, &hdr->saddr);
192
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: mitigate 2-realloc issue
2024-10-25 13:37 ` [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: " Justin Iurman
2024-10-26 8:38 ` kernel test robot
@ 2024-10-26 10:00 ` kernel test robot
1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-10-26 10:00 UTC (permalink / raw)
To: Justin Iurman, netdev
Cc: llvm, oe-kbuild-all, davem, dsahern, edumazet, kuba, pabeni,
horms, linux-kernel, justin.iurman
Hi Justin,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Justin-Iurman/net-ipv6-ioam6_iptunnel-mitigate-2-realloc-issue/20241025-214849
base: net-next/main
patch link: https://lore.kernel.org/r/20241025133727.27742-3-justin.iurman%40uliege.be
patch subject: [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: mitigate 2-realloc issue
config: i386-buildonly-randconfig-004-20241026 (https://download.01.org/0day-ci/archive/20241026/202410261713.GIaQEsJC-lkp@intel.com/config)
compiler: clang version 19.1.2 (https://github.com/llvm/llvm-project 7ba7d8e2f7b6445b60679da826210cdde29eaf8b)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241026/202410261713.GIaQEsJC-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410261713.GIaQEsJC-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from net/ipv6/seg6_iptunnel.c:10:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:8:
In file included from include/linux/cacheflush.h:5:
In file included from arch/x86/include/asm/cacheflush.h:5:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> net/ipv6/seg6_iptunnel.c:130:9: error: call to undeclared function '__seg6_do_srh_encap'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
130 | return __seg6_do_srh_encap(skb, osrh, proto, NULL);
| ^
net/ipv6/seg6_iptunnel.c:130:9: note: did you mean 'seg6_do_srh_encap'?
net/ipv6/seg6_iptunnel.c:128:5: note: 'seg6_do_srh_encap' declared here
128 | int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
| ^
129 | {
130 | return __seg6_do_srh_encap(skb, osrh, proto, NULL);
| ~~~~~~~~~~~~~~~~~~~
| seg6_do_srh_encap
>> net/ipv6/seg6_iptunnel.c:134:5: warning: no previous prototype for function '__seg6_do_srh_encap' [-Wmissing-prototypes]
134 | int __seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
| ^
net/ipv6/seg6_iptunnel.c:134:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
134 | int __seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
| ^
| static
>> net/ipv6/seg6_iptunnel.c:330:9: error: call to undeclared function '__seg6_do_srh_inline'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
330 | return __seg6_do_srh_inline(skb, osrh, NULL);
| ^
net/ipv6/seg6_iptunnel.c:330:9: note: did you mean 'seg6_do_srh_inline'?
net/ipv6/seg6_iptunnel.c:328:5: note: 'seg6_do_srh_inline' declared here
328 | int seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh)
| ^
329 | {
330 | return __seg6_do_srh_inline(skb, osrh, NULL);
| ~~~~~~~~~~~~~~~~~~~~
| seg6_do_srh_inline
>> net/ipv6/seg6_iptunnel.c:334:5: warning: no previous prototype for function '__seg6_do_srh_inline' [-Wmissing-prototypes]
334 | int __seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
| ^
net/ipv6/seg6_iptunnel.c:334:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
334 | int __seg6_do_srh_inline(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
| ^
| static
3 warnings and 2 errors generated.
vim +/__seg6_do_srh_encap +130 net/ipv6/seg6_iptunnel.c
126
127 /* encapsulate an IPv6 packet within an outer IPv6 header with a given SRH */
128 int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
129 {
> 130 return __seg6_do_srh_encap(skb, osrh, proto, NULL);
131 }
132 EXPORT_SYMBOL_GPL(seg6_do_srh_encap);
133
> 134 int __seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh,
135 int proto, struct dst_entry *dst)
136 {
137 struct net *net = dev_net(skb_dst(skb)->dev);
138 struct ipv6hdr *hdr, *inner_hdr;
139 struct ipv6_sr_hdr *isrh;
140 int hdrlen, tot_len, err;
141 __be32 flowlabel;
142
143 hdrlen = (osrh->hdrlen + 1) << 3;
144 tot_len = hdrlen + sizeof(*hdr);
145
146 err = skb_cow_head(skb, tot_len + (!dst ? skb->mac_len
147 : LL_RESERVED_SPACE(dst->dev)));
148 if (unlikely(err))
149 return err;
150
151 inner_hdr = ipv6_hdr(skb);
152 flowlabel = seg6_make_flowlabel(net, skb, inner_hdr);
153
154 skb_push(skb, tot_len);
155 skb_reset_network_header(skb);
156 skb_mac_header_rebuild(skb);
157 hdr = ipv6_hdr(skb);
158
159 /* inherit tc, flowlabel and hlim
160 * hlim will be decremented in ip6_forward() afterwards and
161 * decapsulation will overwrite inner hlim with outer hlim
162 */
163
164 if (skb->protocol == htons(ETH_P_IPV6)) {
165 ip6_flow_hdr(hdr, ip6_tclass(ip6_flowinfo(inner_hdr)),
166 flowlabel);
167 hdr->hop_limit = inner_hdr->hop_limit;
168 } else {
169 ip6_flow_hdr(hdr, 0, flowlabel);
170 hdr->hop_limit = ip6_dst_hoplimit(skb_dst(skb));
171
172 memset(IP6CB(skb), 0, sizeof(*IP6CB(skb)));
173
174 /* the control block has been erased, so we have to set the
175 * iif once again.
176 * We read the receiving interface index directly from the
177 * skb->skb_iif as it is done in the IPv4 receiving path (i.e.:
178 * ip_rcv_core(...)).
179 */
180 IP6CB(skb)->iif = skb->skb_iif;
181 }
182
183 hdr->nexthdr = NEXTHDR_ROUTING;
184
185 isrh = (void *)hdr + sizeof(*hdr);
186 memcpy(isrh, osrh, hdrlen);
187
188 isrh->nexthdr = proto;
189
190 hdr->daddr = isrh->segments[isrh->first_segment];
191 set_tun_src(net, skb_dst(skb)->dev, &hdr->daddr, &hdr->saddr);
192
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue
2024-10-25 21:06 ` Justin Iurman
@ 2024-10-29 15:08 ` Alexander Lobakin
0 siblings, 0 replies; 9+ messages in thread
From: Alexander Lobakin @ 2024-10-29 15:08 UTC (permalink / raw)
To: Justin Iurman
Cc: netdev, davem, dsahern, edumazet, kuba, pabeni, horms,
linux-kernel
From: Justin Iurman <justin.iurman@uliege.be>
Date: Fri, 25 Oct 2024 23:06:36 +0200
> On 10/25/24 17:12, Alexander Lobakin wrote:
>> From: Justin Iurman <justin.iurman@uliege.be>
>> Date: Fri, 25 Oct 2024 15:37:25 +0200
>>
>>> This patch mitigates the two-reallocations issue with ioam6_iptunnel by
>>> providing the dst_entry (in the cache) to the first call to
>>> skb_cow_head(). As a result, the very first iteration would still
>>> trigger two reallocations (i.e., empty cache), while next iterations
>>> would only trigger a single reallocation.
>>
>> [...]
>>
>>> static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
>>> - struct ioam6_lwt_encap *tuninfo)
>>> + struct ioam6_lwt_encap *tuninfo,
>>> + struct dst_entry *dst)
>>> {
>>> struct ipv6hdr *oldhdr, *hdr;
>>> int hdrlen, err;
>>> hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
>>> - err = skb_cow_head(skb, hdrlen + skb->mac_len);
>>> + err = skb_cow_head(skb, hdrlen + (!dst ? skb->mac_len
>>> + : LL_RESERVED_SPACE(dst->dev)));
>>
>> You use this pattern a lot throughout the series. I believe you should
>> make a static inline or a macro from it.
>>
>> static inline u32 some_name(const *dst, const *skb)
>> {
>> return dst ? LL_RESERVED_SPACE(dst->dev) : skb->mac_len;
>> }
>>
>> BTW why do you check for `!dst`, not `dst`? Does changing this affects
>> performance?
>
> Not at all, you're right... even the opposite actually. Regarding the
> static inline suggestion, it could be a good idea and may even look like
> this as an optimization:
>
> static inline u32 dev_overhead(struct dst_entry *dst, struct sk_buff *skb)
> {
> if (likely(dst))
> return LL_RESERVED_SPACE(dst->dev);
>
> return skb->mac_len;
> }
Oh, nice!
>
> The question is... where should it go then? A static inline function per
> file (i.e., ioam6_iptunnel.c, seg6_iptunnel.c, and rpl_iptunnel.c)? In
> that case, it would still be repeated 3 times. Or in a header file
> somewhere, to have it defined only once? If so, what location do you
> think would be best?
100% should be in a header file. Can't suggest any since I don't usually
work with tunnels and ain't deep into its header structure.
Thanks,
Olek
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-29 15:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-25 13:37 [PATCH net-next 0/3] Mitigate the two-reallocations issue for iptunnels Justin Iurman
2024-10-25 13:37 ` [PATCH net-next 1/3] net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue Justin Iurman
2024-10-25 15:12 ` Alexander Lobakin
2024-10-25 21:06 ` Justin Iurman
2024-10-29 15:08 ` Alexander Lobakin
2024-10-25 13:37 ` [PATCH net-next 2/3] net: ipv6: seg6_iptunnel: " Justin Iurman
2024-10-26 8:38 ` kernel test robot
2024-10-26 10:00 ` kernel test robot
2024-10-25 13:37 ` [PATCH net-next 3/3] net: ipv6: rpl_iptunnel: " Justin Iurman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).