simple change to qdisc_restart()

simple change to qdisc_restart()

[Eric Lemoine]

Hi,

Any comments regarding the following patch?

Thx.


--- sch_generic.c.old   Tue May 20 09:11:25 2003
+++ sch_generic.c       Tue May 20 10:16:11 2003
@@ -77,6 +77,7 @@
 int qdisc_restart(struct net_device *dev)
 {
        struct Qdisc *q = dev->qdisc;
+       int sched_flag = 1;
        struct sk_buff *skb;

        /* Dequeue packet */
@@ -120,6 +121,12 @@
                                        printk(KERN_DEBUG "Dead loop on netdevice %s, fix it urgently!\n", dev->name);
                                return -1;
                        }
+
+                       /* At this point we know for sure that someone 
+                        * is taking care of this Qdisc for us so we 
+                        * do not need to schedule tx softirq.
+                        */
+                       sched_flag = 0;
                        netdev_rx_stat[smp_processor_id()].cpu_collision++;
                }

@@ -134,7 +141,8 @@
                 */

                q->ops->requeue(skb, q);
-               netif_schedule(dev);
+               if (sched_flag)
+                       netif_schedule(dev);
                return 1;
        }
        return q->q.qlen
-- 
Eric

Re: simple change to qdisc_restart()

[David S. Miller]

   From: Eric Lemoine <Eric.Lemoine@Sun.COM>
   Date: Tue, 20 May 2003 10:22:17 +0200
   
   Any comments regarding the following patch?

I understand why it is valid, etc., but why do we even want to do
this?  It is not like this dead-loop detection stuff is a hot-path or
anything like that.

Re: simple change to qdisc_restart()

[Eric Lemoine]

>    From: Eric Lemoine <Eric.Lemoine@Sun.COM>
>    Date: Tue, 20 May 2003 10:22:17 +0200
>    
>    Any comments regarding the following patch?
> 
> I understand why it is valid, etc., but why do we even want to do
> this?  It is not like this dead-loop detection stuff is a hot-path or
> anything like that.

I've implemented a prototype that uses per-CPU kernel threads for
processing packets coming in from a single interface. The idea is to
apply multiple CPUs to a single network interface to be able to have
multiple CPUs simultaneously pumping data into the network. So in my
case, I have lots of cpu_collisions and running the tx softirq to do
nothing may lower the performances. Anyway, even though my patch may
help me, it may indeed be irrelevant to the stock kernel.

Thx.

-- 
Eric

Re: simple change to qdisc_restart()

[Robert Olsson]

Eric Lemoine writes:
 > >    From: Eric Lemoine <Eric.Lemoine@Sun.COM>
 > >    Date: Tue, 20 May 2003 10:22:17 +0200
 > >    
 > >    Any comments regarding the following patch?

 I think it will make any use of "raw" dev->hard_start_xmit" impossible. 
 Which is what pktgen uses.

 > > I understand why it is valid, etc., but why do we even want to do
 > > this?  It is not like this dead-loop detection stuff is a hot-path or
 > > anything like that.
 > 
 > I've implemented a prototype that uses per-CPU kernel threads for
 > processing packets coming in from a single interface. The idea is to
 > apply multiple CPUs to a single network interface to be able to have
 > multiple CPUs simultaneously pumping data into the network. So in my
 > case, I have lots of cpu_collisions and running the tx softirq to do
 > nothing may lower the performances. Anyway, even though my patch may
 > help me, it may indeed be irrelevant to the stock kernel.

 Sounds like a project at least having packet reordering and cache bouncing 
 in mind.
 

 Cheers.
						--ro
 

Re: simple change to qdisc_restart()

[Eric Lemoine]

>  > >    Any comments regarding the following patch?
> 
>  I think it will make any use of "raw" dev->hard_start_xmit" impossible. 
>  Which is what pktgen uses.
> 
>  > > I understand why it is valid, etc., but why do we even want to do
>  > > this?  It is not like this dead-loop detection stuff is a hot-path or
>  > > anything like that.
>  > 
>  > I've implemented a prototype that uses per-CPU kernel threads for
>  > processing packets coming in from a single interface. The idea is to
>  > apply multiple CPUs to a single network interface to be able to have
>  > multiple CPUs simultaneously pumping data into the network. So in my
>  > case, I have lots of cpu_collisions and running the tx softirq to do
>  > nothing may lower the performances. Anyway, even though my patch may
>  > help me, it may indeed be irrelevant to the stock kernel.
> 
>  Sounds like a project at least having packet reordering and cache bouncing 
>  in mind.

Let me explain a bit more.

I developped a kernel module that basically implements per-cpu kernel
threads, each being bound to a particular cpu. I also modified the
Myrinet NIC driver and firmware so that they implement per-cpu rx rings.

The NIC makes sure that packets of the same connection are always
deposited in the same ring. Here's how it does it. For each incoming
pkt, the NIC computes the index of the ring into which the packet must
be placed [*], passes this index to the driver, and dmas the packet into
the appropriate ring. The driver uses the ring index to wake up the
appropriate kernel thread. Each kernel-thread behaves in a NAPI manner.

-- 
Eric

Re: simple change to qdisc_restart()

[Eric Lemoine]

> Let me explain a bit more.
> 
> I developped a kernel module that basically implements per-cpu kernel
> threads, each being bound to a particular cpu. I also modified the
> Myrinet NIC driver and firmware so that they implement per-cpu rx rings.
> 
> The NIC makes sure that packets of the same connection are always
> deposited in the same ring. Here's how it does it. For each incoming
> pkt, the NIC computes the index of the ring into which the packet must
> be placed [*], passes this index to the driver, and dmas the packet into
> the appropriate ring. The driver uses the ring index to wake up the
> appropriate kernel thread. Each kernel-thread behaves in a NAPI manner.

Oops. I forgot to mention that:

[*] Currently the NIC simply does ring_idx = IPsrc & (nr_rings-1).

-- 
Eric

Re: simple change to qdisc_restart()

[Robert Olsson]

Eric Lemoine writes:

 > I developped a kernel module that basically implements per-cpu kernel
 > threads, each being bound to a particular cpu. I also modified the
 > Myrinet NIC driver and firmware so that they implement per-cpu rx rings.
 > 
 > The NIC makes sure that packets of the same connection are always
 > deposited in the same ring. Here's how it does it. For each incoming
 > pkt, the NIC computes the index of the ring into which the packet must
 > be placed [*], passes this index to the driver, and dmas the packet into
 > the appropriate ring. The driver uses the ring index to wake up the
 > appropriate kernel thread. Each kernel-thread behaves in a NAPI manner.

 OK! 
 Sounds interesting...
 So reordering should be guaranteed within "connections" but not per interface.

 And if you can repeat the trick with per-cpu rings for tx you can eventually  
 eliminate cache bouncing when sending/freeing skb's.

 We tried to tag with cpu-owner in tx-ring when doing hard_xmit and having same
 cpu sending it to do kfree but the complexity balanced the win... The thinking
 was that per-cpu tx rings could help.

 Cheers.
						--ro

Re: simple change to qdisc_restart()

[Jamal Hadi]

On Tue, 20 May 2003, Robert Olsson wrote:

>
>  Sounds interesting...
>  So reordering should be guaranteed within "connections" but not per interface.
>
>  And if you can repeat the trick with per-cpu rings for tx you can eventually
>  eliminate cache bouncing when sending/freeing skb's.
>
>  We tried to tag with cpu-owner in tx-ring when doing hard_xmit and having same
>  cpu sending it to do kfree but the complexity balanced the win... The thinking
>  was that per-cpu tx rings could help.
>

His patch should be interesting. I have seen NICs showing up in the market
with multiple DMA rings/channels and you can map flows to channels.
Locking the device on egress just because one of the rings is full doesnt
make sense. Such boards maybe the perfect pktgen board for you, btw ;->
Can you post your patch Eric?

cheers,
jamal

Re: simple change to qdisc_restart()

[Eric Lemoine]

> His patch should be interesting. I have seen NICs showing up in the market
> with multiple DMA rings/channels and you can map flows to channels.
> Locking the device on egress just because one of the rings is full doesnt
> make sense. Such boards maybe the perfect pktgen board for you, btw ;->
> Can you post your patch Eric?

I'm working on being authorized to give out my code. It may take some
time...

-- 
Eric