David S. Miller wrote:
From: Ben Greear <[EMAIL PROTECTED]>
Date: Thu, 01 Sep 2005 23:11:28 -0700
A quick optimization is to kmalloc chunks of 1000 or so structs at once
and then write a cheap foomalloc method to grab and release them. We
already take a lock (at least in my implementation), so this should be
small overhead.
Better to eliminate all the kmalloc()'s altogether. It's bad
for a debugging facility to have a known failure mode.
Maybe it's just too late..but I am not understanding this. How
do you print all of the current users of the device when you are
trying to release the device and something has a reference to it?
The netdev_pointer's get linked in to a head pointer in
the netdev itself.
struct netdev {
...
struct hlist_head debug_ref_list;
...
};
struct netdev_pointer {
...
struct hlist_node debug_ref_node;
...
};
So you walk the debug_ref_list to get all the references
grabbed of the object.
Ok, so each object that now has a net_device* dev pointer instead
gets this netdev_pointer object. When a reference is taken,
this netdev_pointer object is linked into the netdevice object
in a list. This requires a lock and is O(1) even with debugging
disabled?
On dev_put, you remove the reference from the netdev's list.
This would be O(N) and require a lock with or without debugging enabled, right?
(N == number of references)
You are also going to pay the memory price of the hlist_node object at least,
even
with debugging disabled. In things like routes and sockets this might add up...
All of the code that currently goes foo->dev would have to be changed to
foo->dev.dev for reference, and/or we'd change most methods to take a pointer
to netdev_pointer instead of netdevice?
Please correct if I am wrong here.
Perhaps an even better idea for class #2 is to use a stack local
"struct netdev_pointer", since that is the mode in which this
case typically occurs, all within the same function so the function
stack frame is live during the entire dummy refrence grab.
Could you actually detect a reference leak in this case? The struct goes out
of scope when the method ends..but since we don't have auto-destructors
like C++, then there is nothing to force the release of the reference.
Most of the cases in this class are in the scope of the method,
ie. we do the put before the function returns.
Right, but we want to catch the cases where someone forgets, so the
pointer object cannot be on the stack (we need something to reference
X time later when we try to free the device and want to know who has
a handle.) This implies kmalloc to me, since setting any static amount
of them in the netdevice is just asking for trouble (or known failure
mode, to use your term :)
Bad caps I can quickly fix..but I actually went to some trouble to
try to get the tabbing correct. Is my patch coming through with spaces
instead of tabs or something like that?
Yes, spaces instead of tabs:
- slave_dev = dev_get_by_name(srq.slave_name);
+ /* Need this up here so we know what the eventual reference for
+ * the slave_dev will be. This is to help with debugging netdev
+ * ref counts. --Ben
+ */
+ s = kmalloc(sizeof(*s), GFP_KERNEL);
+ if (!s) {
+ return -ENOMEM;
+ }
+
+ slave_dev = dev_get_by_name(srq.slave_name, &s->dev);
Ok, will search for those again.
and the unnecessary bracing there needs to be eliminated as well.
Some of us still read code in 24 line terminals from time to time :)
Bleh, some of us with actual monitors hate when commenting
out a line of code changes the logic flow! But, not my ball game,
so I'll comply.
I went ahead and ported the rest of the kernel to my current method
earlier today. Will update the patch with those changes as well as
the fixed capitilization and lack of spaces in a few minutes.
I do like that your method gets rid of the kmalloc in at least one
case, but to be honest, I still am partial to my method at this point
because it has little to no cost when disabled (and it's done :))
Ben
--
Ben Greear <[EMAIL PROTECTED]>
Candela Technologies Inc http://www.candelatech.com
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
