| Message ID | 20210802205133.24071-1-justin.iurman@uliege.be (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] ipv6: Attempt to improve options code parsing | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 137 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 8/2/21 10:51 PM, Justin Iurman wrote: > As per Eric's comment on a previous patchset that was adding a new HopbyHop > option, i.e. why should a new option appear before or after existing ones in the > list, here is an attempt to suppress such competition. It also improves the > efficiency and fasten the process of matching a Hbh or Dst option, which is > probably something we want regarding the list of new options that could quickly > grow in the future. > > Basically, the two "lists" of options (Hbh and Dst) are replaced by two arrays. > Each array has a size of 256 (for each code point). Each code point points to a > function to process its specific option. > > Thoughts? > Hi Justin I think this still suffers from indirect call costs (CONFIG_RETPOLINE=y), and eventually use more dcache. Since we only deal with two sets/arrays, I would simply get rid of them and inline the code using two switch() clauses. I cooked the following patch instead: net/ipv6/exthdrs.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------- 1 file changed, 46 insertions(+), 56 deletions(-) diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c index d897faa4e9e63831b9b4f0ad0e59bf7032b2bd96..5acdc62bb5419b81cac46c935d7436f490dc3e74 100644 --- a/net/ipv6/exthdrs.c +++ b/net/ipv6/exthdrs.c @@ -55,19 +55,6 @@ #include <linux/uaccess.h> -/* - * Parsing tlv encoded headers. - * - * Parsing function "func" returns true, if parsing succeed - * and false, if it failed. - * It MUST NOT touch skb->h. - */ - -struct tlvtype_proc { - int type; - bool (*func)(struct sk_buff *skb, int offset); -}; - /********************* Generic functions *********************/ @@ -112,9 +99,17 @@ static bool ip6_tlvopt_unknown(struct sk_buff *skb, int optoff, return false; } +static bool ipv6_hop_ra(struct sk_buff *skb, int optoff); +static bool ipv6_hop_ioam(struct sk_buff *skb, int optoff); +static bool ipv6_hop_jumbo(struct sk_buff *skb, int optoff); +static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff); +#if IS_ENABLED(CONFIG_IPV6_MIP6) +static bool ipv6_dest_hao(struct sk_buff *skb, int optoff); +#endif + /* Parse tlv encoded option header (hop-by-hop or destination) */ -static bool ip6_parse_tlv(const struct tlvtype_proc *procs, +static bool ip6_parse_tlv(bool hopbyhop, struct sk_buff *skb, int max_count) { @@ -176,20 +171,45 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, if (tlv_count > max_count) goto bad; - for (curr = procs; curr->type >= 0; curr++) { - if (curr->type == nh[off]) { - /* type specific length/alignment - checks will be performed in the - func(). */ - if (curr->func(skb, off) == false) + if (hopbyhop) { + switch (nh[off]) { + case IPV6_TLV_ROUTERALERT: + if (!ipv6_hop_ra(skb, off)) + return false; + break; + case IPV6_TLV_IOAM: + if (!ipv6_hop_ioam(skb, off)) + return false; + break; + case IPV6_TLV_JUMBO: + if (!ipv6_hop_jumbo(skb, off)) + return false; + break; + case IPV6_TLV_CALIPSO: + if (!ipv6_hop_calipso(skb, off)) + return false; + break; + default: + if (!ip6_tlvopt_unknown(skb, off, + disallow_unknowns)) + return false; + break; + } + } else { + switch (nh[off]) { +#if IS_ENABLED(CONFIG_IPV6_MIP6) + case IPV6_TLV_HAO: + if (!ipv6_dest_hao(skb, off)) + return false; + break; +#endif + default: + if (!ip6_tlvopt_unknown(skb, off, + disallow_unknowns)) return false; break; } } - if (curr->type < 0 && - !ip6_tlvopt_unknown(skb, off, disallow_unknowns)) - return false; - padlen = 0; } off += optlen; @@ -267,16 +287,6 @@ static bool ipv6_dest_hao(struct sk_buff *skb, int optoff) } #endif -static const struct tlvtype_proc tlvprocdestopt_lst[] = { -#if IS_ENABLED(CONFIG_IPV6_MIP6) - { - .type = IPV6_TLV_HAO, - .func = ipv6_dest_hao, - }, -#endif - {-1, NULL} -}; - static int ipv6_destopt_rcv(struct sk_buff *skb) { struct inet6_dev *idev = __in6_dev_get(skb->dev); @@ -307,7 +317,7 @@ static int ipv6_destopt_rcv(struct sk_buff *skb) dstbuf = opt->dst1; #endif - if (ip6_parse_tlv(tlvprocdestopt_lst, skb, + if (ip6_parse_tlv(false, skb, net->ipv6.sysctl.max_dst_opts_cnt)) { skb->transport_header += extlen; opt = IP6CB(skb); @@ -1051,26 +1061,6 @@ static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff) return false; } -static const struct tlvtype_proc tlvprochopopt_lst[] = { - { - .type = IPV6_TLV_ROUTERALERT, - .func = ipv6_hop_ra, - }, - { - .type = IPV6_TLV_IOAM, - .func = ipv6_hop_ioam, - }, - { - .type = IPV6_TLV_JUMBO, - .func = ipv6_hop_jumbo, - }, - { - .type = IPV6_TLV_CALIPSO, - .func = ipv6_hop_calipso, - }, - { -1, } -}; - int ipv6_parse_hopopts(struct sk_buff *skb) { struct inet6_skb_parm *opt = IP6CB(skb); @@ -1096,7 +1086,7 @@ int ipv6_parse_hopopts(struct sk_buff *skb) goto fail_and_free; opt->flags |= IP6SKB_HOPBYHOP; - if (ip6_parse_tlv(tlvprochopopt_lst, skb, + if (ip6_parse_tlv(true, skb, net->ipv6.sysctl.max_hbh_opts_cnt)) { skb->transport_header += extlen; opt = IP6CB(skb);
>> As per Eric's comment on a previous patchset that was adding a new HopbyHop >> option, i.e. why should a new option appear before or after existing ones in the >> list, here is an attempt to suppress such competition. It also improves the >> efficiency and fasten the process of matching a Hbh or Dst option, which is >> probably something we want regarding the list of new options that could quickly >> grow in the future. >> >> Basically, the two "lists" of options (Hbh and Dst) are replaced by two arrays. >> Each array has a size of 256 (for each code point). Each code point points to a >> function to process its specific option. >> >> Thoughts? >> > Hi Justin > > I think this still suffers from indirect call costs (CONFIG_RETPOLINE=y), > and eventually use more dcache. Agree with both. It was the compromise for such a solution, unfortunately. > Since we only deal with two sets/arrays, I would simply get rid of them > and inline the code using two switch() clauses. Indeed, this is the more efficient. However, we still have two "issues": - ip6_parse_tlv will keep growing and code could look ugly at some point - there is still a "competition" between options, i.e. "I want to be at the top of the list" Anyway, your solution is better than the current one so it's probably the way to go right now.
On 8/3/21 6:06 PM, Justin Iurman wrote: >>> As per Eric's comment on a previous patchset that was adding a new HopbyHop >>> option, i.e. why should a new option appear before or after existing ones in the >>> list, here is an attempt to suppress such competition. It also improves the >>> efficiency and fasten the process of matching a Hbh or Dst option, which is >>> probably something we want regarding the list of new options that could quickly >>> grow in the future. >>> >>> Basically, the two "lists" of options (Hbh and Dst) are replaced by two arrays. >>> Each array has a size of 256 (for each code point). Each code point points to a >>> function to process its specific option. >>> >>> Thoughts? >>> >> Hi Justin >> >> I think this still suffers from indirect call costs (CONFIG_RETPOLINE=y), >> and eventually use more dcache. > > Agree with both. It was the compromise for such a solution, unfortunately. > >> Since we only deal with two sets/arrays, I would simply get rid of them >> and inline the code using two switch() clauses. > > Indeed, this is the more efficient. However, we still have two "issues": > - ip6_parse_tlv will keep growing and code could look ugly at some point Well, in 10 years there has not been a lot of growth. > - there is still a "competition" between options, i.e. "I want to be at the top of the list" Why would that be ? A switch() is compiled with no particular order by the compiler. Code generation depends on case density, and will use bisection-like strategy. > > Anyway, your solution is better than the current one so it's probably the way to go right now. >
>>>> As per Eric's comment on a previous patchset that was adding a new HopbyHop >>>> option, i.e. why should a new option appear before or after existing ones in the >>>> list, here is an attempt to suppress such competition. It also improves the >>>> efficiency and fasten the process of matching a Hbh or Dst option, which is >>>> probably something we want regarding the list of new options that could quickly >>>> grow in the future. >>>> >>>> Basically, the two "lists" of options (Hbh and Dst) are replaced by two arrays. >>>> Each array has a size of 256 (for each code point). Each code point points to a >>>> function to process its specific option. >>>> >>>> Thoughts? >>>> >>> Hi Justin >>> >>> I think this still suffers from indirect call costs (CONFIG_RETPOLINE=y), >>> and eventually use more dcache. >> >> Agree with both. It was the compromise for such a solution, unfortunately. >> >>> Since we only deal with two sets/arrays, I would simply get rid of them >>> and inline the code using two switch() clauses. >> >> Indeed, this is the more efficient. However, we still have two "issues": >> - ip6_parse_tlv will keep growing and code could look ugly at some point > > Well, in 10 years there has not been a lot of growth. Indeed, but I think it could grow a lot more in short/middle term. Just have a look at current discussions in the IETF (e.g., 6man) about HopbyHop limitations and anything related, as a way to widely improve their support and not just drop them. A better support could bring new options.
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c index d897faa4e9e6..4687fe456608 100644 --- a/net/ipv6/exthdrs.c +++ b/net/ipv6/exthdrs.c @@ -55,19 +55,6 @@ #include <linux/uaccess.h> -/* - * Parsing tlv encoded headers. - * - * Parsing function "func" returns true, if parsing succeed - * and false, if it failed. - * It MUST NOT touch skb->h. - */ - -struct tlvtype_proc { - int type; - bool (*func)(struct sk_buff *skb, int offset); -}; - /********************* Generic functions *********************/ @@ -114,14 +101,14 @@ static bool ip6_tlvopt_unknown(struct sk_buff *skb, int optoff, /* Parse tlv encoded option header (hop-by-hop or destination) */ -static bool ip6_parse_tlv(const struct tlvtype_proc *procs, +static bool ip6_parse_tlv(bool (**procs)(struct sk_buff *skb, int offset), struct sk_buff *skb, int max_count) { int len = (skb_transport_header(skb)[1] + 1) << 3; const unsigned char *nh = skb_network_header(skb); + bool (*func)(struct sk_buff *skb, int offset); int off = skb_network_header_len(skb); - const struct tlvtype_proc *curr; bool disallow_unknowns = false; int tlv_count = 0; int padlen = 0; @@ -176,19 +163,17 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, if (tlv_count > max_count) goto bad; - for (curr = procs; curr->type >= 0; curr++) { - if (curr->type == nh[off]) { - /* type specific length/alignment - checks will be performed in the - func(). */ - if (curr->func(skb, off) == false) - return false; - break; - } - } - if (curr->type < 0 && - !ip6_tlvopt_unknown(skb, off, disallow_unknowns)) + func = procs[nh[off]]; + if (func) { + /* type specific length/alignment checks + * will be performed in the func(). + */ + if (func(skb, off) == false) + return false; + } else if (!ip6_tlvopt_unknown(skb, off, + disallow_unknowns)) { return false; + } padlen = 0; } @@ -267,14 +252,16 @@ static bool ipv6_dest_hao(struct sk_buff *skb, int optoff) } #endif -static const struct tlvtype_proc tlvprocdestopt_lst[] = { +/* Parsing tlv encoded headers for Destination Options. + * + * Parsing functions below return true, if parsing succeed + * and false, if it failed. + * They MUST NOT touch skb->h. + */ +static bool (*tlvprocdestopt[256])(struct sk_buff *skb, int offset) = { #if IS_ENABLED(CONFIG_IPV6_MIP6) - { - .type = IPV6_TLV_HAO, - .func = ipv6_dest_hao, - }, + [IPV6_TLV_HAO] = ipv6_dest_hao, #endif - {-1, NULL} }; static int ipv6_destopt_rcv(struct sk_buff *skb) @@ -307,7 +294,7 @@ static int ipv6_destopt_rcv(struct sk_buff *skb) dstbuf = opt->dst1; #endif - if (ip6_parse_tlv(tlvprocdestopt_lst, skb, + if (ip6_parse_tlv(tlvprocdestopt, skb, net->ipv6.sysctl.max_dst_opts_cnt)) { skb->transport_header += extlen; opt = IP6CB(skb); @@ -1051,24 +1038,17 @@ static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff) return false; } -static const struct tlvtype_proc tlvprochopopt_lst[] = { - { - .type = IPV6_TLV_ROUTERALERT, - .func = ipv6_hop_ra, - }, - { - .type = IPV6_TLV_IOAM, - .func = ipv6_hop_ioam, - }, - { - .type = IPV6_TLV_JUMBO, - .func = ipv6_hop_jumbo, - }, - { - .type = IPV6_TLV_CALIPSO, - .func = ipv6_hop_calipso, - }, - { -1, } +/* Parsing tlv encoded headers for HopbyHop Options. + * + * Parsing functions below return true, if parsing succeed + * and false, if it failed. + * They MUST NOT touch skb->h. + */ +static bool (*tlvprochopopt[256])(struct sk_buff *skb, int offset) = { + [IPV6_TLV_ROUTERALERT] = ipv6_hop_ra, + [IPV6_TLV_CALIPSO] = ipv6_hop_calipso, + [IPV6_TLV_IOAM] = ipv6_hop_ioam, + [IPV6_TLV_JUMBO] = ipv6_hop_jumbo, }; int ipv6_parse_hopopts(struct sk_buff *skb) @@ -1096,7 +1076,7 @@ int ipv6_parse_hopopts(struct sk_buff *skb) goto fail_and_free; opt->flags |= IP6SKB_HOPBYHOP; - if (ip6_parse_tlv(tlvprochopopt_lst, skb, + if (ip6_parse_tlv(tlvprochopopt, skb, net->ipv6.sysctl.max_hbh_opts_cnt)) { skb->transport_header += extlen; opt = IP6CB(skb);
As per Eric's comment on a previous patchset that was adding a new HopbyHop option, i.e. why should a new option appear before or after existing ones in the list, here is an attempt to suppress such competition. It also improves the efficiency and fasten the process of matching a Hbh or Dst option, which is probably something we want regarding the list of new options that could quickly grow in the future. Basically, the two "lists" of options (Hbh and Dst) are replaced by two arrays. Each array has a size of 256 (for each code point). Each code point points to a function to process its specific option. Thoughts? Signed-off-by: Justin Iurman <justin.iurman@uliege.be> --- net/ipv6/exthdrs.c | 86 ++++++++++++++++++---------------------------- 1 file changed, 33 insertions(+), 53 deletions(-)