Re: [patch 4/10] s390: network driver.

[Jeff Garzik <jgarzik@xxxxxxxxx>]

jamal wrote:
> On Wed, 2004-12-15 at 10:03, Thomas Spatzier wrote:
>
>
> > jamal <hadi@xxxxxxxxxx> wrote on 15.12.2004 14:50:27:
> >
> >
> > > When you netif_stop_queue you should never receive packets anymore
> > > at the device level. If you receive any its a bug and you should drop
> > > them and bitch violently. In other words i think what you have at the
> > > moment is bandaid not the solution.
> >
> > When we do a netif_stop_queue, we do not get any more packets.
> > So this behaviour is ok. The problem is that the socket write
> > queues fill up then and get blocked as soon as they are full.
>
>
> This is the strange part. Anyone still willing to provide a sample
> program that hangs?
>
>
> > > Can you describe how your driver uses the netif_start/stop/wake
> > > interfaces?
> >
> > Before the patch we are talking about:
> > When we detect a cable pull (or something like this) we call
> > netif_stop_queue and set switch off the IFF_RUNNING flag.
> > Then when we detect that the cable is plugged in again, we
> > call netif_wake_queue and switch the IFF_RUNNING flag on.
>
>
> Not too bad except user space doesnt get async notification.
>
>
> > And with the patch:
> > On cable pull we switch off IFF_RUNNING and call
> > netif_carrier_off. We still get packets but drop them.
> > When the cable is plugged in we switch on IFF_RUNNING and
> > call netif_carrier_on.
>
>
> Ok, thats something you need to change.
> Why you are setting that IFF_RUNNING flag?
> Look at e1000 they do it right there. 
>
> On link up:
>         netif_carrier_on(netdev);
>         netif_wake_queue(netdev);
> On Link Down:
>         netif_carrier_off(netdev); // wonder if these need swapping
>         netif_stop_queue(netdev);
>
> Still, I think we need to resolve the original problem.
> And I have a feeling we wont be seeing any program which 
> reproduces it ;->


I've been watching this thread, and am still waiting to see a good, 
isolated test case.

My initial reaction was based on an observation that
(a) the proposed s390 change creates a CPU cycle soaker, a /dev/null for 
skbs.
(b) it really sounds like the userland program is doing something broken

Even if (b) is not true, the change is unacceptable due to (a) regardless.

        Jeff