Re: cipe-win32-pre9 fix|
Damion Wilson <dwilson,AT,ibl,DOT,bm>|
Thu, 10 May 2001 18:25:35 +0200|
No. Wait. Stop. The spinlock stuff causes problems in the list management.
I just finished yanking it all out because it was causing crashes due to
invalid irql. The documentation implies (Microsoft docs, go figure) that
the spinlocks should only REALLY be needed on SMP hardware. Please read the
TODO.TXT to find out what I think are the issues.
Where, exactly, is NdisFreeMemory freeing memory with different flags than
it is being allocated with ? Right now, the memory being allocated for
PacketBuffers is different than that for list nodes.
There are two queues: a packet queue and an IRP queue. The handling of all
of it should be fairly synchronous. There, of course, are no guarantees on
whether the code will be called from one thread or another, but in
IRQL_DISPATCH, there should be no preemption anyway1. Read up on it in the
DDK documentation (as crappy as it is).
I originally had an array for this stuff and moved to a linked list for
flexibility. A friend has suggested using vectors, and considering the way
the driver structure has unfolded, this would now be the best option.
However, you would not notice any performance gain unless you were using
gigabit ethernet and even then, the only way to get "real" performance gain
would be to fold the usermode tunnel handling into kernel mode thus
preventing the state changes (all the stuff that windows does to get
to/from usermode) and the general slowness of a usermode (preemptable)
process running at normal priority.
I'm against the fixed buffer space idea. It's inflexible. I'd like to
dynamically allocate everything I can and free things when they are no
longer needed. That is the direction I want to go with this. Too many
Windows programs suffer from sloppy programming practices that cause them
to scale badly. Ever wonder why Windows crashes in low memory situations
while Solaris, say, will continue running albeit slowly, in similar
On Tuesday, May 08, 2001 at 06:43:07 [ADT] Erik Wallin wrote:
> I think I've managed to fix the problems I have reported in CIPE-Win32. I
> put a
> spinlock around all the list management and cleared up a few memory
> that I think were wrong. (Memory allocated as NONCACHE and released
> this flag, or maybe it was the other way around?)
> The reason I think the spinlocks are necessary are that the packet queues
> accessed from two different threads (right?), one communicating the
> to/from NDIS, and one that talks to the service through a pipe. Did I get
> right? Or am I confused as usual. My hunch was that removing a packet
> from the
> queue at the same time as inserting one would mess upp the linked lists.
> seems to help anyway. Or it could be due to the slowdown imposed by the
> spinlocks. I'm on very shaky ground here.
> I'm attaching the two files that I have modified. As soon as I get my
> hands on
> a machine with a real diff, I'll send you just the changes. I wanted to
> this with you as soon as possible though. The version I started out with
> pre9. You should be able to view the changes with windiff from that file.
> Also, I've compiled my driver with the W2K DDK, not the NT DDK. I don't
> know if
> that makes any difference. That alone doesn't solve the problem anyway so
> changes are necessary.
> So far I've only managed to provoke a BSOD when shutting down the adapter
> the service is still running, but that should be somewhat easier to
> debug. The
> other way around doesn't produce any problems though. So the way to stop
> is to stop the service. Then, maybe one might dare stopping the adapter.
> I still get problems with the service crashing now and then. That should
> much more straightforward to debug though. And faster. I'll start looking
> that as soon as I have more time.
> The code in the driver should probably be cleaned up for performance.
> Right now
> all the list management is done under a lock, which is rather wasteful.
> One thing I thought about was to move from doubly linked lists to a fixed
> for the queues. That would save a call to malloc every time a packet
> Perhaps also reusing the buffer space would be possible. My idea here is
> allocate a fixed amount of buffer, say 1 Mb and inserting packets in it
> it's full, instead of a fixed number of packets in the queue, it would be
> fixed buffer size. But, that would be something for a future version of
> CIPE-Win32. Just thought I'd share my thoughts on that point and let you
> comment on it before I go any further in that direction.