On Tue, Aug 01, 2017 at 02:17:58PM -0700, Cong Wang wrote:
> On Mon, Jul 31, 2017 at 4:00 PM, Shaohua Li <s...@kernel.org> wrote:
> > On Mon, Jul 31, 2017 at 03:35:02PM -0700, Cong Wang wrote:
> >> On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li <s...@kernel.org> wrote:
> >> > static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff
> >> > *skb,
> >> > __be32 flowlabel, bool autolabel,
> >> > - struct flowi6 *fl6)
> >> > + struct flowi6 *fl6, u32 hash)
> >> > {
> >> > - u32 hash;
> >> > -
> >> > /* @flowlabel may include more than a flow label, eg, the
> >> > traffic class.
> >> > * Here we want only the flow label value.
> >> > */
> >> > @@ -788,7 +786,8 @@ static inline __be32 ip6_make_flowlabel(struct net
> >> > *net, struct sk_buff *skb,
> >> > net->ipv6.sysctl.auto_flowlabels !=
> >> > IP6_AUTO_FLOW_LABEL_FORCED))
> >> > return flowlabel;
> >> >
> >> > - hash = skb_get_hash_flowi6(skb, fl6);
> >> > + if (skb)
> >> > + hash = skb_get_hash_flowi6(skb, fl6);
> >>
> >>
> >> Why not just move skb_get_hash_flowi6() to its caller?
> >> This check is not necessary. If you don't want to touch
> >> existing callers, you can just introduce a wrapper:
> >>
> >>
> >> static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff
> >> *skb,
> >> __be32 flowlabel, bool autolabel,
> >> struct flowi6 *fl6)
> >> {
> >> u32 hash = skb_get_hash_flowi6(skb, fl6);
> >> return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
> >> }
> >
> > this will always call skb_get_hash_flowi6 for the fast path even auto
> > flowlabel
> > is disabled. I thought we should avoid this.
>
> Yeah, but you can move the check out too,
> something like:
Is this really better? I don't see any point. I'd use my original patch other
than this one. that said, there are just several lines of code, brutally
'abstract' them into a function doesn't make the code better.
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 6eac5cf8f1e6..18ffa824c00a 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -771,31 +771,22 @@ static inline void
> iph_to_flow_copy_v6addrs(struct flow_keys *flow,
>
> #define IP6_DEFAULT_AUTO_FLOW_LABELS IP6_AUTO_FLOW_LABEL_OPTOUT
>
> -static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
> - __be32 flowlabel, bool autolabel,
> - struct flowi6 *fl6)
> +static inline bool ip6_need_flowlabel(struct net *net, __be32
> flowlabel, bool autolabel)
> {
> - u32 hash;
> -
> /* @flowlabel may include more than a flow label, eg, the traffic
> class.
> * Here we want only the flow label value.
> */
> - flowlabel &= IPV6_FLOWLABEL_MASK;
> -
> - if (flowlabel ||
> + if ((flowlabel & IPV6_FLOWLABEL_MASK) ||
> net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
> (!autolabel &&
> net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
> - return flowlabel;
> -
> - hash = skb_get_hash_flowi6(skb, fl6);
> + return false;
>
> - /* Since this is being sent on the wire obfuscate hash a bit
> - * to minimize possbility that any useful information to an
> - * attacker is leaked. Only lower 20 bits are relevant.
> - */
> - rol32(hash, 16);
> + return true;
> +}
>
> +static inline __be32 __ip6_make_flowlabel(struct net *net, __be32
> flowlabel, u32 hash)
> +{
> flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
>
> if (net->ipv6.sysctl.flowlabel_state_ranges)
> @@ -804,6 +795,19 @@ static inline __be32 ip6_make_flowlabel(struct
> net *net, struct sk_buff *skb,
> return flowlabel;
> }
>
> +static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
> + __be32 flowlabel, bool autolabel,
> + struct flowi6 *fl6)
> +{
> + u32 hash;
> +
> + if (!ip6_need_flowlabel(net, flowlabel, autolabel))
> + return flowlabel & IPV6_FLOWLABEL_MASK;
> +
> + hash = skb_get_hash_flowi6(skb, fl6);
> + return __ip6_make_flowlabel(net, flowlabel, hash);
> +}
> +
> static inline int ip6_default_np_autolabel(struct net *net)
> {
> switch (net->ipv6.sysctl.auto_flowlabels) {
