fq_codel and skb->hash

fq_codel and skb->hash

[Andrew Collins]

The fq_codel packet scheduler always regenerates the skb flow hash.  Is there any reason
to do this other than the recent hash perturbation additions?

I ask because I have a case where an incoming set of TCP flows is encapsulated in a
single ipsec tunnel, which is then classified on egress into a single flow by fq_codel
resulting in poor behavior.

Reusing the skb hash set in receive results in much better behavior, as the value is
determined pre-encapsulation and flows end up being distributed more naturally across
codel queues.

Is opportunistically using the pre-existing skb hash (if available) problematic?  Is
there a better way to manage flow separation in routed+encapsulated traffic?

Thanks,
Andrew Collins

Re: fq_codel and skb->hash

[Eric Dumazet]

On Mon, 2017-01-16 at 17:04 -0700, Andrew Collins wrote:
> The fq_codel packet scheduler always regenerates the skb flow hash.  Is there any reason
> to do this other than the recent hash perturbation additions?
> 
> I ask because I have a case where an incoming set of TCP flows is encapsulated in a
> single ipsec tunnel, which is then classified on egress into a single flow by fq_codel
> resulting in poor behavior.
> 
> Reusing the skb hash set in receive results in much better behavior, as the value is
> determined pre-encapsulation and flows end up being distributed more naturally across
> codel queues.



> 
> Is opportunistically using the pre-existing skb hash (if available) problematic?

I guess this would be fine.

We never exported the 'perturbation' or allowed to change/set it.

The only 'risk' would be having a buggy device setting a wrong L4
skb->hash, or buggy protocol messing with skb->hash for a given flow.

>   Is there a better way to manage flow separation in routed+encapsulated traffic?

Encapsulated traffic is fine, since flow dissector skips encap header(s)
up to the L4 header.

Problem is about encrypted traffic, since presumably this L4 header is
opaque for flow dissector ;)

Re: fq_codel and skb->hash

[Eric Dumazet]

On Mon, 2017-01-16 at 17:44 -0800, Eric Dumazet wrote:
> On Mon, 2017-01-16 at 17:04 -0700, Andrew Collins wrote:

> >   Is there a better way to manage flow separation in routed+encapsulated traffic?
> 
> Encapsulated traffic is fine, since flow dissector skips encap header(s)
> up to the L4 header.
> 
> Problem is about encrypted traffic, since presumably this L4 header is
> opaque for flow dissector ;)


BTW, how can we make sure skb->hash is populated before IPSEC ?

Relying on forwarding, and skb->hash set by a device might be not
enough.

Re: fq_codel and skb->hash

[Tom Herbert]

On Mon, Jan 16, 2017 at 6:34 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2017-01-16 at 17:44 -0800, Eric Dumazet wrote:
>> On Mon, 2017-01-16 at 17:04 -0700, Andrew Collins wrote:
>
>> >   Is there a better way to manage flow separation in routed+encapsulated traffic?
>>
>> Encapsulated traffic is fine, since flow dissector skips encap header(s)
>> up to the L4 header.
>>
>> Problem is about encrypted traffic, since presumably this L4 header is
>> opaque for flow dissector ;)
>
>
> BTW, how can we make sure skb->hash is populated before IPSEC ?
>
> Relying on forwarding, and skb->hash set by a device might be not
> enough.
>
Should we do skb_get_hash upon entry to IPsec?

Tom

>
>

Re: fq_codel and skb->hash

[Eric Dumazet]

On Mon, 2017-01-16 at 22:18 -0800, Tom Herbert wrote:
> On Mon, Jan 16, 2017 at 6:34 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Mon, 2017-01-16 at 17:44 -0800, Eric Dumazet wrote:
> >> On Mon, 2017-01-16 at 17:04 -0700, Andrew Collins wrote:
> >
> >> >   Is there a better way to manage flow separation in routed+encapsulated traffic?
> >>
> >> Encapsulated traffic is fine, since flow dissector skips encap header(s)
> >> up to the L4 header.
> >>
> >> Problem is about encrypted traffic, since presumably this L4 header is
> >> opaque for flow dissector ;)
> >
> >
> > BTW, how can we make sure skb->hash is populated before IPSEC ?
> >
> > Relying on forwarding, and skb->hash set by a device might be not
> > enough.
> >
> Should we do skb_get_hash upon entry to IPsec?

Right, but that might add overhead in cases we do not need skb->hash
after IPsec . I've heard IPsec is already quite slow :/

Re: fq_codel and skb->hash

[Andrew Collins]

On 01/17/2017 08:14 AM, Eric Dumazet wrote:
> Right, but that might add overhead in cases we do not need skb->hash
> after IPsec . I've heard IPsec is already quite slow :/

I've been running with the following change locally with good results:

--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -57,7 +57,6 @@ struct fq_codel_sched_data {
         struct fq_codel_flow *flows;    /* Flows table [flows_cnt] */
         u32             *backlogs;      /* backlog table [flows_cnt] */
         u32             flows_cnt;      /* number of flows */
-       u32             perturbation;   /* hash perturbation */
         u32             quantum;        /* psched_mtu(qdisc_dev(sch)); */
         u32             drop_batch_size;
         u32             memory_limit;
@@ -75,9 +74,7 @@ struct fq_codel_sched_data {
  static unsigned int fq_codel_hash(const struct fq_codel_sched_data *q,
                                   struct sk_buff *skb)
  {
-       u32 hash = skb_get_hash_perturb(skb, q->perturbation);
-
-       return reciprocal_scale(hash, q->flows_cnt);
+       return reciprocal_scale(skb_get_hash(skb), q->flows_cnt);
  }

  static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
@@ -482,7 +479,6 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt)
         q->memory_limit = 32 << 20; /* 32 MBytes */
         q->drop_batch_size = 64;
         q->quantum = psched_mtu(qdisc_dev(sch));
-       q->perturbation = prandom_u32();
         INIT_LIST_HEAD(&q->new_flows);
         INIT_LIST_HEAD(&q->old_flows);
         codel_params_init(&q->cparams);

Any interest in me spinning a real patch for this?  I agree that it'd be better
if we were guaranteed to get a pre-encryption flow hash for any IPsec traffic,
but in my particular case I don't care, as I control the HW and can make it give
me a hash. :)

Thanks,
Andrew Collins

Re: fq_codel and skb->hash

[Eric Dumazet]

On Wed, 2017-01-18 at 13:21 -0700, Andrew Collins wrote:

> 
> Any interest in me spinning a real patch for this?  I agree that it'd be better
> if we were guaranteed to get a pre-encryption flow hash for any IPsec traffic,
> but in my particular case I don't care, as I control the HW and can make it give
> me a hash. :)

You certainly can send this as a patch of its own.

The pre-encryption flow hash would be a separate patch and is not a
prereq.

Thanks.