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(>new_flows);
INIT_LIST_HEAD(>old_flows);
codel_params_init(>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.

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  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

[Tom Herbert]

On Mon, Jan 16, 2017 at 6:34 PM, Eric Dumazet  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 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

[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 ;)

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