Hello!
> No, this is unrelated. The specific problem was a crude coversion of
> drivers to use modules, but the conversion only supported a single card via
> a single static structure.
>
> I didn't use that module conversion, and instead added module support
> independently. Thus drivers maintained "in-kernel" kept the cruft, while my
> versions never had it.
You are right.
> The reason Space.c still exists was the big problem when I tried to remove
> it. See net_init.c, which was supposed to take over the functionality of
> Space.c. But using it exclusively broke the documentation, which advised
> people to directly edit Space.c (LILO parameters and modules were not fully
> supported back then). So Space.c had to remain.
>
> I assumed that Space.c would be around only temporarily, but people then
> people continue to add ever more elaborate initialization cruft to Space.c.
>
> We can now get rid of Space.c, since no current documentation advises people
> to modify it. But it can and should be done in a way that does not break
> LILO-passed parameters, which are still required for some configurations.
I tried to kill it while 2.1 and failed miserably by the same reason. 8)
Too much of hacks... It was difficult to clean this without harm.
The attempt made recently (seems, by Alan) is great step,
but it is incomplete again. Yes, Space.c must die.
> > I do not know, I am not an expert here too. Seems, all this code is supposed
> > to be executed only under kernel lock and never executed from an
> > interrupt context. Then it should be valid.
>
> Do you mean under a global whole-kernel lock?
Yes. It was true in 2.2 and it should be still true in 2.3.
In current 2.3 only calls made from networking layer are made
not under big kernel lock.
> Not recommended? But look at what the only guide, the driver conversions,
> have as a de facto recommendation -- a horrible spinning waste of time, the
> cost of which is hidden because it's difficult to measure.
No, Donald. It does not... Well, it was not supposed to tell this at least.
Probably, it is badly written.
> > If you mean "controller" lock, it is TX lock as rule.
>
> The queue logic should default to seeing that driver Tx queue routine is
> serialized. This is a cleaner interface, results in less wasted time, and
> should almost never be a SMP performance hit. There are few (no?) drivers
> that do substantial-but-parallelizable work when queuing a packet for
> transmission.
>
> The existing practice makes it appear that the driver must always protect
> itself against simultaneous transmission attempts from multiple processors.
It is _really_ protected by top level (dev->xmit_lock).
The problem is that top level cannot provide this lock for TX completion
IRQs and, even more, driver cannot grab this top level lock too,
because this lock cannot be get from IRQ context.
So, driver is deemed either to serialize itself via tbusy (as tulip did),
or to use TX lock (as eepro100 did).
Both of the ways are described in Jamal's doc.
The second way is much simpler logically.
The first is cheaper from performance viewpoint, but it is really
complicated. It requires understanding CPU ordering, memory barriers etc.
We cannot force people to use this, actually the only known example
of flawlessly working driver using this approach was your tulip.
But I am afraid even it will not work on smp alpha or ultra,
due to absence of cpu ordering there.
> > RX locking is made automatically by IRQ serialization logic,
> > so that the question occurs only when driver needs to make something
> > serialized with RX interrupts itself. No help from top level
> > is possible here, it is an internal driver problem and driver
> > have to create its private locks, if it is necessary,
> > or, alternatively, to disable device irqs, while doing dangerous job.
> >
> > > If this is correct, should we be using separate rx and tx device-private
> > > locks?
> >
> > If driver needs them.
>
> Arrgggg. It's hard to tell what is an SMP bug and what is expected behavior
> that the driver must protect itself against.
Let me to reproduce sentence several lines ago:
> > RX locking is made automatically by IRQ serialization logic,
See?
But driver really can require separate RX path serialization
wrt control functions or wrt TX path, f.e. ne2000 needs this.
In these cases an additional technique is still required.
Such technique cannot supplied by top level by clear reasons,
if not to consdier spinlocks and disable_irq_nosync() as such technique,
certainly.
> For instance, there was a bug in (IIRC) early 2.2 kernels where an interrupt
> handler would be re-entered on SMP systems. It took a long time to track
> this down. The reaction I got (which was later corrected) was that it was
> perfectly reasonable for two processors to be running the interrupt handler
> simultaneously, and that driver should add locks to make certain that this
> worked. Yes, that could be made to work, but it would add horrible locking
> overhead for the very rare case where more than one processor could do work.
It was _my_ statement. I was beated and recognized the defeat. 8)
[ Though to be honest, I still suspect sometimes that it was right way.
Look at current IRQ engine, it is _very_ complicated due to this
service. It was not easy to recognize that such overhead
at the deepest level is good. But all this is history in any case.
]
> This is a buglet where the driver probes are called a zillion times, every
> probe called for each entry in Space.c, rather than calling the PCI (and
> other non-ISA) probes just once in an initial phase. This behavior is
> always wrong, even for the ISA drivers which should only be called multiple
> time if a card is found.
Indeed... I see, I see.
Alexey
|