Re: [patch 1/7] net_device list cleanup: core

2006-07-10 Thread Andrey Savochkin 
On Sat, Jul 08, 2006 at 01:48:13AM +0900, YOSHIFUJI Hideaki / [EMAIL 
PROTECTED](B wrote:
 In article [EMAIL PROTECTED] (at Fri, 7 Jul 2006 11:54:25 +0400), Andrey 
 Savochkin [EMAIL PROTECTED] says:
 
  On Fri, Jul 07, 2006 at 01:34:34PM +0900, YOSHIFUJI Hideaki / [EMAIL 
  PROTECTED](B wrote:
   In article [EMAIL PROTECTED] (at Mon, 3 Jul 2006 12:18:51 +0400), 
   Andrey Savochkin [EMAIL PROTECTED] says:
   
@@ -3271,22 +3277,22 @@ int unregister_netdevice(struct net_devi
 
/* And unlink it from device chain. */
for (dp = dev_base; (d = *dp) != NULL; dp = d-next) {
   
   Why not for_each_netdev?
  
  it's a different list
 
 Sorry, I still do not understand.
 In other words, why will we still have dev-next?
 After introducing net_device-dev_list, we do not need
 dev-next anymore, do we?

dev-next is removed in the last patch, to make possible the bisection
of patch list.
-
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

Re: [patch 1/7] net_device list cleanup: core

2006-07-07 Thread Andrey Savochkin 
On Fri, Jul 07, 2006 at 01:34:34PM +0900, YOSHIFUJI Hideaki / [EMAIL 
PROTECTED](B wrote:
 In article [EMAIL PROTECTED] (at Mon, 3 Jul 2006 12:18:51 +0400), Andrey 
 Savochkin [EMAIL PROTECTED] says:
 
  @@ -3271,22 +3277,22 @@ int unregister_netdevice(struct net_devi
   
  /* And unlink it from device chain. */
  for (dp = dev_base; (d = *dp) != NULL; dp = d-next) {
 
 Why not for_each_netdev?

it's a different list
-
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

Re: [patch 1/7] net_device list cleanup: core

2006-07-07 Thread YOSHIFUJI Hideaki / 吉藤英明 
In article [EMAIL PROTECTED] (at Fri, 7 Jul 2006 11:54:25 +0400), Andrey 
Savochkin [EMAIL PROTECTED] says:

 On Fri, Jul 07, 2006 at 01:34:34PM +0900, YOSHIFUJI Hideaki / [EMAIL 
 PROTECTED](B wrote:
  In article [EMAIL PROTECTED] (at Mon, 3 Jul 2006 12:18:51 +0400), Andrey 
  Savochkin [EMAIL PROTECTED] says:
  
   @@ -3271,22 +3277,22 @@ int unregister_netdevice(struct net_devi

 /* And unlink it from device chain. */
 for (dp = dev_base; (d = *dp) != NULL; dp = d-next) {
  
  Why not for_each_netdev?
 
 it's a different list

Sorry, I still do not understand.
In other words, why will we still have dev-next?
After introducing net_device-dev_list, we do not need
dev-next anymore, do we?

--yoshfuji

-
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

Re: [patch 1/7] net_device list cleanup: core

2006-07-06 Thread YOSHIFUJI Hideaki / 吉藤英明 
In article [EMAIL PROTECTED] (at Mon, 3 Jul 2006 12:18:51 +0400), Andrey 
Savochkin [EMAIL PROTECTED] says:

 @@ -3271,22 +3277,22 @@ int unregister_netdevice(struct net_devi
  
   /* And unlink it from device chain. */
   for (dp = dev_base; (d = *dp) != NULL; dp = d-next) {

Why not for_each_netdev?

--yoshfuji
-
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

Re: [patch 1/7] net_device list cleanup: core

2006-07-05 Thread Andrey Savochkin 
On Tue, Jul 04, 2006 at 08:35:37PM +0400, A.N.Kuznetsov wrote:
 
  Different modules want different kinds of lookup.
  So, I'm thinking about something like ilookup5.
 
  The next question: would people agree to review a patch doing this for
  net_devices? :)
 
 One not original suggestion, which did not sound nevertheless:
 to implement netdev_iterate_list() or whatever, update only core
 and a few of devices and deprecate dev_base_head
 with __deprecated_for_modules adding it to
 Documentation/feature-removal-schedule.txt

I like this idea

Andrey
-
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

Re: [patch 1/7] net_device list cleanup: core

2006-07-04 Thread Andrey Savochkin 
Christoph,

On Mon, Jul 03, 2006 at 06:46:50PM +0100, Christoph Hellwig wrote:
 On Mon, Jul 03, 2006 at 12:18:51PM +0400, Andrey Savochkin wrote:
  Cleanup of net_device list use in net_dev core and IP.
  The cleanup consists of
   - converting the to list_head, to make the list double-linked (thus making
 remove operation O(1)), and list walks more readable;
   - introducing of for_each_netdev wrapper over list_for_each.
 
 When you change all this please make sure dev_base_head is never directly
 accessed anymore, not even through macros and dev_base_head is not exported
 anymore.  That's the only way to keep drivers messing with it.
 
 Yes, it's a little more work as you need to audit all drivers to see what
 they are doing and find suitable abstractions but it's a must have that
 should have been done a lot earlier.

Hiding dev_base_head can be done by converting first_netdev/next_netdev into
functions and implementing for_each_netdev loop through them.

Or are you talking about abstractions like functions
for_each_netdev/find_netdev with callbacks?
Do you think that hiding the list internals is worth the additional
complexity and substantial increase of the patch size?

Andrey
-
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

Re: [patch 1/7] net_device list cleanup: core

2006-07-04 Thread Christoph Hellwig 
On Tue, Jul 04, 2006 at 11:24:05AM +0400, Andrey Savochkin wrote:
  Yes, it's a little more work as you need to audit all drivers to see what
  they are doing and find suitable abstractions but it's a must have that
  should have been done a lot earlier.
 
 Hiding dev_base_head can be done by converting first_netdev/next_netdev into
 functions and implementing for_each_netdev loop through them.
 
 Or are you talking about abstractions like functions
 for_each_netdev/find_netdev with callbacks?

an for_each_netdev with a callback makes sense and gives a cleaner
abstraction, yes.  I don't think you should need a callback for the lookup
structure.

 Do you think that hiding the list internals is worth the additional
 complexity and substantial increase of the patch size?

Yes, absolutely.  We've converted scsi hosts and devices from a model
where drivers could directly access the list to strict iterators in the
2.5 series.  It's quite a lot of work as you have to understand what
the drivers actually do (and to at least 50% they were doing something
really stupid) and convert them to the right abstractions.
-
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

Re: [patch 1/7] net_device list cleanup: core

2006-07-04 Thread Andrey Savochkin 
On Tue, Jul 04, 2006 at 10:10:03AM +0100, Christoph Hellwig wrote:
 On Tue, Jul 04, 2006 at 11:24:05AM +0400, Andrey Savochkin wrote:
   Yes, it's a little more work as you need to audit all drivers to see what
   they are doing and find suitable abstractions but it's a must have that
   should have been done a lot earlier.
  
  Hiding dev_base_head can be done by converting first_netdev/next_netdev into
  functions and implementing for_each_netdev loop through them.
  
  Or are you talking about abstractions like functions
  for_each_netdev/find_netdev with callbacks?
 
 an for_each_netdev with a callback makes sense and gives a cleaner
 abstraction, yes.  I don't think you should need a callback for the lookup
 structure.

Different modules want different kinds of lookup.
So, I'm thinking about something like ilookup5.

 
  Do you think that hiding the list internals is worth the additional
  complexity and substantial increase of the patch size?
 
 Yes, absolutely.  We've converted scsi hosts and devices from a model
 where drivers could directly access the list to strict iterators in the
 2.5 series.  It's quite a lot of work as you have to understand what
 the drivers actually do (and to at least 50% they were doing something
 really stupid) and convert them to the right abstractions.

The next question: would people agree to review a patch doing this for
net_devices? :)

Andrey
-
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

Re: [patch 1/7] net_device list cleanup: core

2006-07-04 Thread Alexey Kuznetsov 
Hello!

 Different modules want different kinds of lookup.
 So, I'm thinking about something like ilookup5.

 The next question: would people agree to review a patch doing this for
 net_devices? :)

One not original suggestion, which did not sound nevertheless:
to implement netdev_iterate_list() or whatever, update only core
and a few of devices and deprecate dev_base_head
with __deprecated_for_modules adding it to
Documentation/feature-removal-schedule.txt

Alexey
-
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

[patch 1/7] net_device list cleanup: core

2006-07-03 Thread Andrey Savochkin 
Cleanup of net_device list use in net_dev core and IP.
The cleanup consists of
 - converting the to list_head, to make the list double-linked (thus making
   remove operation O(1)), and list walks more readable;
 - introducing of for_each_netdev wrapper over list_for_each.

Signed-off-by: Andrey Savochkin [EMAIL PROTECTED]
Signed-off-by: Kirill Korotaev [EMAIL PROTECTED]
---
 include/linux/netdevice.h |   29 ++-
 net/core/dev.c|   48 +-
 net/ipv4/devinet.c|6 ++---
 net/ipv6/addrconf.c   |8 +++
 net/ipv6/anycast.c|   10 +
 5 files changed, 68 insertions(+), 33 deletions(-)

--- ./include/linux/netdevice.h.vedevbase-core  Mon Jul  3 15:14:15 2006
+++ ./include/linux/netdevice.h Mon Jul  3 16:09:11 2006
@@ -290,7 +290,8 @@ struct net_device
unsigned long   state;
 
struct net_device   *next;
-   
+   struct list_headdev_list;
+
/* The device initialization function. Called only once. */
int (*init)(struct net_device *dev);
 
@@ -558,8 +559,34 @@ struct packet_type {
 
 extern struct net_device   loopback_dev;   /* The loopback 
*/
 extern struct net_device   *dev_base;  /* All devices 
*/
+extern struct list_headdev_base_head;  /* All 
devices */
 extern rwlock_tdev_base_lock;  /* 
Device list lock */
 
+#define for_each_netdev(p) list_for_each_entry(p, dev_base_head, dev_list)
+
+/*
+ * When possible, it is preferrable to use for_each_netdev() loop
+ * defined above, rather than first_netdev()/next_netdev() macros.
+ * for_each_netdev() loop makes the intentions clearer, and gives more
+ * flexibility in device list implementation.
+ * While next_netdev() is unavoidable in seq_proc functions,
+ * first_netdev() should be needed quite rarely.
+ */
+#define first_netdev() ({ \
+   list_empty(dev_base_head) ? NULL : \
+   list_entry(dev_base_head.next, \
+   struct net_device, \
+   dev_list); \
+})
+#define next_netdev(dev)   ({ \
+   struct list_head *__next; \
+   __next = (dev)-dev_list.next; \
+   __next == dev_base_head ? NULL : \
+   list_entry(__next, \
+   struct net_device, \
+   dev_list); \
+})
+
 extern int netdev_boot_setup_check(struct net_device *dev);
 extern unsigned long   netdev_boot_base(const char *prefix, int unit);
 extern struct net_device*dev_getbyhwaddr(unsigned short type, char 
*hwaddr);
--- ./net/core/dev.c.vedevbase-core Mon Jul  3 15:14:19 2006
+++ ./net/core/dev.cMon Jul  3 16:09:11 2006
@@ -181,6 +181,9 @@ DEFINE_RWLOCK(dev_base_lock);
 EXPORT_SYMBOL(dev_base);
 EXPORT_SYMBOL(dev_base_lock);
 
+LIST_HEAD(dev_base_head);
+EXPORT_SYMBOL(dev_base_head);
+
 #define NETDEV_HASHBITS8
 static struct hlist_head dev_name_head[1NETDEV_HASHBITS];
 static struct hlist_head dev_index_head[1NETDEV_HASHBITS];
@@ -575,11 +578,11 @@ struct net_device *dev_getbyhwaddr(unsig
 
ASSERT_RTNL();
 
-   for (dev = dev_base; dev; dev = dev-next)
+   for_each_netdev(dev)
if (dev-type == type 
!memcmp(dev-dev_addr, ha, dev-addr_len))
-   break;
-   return dev;
+   return dev;
+   return NULL;
 }
 
 EXPORT_SYMBOL(dev_getbyhwaddr);
@@ -589,14 +592,15 @@ struct net_device *dev_getfirstbyhwtype(
struct net_device *dev;
 
rtnl_lock();
-   for (dev = dev_base; dev; dev = dev-next) {
+   for_each_netdev(dev) {
if (dev-type == type) {
dev_hold(dev);
-   break;
+   rtnl_unlock();
+   return dev;
}
}
rtnl_unlock();
-   return dev;
+   return NULL;
 }
 
 EXPORT_SYMBOL(dev_getfirstbyhwtype);
@@ -617,14 +621,15 @@ struct net_device * dev_get_by_flags(uns
struct net_device *dev;
 
read_lock(dev_base_lock);
-   for (dev = dev_base; dev != NULL; dev = dev-next) {
+   for_each_netdev(dev) {
if (((dev-flags ^ if_flags)  mask) == 0) {
dev_hold(dev);
-   break;
+   read_unlock(dev_base_lock);
+   return dev;
}
}
read_unlock(dev_base_lock);
-

Re: [patch 1/7] net_device list cleanup: core

2006-07-03 Thread Christoph Hellwig 
On Mon, Jul 03, 2006 at 12:18:51PM +0400, Andrey Savochkin wrote:
 Cleanup of net_device list use in net_dev core and IP.
 The cleanup consists of
  - converting the to list_head, to make the list double-linked (thus making
remove operation O(1)), and list walks more readable;
  - introducing of for_each_netdev wrapper over list_for_each.

When you change all this please make sure dev_base_head is never directly
accessed anymore, not even through macros and dev_base_head is not exported
anymore.  That's the only way to keep drivers messing with it.

Yes, it's a little more work as you need to audit all drivers to see what
they are doing and find suitable abstractions but it's a must have that
should have been done a lot earlier.

-
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