Re: [PATCH v2] Receive Packet Steering

[Tom Herbert <therbert@google.com>]

On Sun, May 3, 2009 at 10:30 PM, Eric Dumazet <dada1@cosmosbay.com> wrote:
>
> Tom Herbert a écrit :
> > This is an update of the receive packet steering patch (RPS) based on received
> > comments (thanks for all the comments).  Improvements are:
> >
> > 1) Removed config option for the feature.
> > 2) Made scheduling of backlog NAPI devices between CPUs lockless and much
> > simpler.
> > 3) Added new softirq to do defer sending IPIs for coalescing.
> > 4) Imported hash from simple_rx_hash.  Eliminates modulo operation to convert
> > hash to index.
> > 5) If no cpu is found for packet steering, then netif_receive_skb processes
> > packet inline as before without queueing.  In paritcular if RPS is not
> > configured on a device the receive path is unchanged from current for
> > NAPI devices (one additional conditional).
> >
> > Tom
>
> Seems cool, but I found two errors this morning before my cofee ;)
>
> Is it a working patch or an RFC ?
>
Patch mostly works.  It's based on code from an earlier kernel that
we've been running for more than year.

> Its also not clear from ChangeLog how this is working, and even
> after reading your patch, its not yet very clear. Please provide
> more documentation, on every submission.
>
Okay.

> What about latencies ? I really do think that if cpu handling
> device is lightly loaded, it should handle packet itself, without
> giving it to another cpu, incurring many cache lines bounces.
>

While it's true that this scheme adds overhead for processing a single
packet at a time, we've found that by setting the per device CPU mask
to CPUs sharing the same L2/L3 cache we can reduce that overhead
substantially to the point that even for a small number of active
connections (around ten in out setup) the benefits of parallelizing
the path overcome the extra overhead resulting in lower average
latency.  So this would increase latency for doing a single ping, but
even for a moderate loaded server we see latency improvements.

> > +static int enqueue_to_backlog(struct sk_buff *skb, int cpu)
> > +{
> > +     struct softnet_data *queue;
> > +     unsigned long flags;
> > +
> > +     queue = &per_cpu(softnet_data, cpu);
> > +     spin_lock_irqsave(&queue->input_pkt_queue.lock, flags);
>
> I wonder... isnt it going to really hurt with cache line ping pongs ?
>

I suppose it is possible, although we haven't see this pop up in
profiling.  Coalescing packets before doing the IPI might be
alleviating that.


> > +             /* Schedule NAPI for backlog device */
> > +             if (napi_schedule_prep(&queue->backlog)) {
> > +                     if (cpu != smp_processor_id()) {
> > +                             cpu_set(cpu,
> > +                                 get_cpu_var(rps_remote_softirq_cpus));
>
> get_cpu_var() increases preempt_count (preempt_disable), where is the opposite decrease ?
>

Right, should be __get_cpu_var.

Tom