Thread: Re: [Ndiswrapper-general] Massive Memory Leak
Status: Beta
Brought to you by:
pgiri
From: <ker...@co...> - 2005-02-21 13:11:56
|
> a chance to. By the way, which version did you apply it to? That is, > which version did you make the changes to? I have only been able to > get version 1.0 to work for me, as I said before. Once this leak is > fixed for good, I should be able to use my wireless. > > Jonathan > I forgot to clarify that the patch is against ndiswrapper 1.0 sources. So you should be good to try the patch. Also, ignore the second mail - KMail messed one of the two - I wasn't sure which. Thanks! Parag |
From: <ker...@co...> - 2005-02-22 03:53:27
|
> I wanted to acknowledge Parag for pointing out the problem in CVS log, but > forgot to do so. Sorry about that. So here goes - thanks Parag for your > work. > > -- > Giri Hehe - No problem with that, I can't be more happy - I can run all the 64bits without a wire, non stop :) ! Many Thanks for the excellent work. Parag |
From: Giridhar P. <gi...@lm...> - 2005-02-23 09:19:59
|
I can't put in words how I hate that I don't have amd64 box to test all this. I have wasted most of my last two days over this non-issue. It turns out that all the "fixes" are wrong, and the problem in fact had been fixed sometime after 1.0. 1.1rc1 certainly had proper fix for memory leak - if only people tried that to check for memory leak, it would've saved lot of trouble. The current CVS should *again* work for amd64 without memory leak. If someone can try this and confirm this or provide feedback about what is going wrong, in case of problem, we can proceed to 1.1rc3. If you want to understand what the fix is, see CVS log. -- Giri |
From: Jonathan B. <be...@gm...> - 2005-02-23 19:24:43
|
On Wed, 23 Feb 2005 04:20:22 -0500, Giridhar Pemmasani <gi...@lm...> wrote: > I can't put in words how I hate that I don't have amd64 box to test > all this. I have wasted most of my last two days over this > non-issue. It turns out that all the "fixes" are wrong, and the > problem in fact had been fixed sometime after 1.0. 1.1rc1 certainly > had proper fix for memory leak - if only people tried that to check > for memory leak, it would've saved lot of trouble. > > The current CVS should *again* work for amd64 without memory leak. If > someone can try this and confirm this or provide feedback about what > is going wrong, in case of problem, we can proceed to 1.1rc3. > > If you want to understand what the fix is, see CVS log. > > -- > Giri Well, CVS checked out about 10:30 am CST still does not work for me. Should we sue CVS now instead of the nightlies? I got netconsole setup and was finally able to capture an Oops for you. See attached ndis.oops.txt.gz. Also, I compiled with DEBUG=3. ndis.messages.txt.gz has the excerpt from /var/log/messages. I ran the following commands to get this: modprobe ndiswrapper # (this is where the log starts) iwconfig wlan0 key open `cat key.txt` # setup WEP, now I can see my AP ifconfig wlan0 up # all's well here dhclient wlan0 As you might see in the log, the oops occurs just after getting the DHCP IP address. The log ends with what was logged to file before the oops shut everything down, which I guess may or may not be everything, I don't know. Also attached is the buginfo. Let me know if there is any other info you need. Jonathan |
From: Adrian Irving-B. <wis...@wi...> - 2005-02-23 21:37:16
|
On Wed, Feb 23, 2005 at 01:24:32PM -0600, Jonathan Berry wrote: > Well, CVS checked out about 10:30 am CST still does not work for me. > Should we sue CVS now instead of the nightlies? I seem to recall it being mentioned before on the list that Sourceforce public-access CVS is generally delayed by upwards of 24 hours or more. Hence, nightlies tend to be the preferred way of distributing code. They also can happen more often than just nightly, as in a 'here, try this' response to someone's problem. |
From: Jonathan B. <be...@gm...> - 2005-02-23 22:20:56
|
On Wed, 23 Feb 2005 16:36:51 -0500, Adrian Irving-Beer <wis...@wi...> wrote: > On Wed, Feb 23, 2005 at 01:24:32PM -0600, Jonathan Berry wrote: > > > Well, CVS checked out about 10:30 am CST still does not work for me. > > Should we use CVS now instead of the nightlies? > > I seem to recall it being mentioned before on the list that > Sourceforce public-access CVS is generally delayed by upwards of 24 > hours or more. Hence, nightlies tend to be the preferred way of > distributing code. Yes, it was (I was a part of the discussion then) and it was to this that I referred when I asked my question. But in this thread, Giri said to use latest CVS. I wasn't sure whether this was just out of habit, or whether the he knew that anon-cvs was up-to-date, or what. > They also can happen more often than just nightly, as in a 'here, try > this' response to someone's problem. That's cool. I have noticed some things that suggested it might work that way. Perhaps this is what Giri actually meant. I wasn't sure and wanted to find out, hence I asked the question ;). Jonathan |
From: Adrian Irving-B. <wis...@wi...> - 2005-02-23 22:36:25
|
On Wed, Feb 23, 2005 at 04:20:47PM -0600, Jonathan Berry wrote: > Yes, it was (I was a part of the discussion then) and it was to this > that I referred when I asked my question. But in this thread, Giri > said to use latest CVS. I wasn't sure whether this was just out of > habit, or whether the he knew that anon-cvs was up-to-date, or what. Ah, my mistake. Never mind. :) |
From: Karl V. <kar...@se...> - 2005-02-24 15:15:12
|
Jonathan Berry <be...@gm...> wrote in news:876...@ma...: > Well, CVS checked out about 10:30 am CST still does not work for me. > Should we sue CVS now instead of the nightlies? I got netconsole > setup and was finally able to capture an Oops for you. See attached > ndis.oops.txt.gz. Also, I compiled with DEBUG=3. > ndis.messages.txt.gz has the excerpt from /var/log/messages. I ran > the following commands to get this: > modprobe ndiswrapper # (this is where the log starts) > iwconfig wlan0 key open `cat key.txt` # setup WEP, now I can see my AP > ifconfig wlan0 up # all's well here > dhclient wlan0 > As you might see in the log, the oops occurs just after getting the > DHCP IP address. The log ends with what was logged to file before the > oops shut everything down, which I guess may or may not be everything, > I don't know. > Also attached is the buginfo. Let me know if there is any other info > you need. > > Jonathan Jonathan, I did my testing of 1.1rc2 using kernel 2.6.11-rc3-mm2. I've put up my kernel .config file at: http://users.telenet.be/kvogel/config-rc3-mm2-x86_64.txt The driver I'm using: driver netbc564 (,10/01/2002,3.70.17.5) is the same as the one you are using. Maybe you can try to build the same kernel and try again. NOTE: I tested with WPA-PSK using a static IP address. Karl. |
From: Jonathan B. <be...@gm...> - 2005-02-26 06:34:03
|
On Thu, 24 Feb 2005 15:04:38 +0000 (UTC), Karl Vogel <kar...@se...> wrote: > Jonathan Berry <be...@gm...> wrote in > news:876...@ma...: > > Well, CVS checked out about 10:30 am CST still does not work for me. > > Should we sue CVS now instead of the nightlies? I got netconsole > > setup and was finally able to capture an Oops for you. See attached > > ndis.oops.txt.gz. Also, I compiled with DEBUG=3. > > ndis.messages.txt.gz has the excerpt from /var/log/messages. I ran > > the following commands to get this: > > modprobe ndiswrapper # (this is where the log starts) > > iwconfig wlan0 key open `cat key.txt` # setup WEP, now I can see my AP > > ifconfig wlan0 up # all's well here > > dhclient wlan0 > > As you might see in the log, the oops occurs just after getting the > > DHCP IP address. The log ends with what was logged to file before the > > oops shut everything down, which I guess may or may not be everything, > > I don't know. > > Also attached is the buginfo. Let me know if there is any other info > > you need. > > > > Jonathan > > Jonathan, > > I did my testing of 1.1rc2 using kernel 2.6.11-rc3-mm2. I've put up > my kernel .config file at: > > http://users.telenet.be/kvogel/config-rc3-mm2-x86_64.txt > > The driver I'm using: > driver netbc564 (,10/01/2002,3.70.17.5) > > is the same as the one you are using. > > Maybe you can try to build the same kernel and try again. > > NOTE: I tested with WPA-PSK using a static IP address. > > Karl. Hi Karl, I'm using the latest stock kernel from FC3, which is a 2.6.10 series kernel. I glanced at your config a little and I won't be able to use it directly. This is a laptop and some things like PCMCIA and frequency scaling are missing that I need. Is there an easy way to do a diff between two config files? Simply using diff doesn't seem helpful, the files must be in a different order. I know FC3 has CONFIG_DEBUG_SPINLOCK set and that has given me problems in the past. But 1.0 works just fine, and the last time I tried a post 1.0 version with a kernel without the SPINLOCK stuff, it still locked. I'm not sure about using a vanilla kernel, as I'm not sure what FC3 has added that it wouldn't have. I'm not using SELinux at least. I may try a custom stock kernel and see what happens, and I might see what I can do with a vanilla kernel. I'm using WEP and DHCP, so perhaps that could be a difference. I only get a crash after I DHCP. I can try setting up a static IP, but I'm not sure how to get things working. Whatever dhclient does works things out. I guess I'll have to look more into that stuff. Jonathan |
From: <ker...@co...> - 2005-02-23 14:19:19
|
> Anyone have any tips for using netconsole? I'd really like to get > that setup so that I can grab these Oopses. I think I have my laptop > setup right, I just cannot figure out how to see stuff on the remote > machine. I can't seem to find good documentation. > > > -- > > Giri > > Thanks, > Jonathan Bruce Perens has a good article on Technocrat.net on using netconsole - http://technocrat.net/article.pl?sid=04/08/14/0236245&mode=thread Parag |
From: <ker...@co...> - 2005-02-23 14:36:52
|
I browsed the current CVS. The fix may work but it is horrible performance wise - With this fix the BCM driver will end up doing bazillion kmalloc(, GFP_ATOMIC) and kfree()s. Imagine how it will screw up the VM for no good reason. I have been stating this but here it goes again - NdisAllocateBuffer does _not_ allocate memory - it just establishes the kernel mappings for user allocated memory area. This means driver allocates memory once and asks for NdisAllocateBuffer to establish the mappings multiple times which isn't anywhere near as bad as having to do a kmalloc(). Your earlier fix was more correct one. Thanks Parag > > I can't put in words how I hate that I don't have amd64 box to test > all this. I have wasted most of my last two days over this > non-issue. It turns out that all the "fixes" are wrong, and the > problem in fact had been fixed sometime after 1.0. 1.1rc1 certainly > had proper fix for memory leak - if only people tried that to check > for memory leak, it would've saved lot of trouble. > > The current CVS should *again* work for amd64 without memory leak. If > someone can try this and confirm this or provide feedback about what > is going wrong, in case of problem, we can proceed to 1.1rc3. > > If you want to understand what the fix is, see CVS log. > > -- > Giri |
From: Giridhar P. <gi...@lm...> - 2005-02-25 21:15:18
|
Parag> The fix may work but it is horrible performance wise - With this fix Parag> the BCM driver will end up doing bazillion kmalloc(, GFP_ATOMIC) and Parag> kfree()s. Why bazillion? For every packet we allocate a buffer. That is how Windows would probably do - somewhere NDIS says that the pool can be NULL. And NdisFreeBuffer doesn't pass pool, so you have to allocate/free each buffer. Parag> Imagine how it will screw up the VM for no good reason. How? What is your concern? Parag> I have been stating this but here it goes again - NdisAllocateBuffer Parag> does _not_ allocate memory - it just establishes the kernel mappings Parag> for user allocated memory area. This means driver allocates memory Parag> once and asks for NdisAllocateBuffer to establish the mappings Parag> multiple times which isn't anywhere near as bad as having to do a Parag> kmalloc(). See above. Parag> Your earlier fix was more correct one. Earlier one was traversing the pool to check for duplicate allocation. In some drivers, the number of such checks can be 256 (32-bit Broadcom driver allocates 256 buffers). Going through 256 pointers may be less efficient than a kmalloc. Moreover, some drivers allocate many buffers, but use only a couple. Having said all that, let me add that I have an alternate implementation that actually uses pools, allocates a buffer only once and allocates only when necessary. It also uses the same approach for packets. I have done very little testing of this code, as my laptop's power adapter is broken. If things work well with this patch (I will commit it in the next few hours), 1.1 will be released over the weekend. -- Giri |
From: Parag W. <ker...@co...> - 2005-02-25 23:04:04
|
On Friday 25 February 2005 04:15 pm, Giridhar Pemmasani wrote: > Why bazillion? For every packet we allocate a buffer. That is how Windows > would probably do - somewhere NDIS says that the pool can be NULL. And > NdisFreeBuffer doesn't pass pool, so you have to allocate/free each buffer. I have instrumented this - BCM Driver (atleast on AMD64) does to the tune of 40000 calls in under a minute to NdisAllocateBuffer. Traversing pointers vs. kmalloc speed is debatable but consider the fragmentation that happens after 40000*60*2 kmalloc and kfrees. At least I haven't seen anything real that does so many kmallocs and kfrees. Also see this thread at MSDN - http://msdn.microsoft.com/newsgroups/default.aspx?dg=microsoft.public.development.device.drivers&tid=2e8cf103-acd9-494a-bc81-f02a579e7f13&cat=en-us-msdn-windev-winddk&lang=en&cr=US&sloc=en-us&m=1&p=1 Parag |
From: Giridhar P. <gi...@lm...> - 2005-02-25 23:55:19
|
The information there is misleading. One has to allocate space for MDL so you can initialize its members so MDL can describe a preallocated memory (what is passed in 'virt'). In Windows 9x/ME, you don't need to allocate space (since 'pool' is used there), but in 2000/XP, pool is not used. If no space for MDL is allocated anywhere, how can you initialize its members? -- Giri |
From: Parag W. <ker...@co...> - 2005-02-26 00:27:50
|
On Friday 25 February 2005 06:55 pm, Giridhar Pemmasani wrote: > In Windows 9x/ME, you don't need to allocate space (since > 'pool' is used there), but in 2000/XP, pool is not used. If no space for > MDL is allocated anywhere, how can you initialize its members? You have to keep this in mind - On Windows XP, IoAllocateMDL has a "look-aside" pool of MDLs. So it isn't newly allocated. Driver can call it as many times as needed (MSDN) without taking a performance hit precisely because of the pooling behavior. In NDISWrapper case, I am not saying it must not be allocated - I am saying must be allocated exactly once per VirtAddr (VA). The BCM driver, calls NdisAllocateBuffer with only handful of distinct VAs. The expected behavior is not to allocate memory for each VA - instead once per VA. What your patch does is for *each* call to NdisAllocateBuffer, it ends up doing a kmalloc (and for each IoFreeMDL a kfree) which is superfluous. I am running my laptop with my second patch which does kmalloc in NdisAllocateBuffer - but only once for each VA. It returns the same, previously allocated buffer if caller calls it with a previously passed in VA. This, so far has worked absolutely fine under heavy use. The driver manages free/busy for the buffers, so just re-instate your earlier pool patch and we will be all set. (Preferably take the route of my patch - allocate on demand - Instead of unnecessarily pooling and walking over 256 buffers, create one for each distinct VA as required.) Parag |
From: Giridhar P. <gi...@lm...> - 2005-02-26 03:20:47
|
On Fri, 25 Feb 2005 19:27:40 -0500, Parag Warudkar <ker...@co...> said: Parag> In NDISWrapper case, I am not saying it must not be allocated - I am Parag> saying must be allocated exactly once per VirtAddr (VA). That is not correct. MDL doesn't have to be allocated once for a virtual address. Let me explain. MDL describes virtual address mapping (to pages). Strictly, MDL is not necessary in Linux, since all virtual addresses are in kernel space. But because of Windows, drivers want to allocate buffer. You don't need to allocate once per VA. That is bogus and that is why I reverted that patch. You map one MDL to one VA. How you get MDL is upto you. This is what is missing in the URL that you pointed earlier. Parag> my second patch which does kmalloc in NdisAllocateBuffer - but only Parag> once for each VA. It returns the same, previously allocated buffer if Parag> caller calls it with a previously passed in VA. This, so far has Parag> worked absolutely fine under heavy use. This is not correct either. Parag> The driver manages free/busy for the buffers, so just re-instate your Parag> earlier pool patch and we will be all set. (Preferably take the route Parag> of my patch - allocate on demand - Instead of unnecessarily pooling It may work, but neither of them is either efficient or correct. And please don't ask me to take what patch to commit - if I am convinced a patch is good, I will commit it even if not requested. If patch is not good but the idea is correct, I will adjust the patch as required. As I said before, I have already reworked all this buffers/packets so it not only works (well, should work anyway, since I haven't been able to test with all drivers that I usually test, as my laptop's power adapter is broken) but is "more correct" (there are some intricate details that we can safely ignore) and efficient. I will commit this code later today/tomorrow. -- Giri |
From: Parag W. <ker...@co...> - 2005-02-26 03:45:37
|
On Friday 25 February 2005 10:20 pm, Giridhar Pemmasani wrote: > r. You don't need to allocate once per VA. That is bogus and that is why I reverted that patch. You map one MDL to one VA. How you get MDL is upto you. Giri, What _are_ you arguing about? Aren't above two statements contradictory? "You don't need to allocate once per VA" VS. "You map one MDL to one VA." I am saying the same thing - Have one MDL allocated for one VA and return that everytime for that same VA . I think this is getting obfuscated to death for no good reason. The whole point is about not doing NdisAllocateBuffer => kmalloc and NdisFreeBuffer=>kfree which the current CVS does. As I said this results into extreme number of calls to kmalloc and kfree for no reason. Windows doesn't do that, (IoAllocateMdl uses pools, that's exactly why the BCM driver can afford to do so many number of calls to NdisAllocateBuffer - If it was a memory allocation per call, I am sure no one will use Broadcom's products.) so we should not do it either. > It may work, but neither of them is either efficient or correct. And please > don't ask me to take what patch to commit - if I am convinced a patch is > good, I will commit it even if not requested. If patch is not good but the > idea is correct, I will adjust the patch as required. That was frustrating - I am not marketing my patch, I don't care a bit if you apply or not - really. I was suggesting correction to the current code which I feel is technically correct - No damn thing beyond that. Anyway - Let's hope your future commit (Again I am not *asking* for a commit ;) fixes it the right way and everyone will be happy. Parag |
From: Giridhar P. <gi...@lm...> - 2005-02-26 05:46:09
|
On Fri, 25 Feb 2005 22:45:26 -0500, Parag Warudkar <ker...@co...> said: Parag> What _are_ you arguing about? Aren't above two statements Parag> contradictory? "You don't need to allocate once per VA" VS. "You map Parag> one MDL to one VA." I am saying the same thing - Have one MDL Parag> allocated for one VA and return that everytime for that same VA . Those _are_ two different things. I can't explain any more clearly. Parag> point is about not doing NdisAllocateBuffer => kmalloc and Parag> NdisFreeBuffer=>kfree which the current CVS does. As I said this Parag> results into If your concern is that allocating/freeing too many times, if it is not necessary, I agree with that. But dragging it without understanding what is correct and what is not is hardly useful. Just because it works for you doesn't mean it is the right way. Parag> That was frustrating - I am not marketing my patch, I don't care a Parag> bit if you apply or not - really. I was suggesting correction to the Parag> current code which I feel is technically correct - No damn thing Parag> beyond that. Don't push it. Parag> Anyway - Let's hope your future commit (Again I am not *asking* for a Parag> commit ;) fixes it the right way and everyone will be happy. I couldn't care less about _your_ feeling or whether _you_ will be happy or not. I commit what is right. I want things to work for everyone (or at least as many as possible). If you think that was frustrating, you should know what it is that I spent whole two days trying to understand your wrong explanation (that a buffer is not being freed, which 1.1rc1 was doing) and patch (which ignored the case that buffer is getting freed in IoFreeMdl and not in NdisFreeBuffer). Only after spending time with Karl Vogel on IRC that I realized what is going on with 64-bit driver. In the process it broke other drivers and resulted in broken 1.1rc2. I don't have patience or interest in explaining any further. -- Giri |
From: Jan K. <jan...@we...> - 2005-02-26 09:44:11
Attachments:
smime.p7s
|
Hi, I haven't followed your intensive discussion about the way NdisAllocateBuffer[Pool], NdisFreeBuffer etc. work in details, so I may repeat some already known facts now. However, I would like to share a bit of my experiences collected during the development of IrCOMM2k (i.e. the Linux-IrDA port to Windows). First, NdisAllocateBufferPool is actually a dummy function. I even tracked this once down to the real Windows kernel code while chasing some bug in IrCOMM2k. The is no pool preallocated. Second, a NDIS_BUFFER - that's what NdisAllocateBuffer returns - is nothing else than an MDL, I guess it translates "memory descriptor list". MDLs are not allocated from pools, see IoAllocateMdl. But these MDL are just small structures (see DDK headers) which are used to link given memory buffer fragments to a chain of buffers. So, what IoAllocateMdl does is to link the given virtual address to the MDL while just allocating the MDL itself. The same should NdisAllocateBuffer likely do (as NDIS is just a wrapper on the kernel services). I guess a kmalloc or a kmem_cache_alloc from a prepared kmem_cache should be ok for ndiswrapper as well. And third, the driver doesn't need to call any NdisFreeBuffer if it chained the buffer to an NDIS_PACKET. On release of the packet, also the chained buffer fragments described by NDIS_BUFFERs have to be released by the NDIS subsystem. That's why the driver calls only NdisAllocateBuffer. Ah, and NdisChainBufferAtFront/Back are just #defines, take a look at the ndis.h header that come with the DDK (Giri should be able to do so ;). Jan |
From: Giridhar P. <gi...@lm...> - 2005-02-26 14:58:13
|
Jan, This is for documentation, as I bent NDIS a bit in implementing it in the current CVS. I know that NdisAllocateBufferPool may not do anything in 2000/XP. However, since MDL/ndis_buffer is opaque, there is nothing in we treating all this differently. At least for now (as CVS log explained), I don't see anything wrong with that. There may be other ways of implementing NdisAllocate(Free)Buffer, and if we have better ideas, we can certainly implement it. BTW, my comment in CVS log about problems with a driver that cares about mdl->size may not correct - since nowhere in current code (unless I missed it) we access mdl->size, so even if we allocate an MDL that doesn't describe a memory region, it should be safe. If in future we need to return mdl->size to the driver, we should remember that mdl->size may be wrong, so instead, we should calculate the size (with MmSizeOfMdl), so driver gets the correct size. Actual values in there don't matter anyway, since we don't lock pages in Linux (as explained in CVS log). -- Giri |
From: Jan K. <jan...@we...> - 2005-02-26 17:41:49
Attachments:
smime.p7s
|
Giridhar Pemmasani wrote: > Jan, > > This is for documentation, as I bent NDIS a bit in implementing it in > the current CVS. > > I know that NdisAllocateBufferPool may not do anything in > 2000/XP. However, since MDL/ndis_buffer is opaque, there is nothing in > we treating all this differently. At least for now (as CVS log > explained), I don't see anything wrong with that. There may be other > ways of implementing NdisAllocate(Free)Buffer, and if we have better > ideas, we can certainly implement it. BTW, my comment in CVS log about > problems with a driver that cares about mdl->size may not correct - > since nowhere in current code (unless I missed it) we access > mdl->size, so even if we allocate an MDL that doesn't describe a > memory region, it should be safe. If in future we need to return > mdl->size to the driver, we should remember that mdl->size may be > wrong, so instead, we should calculate the size (with MmSizeOfMdl), so > driver gets the correct size. Actual values in there don't matter > anyway, since we don't lock pages in Linux (as explained in CVS log). > I think the content of that page array behind the MDL head might be actually transparent to the driver, but I found at least one driver example in the DDK which accesses the array directly (src/kernel/agp/agplib/intrface.c). The purpose: two MDL are being merged into a single new one. So the driver copies both arrays together into the freshly allocated jumbo MDL. As it is an "official" example, this shows that published data structures of the DDK ought to be respected to avoid rare but likely hard to chase problems with it. Considering the content of the page array again: the described pages are non-paged, i.g. physically present under Windows. This may let the Windows driver try to use the page addresses in the array for handing them over to the hardware for DMA purposes. But this is theoretical as there are other functions for it. :) And regarding the pooled approach for NDIS_BUFFERs: I don't expect a significant performance gain. Take a look at the kmalloc implementation - there are already pools for differently sized blocks. The only optimisation would be to use kmem_cache_xxx for allocating pools of custom-sized blocks (e.g. for NDIS_PACKET). Jan |
From: Giridhar P. <gi...@lm...> - 2005-02-26 18:19:52
|
On Sat, 26 Feb 2005 18:40:42 +0100, Jan Kiszka <jan...@we...> said: Jan> And regarding the pooled approach for NDIS_BUFFERs: I don't expect a Jan> significant performance gain. Take a look at the kmalloc implementation Jan> - there are already pools for differently sized blocks. The only Jan> optimisation would be to use kmem_cache_xxx for allocating pools of Jan> custom-sized blocks (e.g. for NDIS_PACKET). The concern is not a performance gain as much as fragmentation. As I indicated privately to you, I have already created a patch that uses kmem_cache in my tree. Considering that there are some critical issues in 1.0, I would rather release 1.1 with the current 1.1rc3 rather than go through another rc. I use kmem_cache only for NDIS_BUFFER/MDL. As I understand, NDIS_PACKETs shouldn't have problems with pool, although it is trivial to patch packet allocation/free to use kmem_cache. -- Giri |
From: Jan K. <jan...@we...> - 2005-02-26 17:24:18
Attachments:
smime.p7s
|
Jan Kiszka wrote: > And third, the driver doesn't need to call any NdisFreeBuffer if it > chained the buffer to an NDIS_PACKET. On release of the packet, also the > chained buffer fragments described by NDIS_BUFFERs have to be released > by the NDIS subsystem. That's why the driver calls only > NdisAllocateBuffer. Ah, and NdisChainBufferAtFront/Back are just Bullshit. I oversaw the related NdisFreeBuffer in my own code (hidden in some inline function). Releasing buffers is still up to the driver which allocated it - after unchaining it from the packet. Jan |
From: <ker...@co...> - 2005-02-26 14:04:34
|
>trying to understand your wrong explanation (that a buffer is >not being freed, which 1.1rc1 was doing) and patch (which ignored the case >that buffer is getting freed in IoFreeMdl and not in NdisFreeBuffer). My explanation was it was _not_ getting kfree'd - from linux kernel PoV the memory which was being allocated using kmalloc wasn't kfree'd. That was _NOT_ wrong, else we would not have exchanged a single mail about this. My patch removed the need for unconditionally kmalloc'ing each time which was correct in two senses - stop the leak and don't unnecessarily allocate. Again go back to the archive - the discussion was for 1.0 version of ndiswrapper. You can't blame people on what stunts you did for -rc. After you realized that (for some reason) it was leaking, the only point of disagreement was whether or not to do pool the memory required for MDL. Now that you agree that is should be, cat THIS_THREAD > /dev/null Parag > On Fri, 25 Feb 2005 22:45:26 -0500, Parag Warudkar <ker...@co...> > said: > > Parag> What _are_ you arguing about? Aren't above two statements > Parag> contradictory? "You don't need to allocate once per VA" VS. "You map > Parag> one MDL to one VA." I am saying the same thing - Have one MDL > Parag> allocated for one VA and return that everytime for that same VA . > > Those _are_ two different things. I can't explain any more clearly. > > Parag> point is about not doing NdisAllocateBuffer => kmalloc and > Parag> NdisFreeBuffer=>kfree which the current CVS does. As I said this > Parag> results into > > If your concern is that allocating/freeing too many times, if it is not > necessary, I agree with that. But dragging it without understanding what is > correct and what is not is hardly useful. Just because it works for you > doesn't mean it is the right way. > > Parag> That was frustrating - I am not marketing my patch, I don't care a > Parag> bit if you apply or not - really. I was suggesting correction to the > Parag> current code which I feel is technically correct - No damn thing > Parag> beyond that. > > Don't push it. > > Parag> Anyway - Let's hope your future commit (Again I am not *asking* for a > Parag> commit ;) fixes it the right way and everyone will be happy. > > I couldn't care less about _your_ feeling or whether _you_ will be happy or > not. I commit what is right. I want things to work for everyone (or at least > as many as possible). > > If you think that was frustrating, you should know what it is that I spent > whole two days trying to understand your wrong explanation (that a buffer is > not being freed, which 1.1rc1 was doing) and patch (which ignored the case > that buffer is getting freed in IoFreeMdl and not in NdisFreeBuffer). Only > after spending time with Karl Vogel on IRC that I realized what is going on > with 64-bit driver. In the process it broke other drivers and resulted in > broken 1.1rc2. > > I don't have patience or interest in explaining any further. > > -- > Giri |
From: Giridhar P. <gi...@lm...> - 2005-02-26 15:59:56
|
On Sat, 26 Feb 2005 14:04:25 +0000, ker...@co... (Parag Warudkar) said: Parag> not have exchanged a single mail about this. My patch Parag> removed the need for unconditionally kmalloc'ing each time Parag> which was correct in two senses - stop the leak and don't Parag> unnecessarily allocate. Try your patch (without any changes) with CONFIG_DEBUG_SLAB enabled. Parag> version of ndiswrapper. You can't blame people on what Parag> stunts you did for -rc. The stunt was trying to implement your broken explanation. Parag> After you realized that (for some reason) it was leaking, Again, it was not leaking at least in 1.1rc1. You submitted a patch for old version of ndiswrapper. Although you mentioned that it is for 1.0, your explanation of what was going on was wrong. From the explanation, there was no way I could figure out that 1.1rc1 is doing any differently, and I understood it as a bug in 64-bit driver for which we need to have a workaround. Parag> pool the memory required for MDL. I didn't agree that we should use pool. It is just that my current implementation uses pool. There are problems with this approach and there are alternate ways of doing so (e.g., use kmem_cache to avoid fragmentation). If I see any problems with current implementation, I will replace current implementation with kmem_cache implementation. Parag> Now that you agree thatis should be, cat THIS_THREAD > Parag> /dev/null Why? If you didn't want me to read it, you shouldn't have sent this mail. If you didn't want others to read it, you could've not copied to ndiswrapper mailing list. You can't ramble and ask people to ignore. -- Giri |