linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH SET][bonding] cleanup
@ 2003-09-25 12:49 Shmulik Hen
  2003-09-25 16:22 ` Jay Vosburgh
  2003-09-25 16:47 ` [Bonding-announce] " Chad N. Tindel
  0 siblings, 2 replies; 8+ messages in thread
From: Shmulik Hen @ 2003-09-25 12:49 UTC (permalink / raw)
  To: bonding-devel, bonding-announce, netdev, linux-kernel, linux-net
  Cc: Jeff Garzik, Jay Vosburgh, Noam, Amir, Mendelson, Tsippy, Noam,
	Marom, Shmulik, Hen

Hi,

Now that all the 2.4<->2.6 synchronizing stuff is done, we are moving 
forward with the cleanup plan. This set is similar to the previous 
set I sent on 8/27, but it is based on the latest version that was 
accepted into the kernel with the seq_file changes, a few bug fixes 
and a bit more cleanup stuff. This set is very comprehensive and 
touches almost all the code. The set is broken down to many patches 
for better tracking. It was already tested by me for functionality 
and is undergoing a more thorough set of testing by our QA group for 
any corner case bugs. A set that cleans up the 802.3ad code will 
follow shortly.

This patch applies on 2.4.23-pre5. It would also apply on 2.6.0 after 
Amir's patch 2/10 from the "[bonding 2.6] propagating master's 
settings to slaves" set is accepted by Jeff and applied on 2.6.

patch set can be downloaded from:
http://osdn.dl.sourceforge.net/sourceforge/bonding/bonding-cleanup-2.4.23-pre5.tar.bz2

This will update the following files:

        Documentation/networking/bonding.txt
        Documentation/networking/ifenslave.c
        drivers/net/bonding/bond_3ad.c
        drivers/net/bonding/bond_alb.c
        drivers/net/bonding/bond_alb.h
        drivers/net/bonding/bonding.h
        drivers/net/bonding/bond_main.c
        include/linux/if_bonding.h

Description:
patch 1 - ifenslave lite - No more IP settings to slaves, unified 
          printing format, code re-org and broken to more functions.
patch 2 - convert all debug prints to use the dprintk macro and 
          consolidate format of all prints (e.g. "bonding: Error: 
          ...").
patch 3 - death of typedef. eliminate bonding_t/slave_t types and 
          consolidate casting.
patch 4 - remove dead code, old compatibility stuff and redundant 
          checks.
patch 5 - consolidate timers initialization, error checking and 
          re-queuing.
patch 6 - convert too long if-else to a switch-case. Fix all locations 
          that handles bond->primary.
patch 7 - eliminate the multicast_mode module param. settings are now 
          done only according to mode.
patch 8 - slave list iteration - bond is no longer part of the list, 
          added cyclic list iteration macros.
patch 9 - consolidate function declarations:
          o all functions begin with bond_
          o return value, function name and all params are on the same 
            line.
          o change some function names to be more descriptive.
patch 10 - consolidate names of function params and variables (e.g. 
           bond_dev instead of dev/master/master_dev).
patch 11 - change names/types for some of the members in struct 
           bonding. change position of members.
patch 12 - consolidate return values of functions.
patch 13 - put curly braces around all if, else, for, while, switch 
           statements. change conditions to short format.
           e.g. (ptr == NULL) ==> (!ptr)
patch 14 - consolidate error handling in all xmit functions.
patch 15 - chomp all trailing white space.
patch 16 - remove duplicate empty lines. add empty lines where needed 
           to improve readability.
patch 17 - fix indentations.
patch 18 - code re-organization in bond_main.c according to context
           (e.g. module initialization, bond initialization, device 
           entry points, monitoring, etc.). some last minute minor 
           changes and fixes.

-- 
| Shmulik Hen   Advanced Network Services  |
| Israel Design Center, Jerusalem          |
| LAN Access Division, Platform Networking |
| Intel Communications Group, Intel corp.  |


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH SET][bonding] cleanup
  2003-09-25 12:49 [PATCH SET][bonding] cleanup Shmulik Hen
@ 2003-09-25 16:22 ` Jay Vosburgh
  2003-09-25 16:47 ` [Bonding-announce] " Chad N. Tindel
  1 sibling, 0 replies; 8+ messages in thread
From: Jay Vosburgh @ 2003-09-25 16:22 UTC (permalink / raw)
  To: shmulik.hen
  Cc: bonding-devel, bonding-announce, netdev, linux-kernel, linux-net,
	Jeff Garzik, Noam, Amir, Mendelson, Tsippy, Noam, Marom


>patch 7 - eliminate the multicast_mode module param. settings are now 
>          done only according to mode.

	This goes a bit beyond straight cleanup; could you explain the
rationale for this change?  Also, unless I'm missing something, the
patch does not appear to update bonding.txt to reflect the fact that
the module parameter is no more.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bonding-announce] [PATCH SET][bonding] cleanup
  2003-09-25 12:49 [PATCH SET][bonding] cleanup Shmulik Hen
  2003-09-25 16:22 ` Jay Vosburgh
@ 2003-09-25 16:47 ` Chad N. Tindel
  2003-09-25 17:11   ` Shmulik Hen
  1 sibling, 1 reply; 8+ messages in thread
From: Chad N. Tindel @ 2003-09-25 16:47 UTC (permalink / raw)
  To: Shmulik Hen
  Cc: bonding-devel, bonding-announce, netdev, linux-kernel, linux-net,
	Jeff Garzik, Jay Vosburgh, Noam, Amir, Mendelson, Tsippy, Noam,
	Marom

> patch set can be downloaded from:
> http://osdn.dl.sourceforge.net/sourceforge/bonding/bonding-cleanup-2.4.23-pre5.tar.bz2
> 
> This will update the following files:
> 
>         Documentation/networking/bonding.txt
>         Documentation/networking/ifenslave.c
>         drivers/net/bonding/bond_3ad.c
>         drivers/net/bonding/bond_alb.c
>         drivers/net/bonding/bond_alb.h
>         drivers/net/bonding/bonding.h
>         drivers/net/bonding/bond_main.c
>         include/linux/if_bonding.h
> 
> Description:
> patch 1 - ifenslave lite - No more IP settings to slaves, unified 
>           printing format, code re-org and broken to more functions.
> patch 2 - convert all debug prints to use the dprintk macro and 
>           consolidate format of all prints (e.g. "bonding: Error: 
>           ...").
> patch 3 - death of typedef. eliminate bonding_t/slave_t types and 
>           consolidate casting.
> patch 4 - remove dead code, old compatibility stuff and redundant 
>           checks.

I'm a bit concerned about doing some of this stuff in the 2.4 series.  That
compatibility stuff is there for a reason, and was set to be removed in
2.6.  Perhaps we shouldn't be doing stuff this drastic until 2.6 because of
the risk of breaking users.  

Chad

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bonding-announce] [PATCH SET][bonding] cleanup
  2003-09-25 16:47 ` [Bonding-announce] " Chad N. Tindel
@ 2003-09-25 17:11   ` Shmulik Hen
  2003-09-25 17:33     ` Jay Vosburgh
  0 siblings, 1 reply; 8+ messages in thread
From: Shmulik Hen @ 2003-09-25 17:11 UTC (permalink / raw)
  To: Chad N. Tindel
  Cc: bonding-devel, bonding-announce, netdev, linux-kernel, linux-net,
	Jeff Garzik, Jay Vosburgh, Noam, Amir, Mendelson, Tsippy, Noam,
	Marom

On Thursday 25 September 2003 07:47 pm, Chad N. Tindel wrote:
> > patch 4 - remove dead code, old compatibility stuff and redundant
> >           checks.
>
> I'm a bit concerned about doing some of this stuff in the 2.4
> series.  That compatibility stuff is there for a reason, and was
> set to be removed in 2.6.  Perhaps we shouldn't be doing stuff this
> drastic until 2.6 because of the risk of breaking users.

That's the word I got from Jay in response to the " [Kernel-janitors] 
old ioctl definitions in 2.5" thread.

>Jay Vosburgh <fubar@us.ibm.com> wrote:
>	I was going to add it on to the end of the clean up set, but
> if you want to do it, go ahead.  Nobody seems to have objected to
> removing the _OLD stuff, which I view as a good thing.

-- 
| Shmulik Hen   Advanced Network Services  |
| Israel Design Center, Jerusalem          |
| LAN Access Division, Platform Networking |
| Intel Communications Group, Intel corp.  |


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bonding-announce] [PATCH SET][bonding] cleanup
  2003-09-25 17:11   ` Shmulik Hen
@ 2003-09-25 17:33     ` Jay Vosburgh
  2003-09-25 17:43       ` Shmulik Hen
  2003-09-25 21:13       ` Chad N. Tindel
  0 siblings, 2 replies; 8+ messages in thread
From: Jay Vosburgh @ 2003-09-25 17:33 UTC (permalink / raw)
  To: shmulik.hen
  Cc: Chad N. Tindel, bonding-devel, netdev, linux-kernel, linux-net,
	Jeff Garzik, Noam, Amir, Mendelson, Tsippy, Noam, Marom


	[removed bonding-announce from cc:]

>On Thursday 25 September 2003 07:47 pm, Chad N. Tindel wrote:
>> > patch 4 - remove dead code, old compatibility stuff and redundant
>> >           checks.
>>
>> I'm a bit concerned about doing some of this stuff in the 2.4
>> series.  That compatibility stuff is there for a reason, and was
>> set to be removed in 2.6.  Perhaps we shouldn't be doing stuff this
>> drastic until 2.6 because of the risk of breaking users.
>
>That's the word I got from Jay in response to the " [Kernel-janitors] 
>old ioctl definitions in 2.5" thread.
>
>>Jay Vosburgh <fubar@us.ibm.com> wrote:
>>	I was going to add it on to the end of the clean up set, but
>> if you want to do it, go ahead.  Nobody seems to have objected to
>> removing the _OLD stuff, which I view as a good thing.

	My thinking here is that any ifenslave old enough (two years
or more) to still be using the OLD ioctl values is unlikely to work
with the current kernel driver, and if somebody did try it, it's
better to have the call fail outright than perform weird and
mysterious rituals in kernel memory.  I have trouble envisioning an
scenario where a user would be using the latest 2.4.23 kernel, but an
ifenslave from, what, 2.2.15? 2.4.5? or so.

	Separately, recent ifenslaves have compatibility code within
them to cover the great "ifenslave calling sequence change" from April
or so.  As much as I love the sleek new slimmed down ifenslave, I'm
not absolutely sure we can nuke that compatibility stuff within
ifenslave.  I really, really wanna, but I'm not sure if it will cause
problems for end users.  This is the upgrade scenario that prompted
the creation of the whole "ABI version" and compat stuff in the first
place; if we don't have to worry about that, then the simpler
ifenslave can be used, and I think the ethtool ABI version hack can go
away (since we wouldn't need an ABI version if there's only one).

	Comments?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bonding-announce] [PATCH SET][bonding] cleanup
  2003-09-25 17:33     ` Jay Vosburgh
@ 2003-09-25 17:43       ` Shmulik Hen
  2003-09-25 21:13       ` Chad N. Tindel
  1 sibling, 0 replies; 8+ messages in thread
From: Shmulik Hen @ 2003-09-25 17:43 UTC (permalink / raw)
  To: Jay Vosburgh
  Cc: Chad N. Tindel, bonding-devel, netdev, linux-kernel, linux-net,
	Jeff Garzik, Noam, Amir, Mendelson, Tsippy, Noam, Marom

On Thursday 25 September 2003 08:33 pm, Jay Vosburgh wrote:
>
> 	Separately, recent ifenslaves have compatibility code within
> them to cover the great "ifenslave calling sequence change" from
> April or so.  As much as I love the sleek new slimmed down
> ifenslave, I'm not absolutely sure we can nuke that compatibility
> stuff within ifenslave.  I really, really wanna, but I'm not sure
> if it will cause problems for end users.  This is the upgrade
> scenario that prompted the creation of the whole "ABI version" and
> compat stuff in the first place; if we don't have to worry about
> that, then the simpler ifenslave can be used, and I think the
> ethtool ABI version hack can go away (since we wouldn't need an ABI
> version if there's only one).
>
> 	Comments?
>

I think I better leave this for Amir to answer. He's our ABI expert 
and this needs carefull consideration, especially now that he's 
working on enhancing ifenslave's capabilities for the hot operation 
stuff. However, he won't be able to do that before Monday since we're 
going out on a long weekend - it's holiday season over here.

-- 
| Shmulik Hen   Advanced Network Services  |
| Israel Design Center, Jerusalem          |
| LAN Access Division, Platform Networking |
| Intel Communications Group, Intel corp.  |


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bonding-announce] [PATCH SET][bonding] cleanup
  2003-09-25 17:33     ` Jay Vosburgh
  2003-09-25 17:43       ` Shmulik Hen
@ 2003-09-25 21:13       ` Chad N. Tindel
  2003-10-05 22:28         ` Willy TARREAU
  1 sibling, 1 reply; 8+ messages in thread
From: Chad N. Tindel @ 2003-09-25 21:13 UTC (permalink / raw)
  To: Jay Vosburgh
  Cc: shmulik.hen, Chad N. Tindel, bonding-devel, netdev, linux-kernel,
	linux-net, Jeff Garzik, Noam, Amir, Mendelson, Tsippy, Noam,
	Marom

> >>	I was going to add it on to the end of the clean up set, but
> >> if you want to do it, go ahead.  Nobody seems to have objected to
> >> removing the _OLD stuff, which I view as a good thing.
> 
> 	My thinking here is that any ifenslave old enough (two years
> or more) to still be using the OLD ioctl values is unlikely to work
> with the current kernel driver, and if somebody did try it, it's
> better to have the call fail outright than perform weird and
> mysterious rituals in kernel memory.  I have trouble envisioning an
> scenario where a user would be using the latest 2.4.23 kernel, but an
> ifenslave from, what, 2.2.15? 2.4.5? or so.

I was specifically told by David Miller that we are not to break binary
compatibility within a 2.4 release.  Such things had to wait until 2.5 
or later.  We can not require a user to upgrade their ifenslave within a 2.4
series kernel just to keep using the same functionality they were using in 
2.4.1.  Obviously we can require them to upgrade in order to keep using
new functionality.  So the _OLD stuff needs to stay in the 2.4 kernel.  If
this was brought up in an earlier thread, then I just missed it.

Chad

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bonding-announce] [PATCH SET][bonding] cleanup
  2003-09-25 21:13       ` Chad N. Tindel
@ 2003-10-05 22:28         ` Willy TARREAU
  0 siblings, 0 replies; 8+ messages in thread
From: Willy TARREAU @ 2003-10-05 22:28 UTC (permalink / raw)
  To: Jay Vosburgh, shmulik.hen, Chad N. Tindel, bonding-devel, netdev,
	linux-kernel, linux-net, Jeff Garzik, Noam, Amir, Mendelson,
	Tsippy, Noam, Marom

Hi !

On Thu, Sep 25, 2003 at 05:13:00PM -0400, Chad N. Tindel wrote:
 
> I was specifically told by David Miller that we are not to break binary
> compatibility within a 2.4 release.  Such things had to wait until 2.5 
> or later.  We can not require a user to upgrade their ifenslave within a 2.4
> series kernel just to keep using the same functionality they were using in 
> 2.4.1.

I strongly agree. I have been facing this problem and it was really a pain. I
used the last bonding version which didn't define the ABI version, together
with the associated ifenslave, but when the need to upgrade to plain 2.4.22
came in, I had the surprize of getting a non-working bonding because this
intermediate ifenslave. Well, I upgraded it to latest version, which prevents
me from downgrading to the previous kernel because it has ABIv2 with no version,
so the newer ifenslave thinks it's an ABIv1. So the result is a symlink with
two versions of ifenslave on the disk, just in case I have to downgrade.

Although I agree it's clearly my fault and I should have been more careful, I
prefer to warn everyone about the consequences this might have on production
systems. Schmulik has done quite a great job here, and I believe most of it
should be integrated, but we have to carefully test each combination of old/new
ifenslave with old/new driver if we don't want to break some setups or prevent
admins from downgrading if something goes wrong.

Cheers,
Willy


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2003-10-05 22:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-09-25 12:49 [PATCH SET][bonding] cleanup Shmulik Hen
2003-09-25 16:22 ` Jay Vosburgh
2003-09-25 16:47 ` [Bonding-announce] " Chad N. Tindel
2003-09-25 17:11   ` Shmulik Hen
2003-09-25 17:33     ` Jay Vosburgh
2003-09-25 17:43       ` Shmulik Hen
2003-09-25 21:13       ` Chad N. Tindel
2003-10-05 22:28         ` Willy TARREAU

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).