Netdev List
 help / color / mirror / Atom feed
* RE: [RFC] CAIF Protocol Stack
From: Sjur Brændeland @ 2009-09-18 13:38 UTC (permalink / raw)
  To: Rémi Denis-Courmont, netdev
In-Reply-To: <0f510ae3e0a78a2c1345d8e08bdafb0e@chewa.net>

> -----Original Message-----
> From: Rémi Denis-Courmont
> Sent: 18. september 2009 14:32
>     Hello,
> 
> On Wed, 16 Sep 2009 14:30:34 +0200, Sjur Brændeland 
> <sjur.brandeland@stericsson.com> wrote:
> > The Implementation of CAIF is divided into:
> > * CAIF Devices: Character Device, Net Device and Kernel API.
> > * CAIF Protocol Implementation
> > * CAIF Link Layer
> 
> I'm a bit confused here. What do you call a CAIF Device?
> 
> Do you mean a GPRS context is a network device, and an AT 
> command interface is a character device? Or is the CAIF modem 
> a device? or what?

What I meant was:
* "Net Device" - a "struct net_device" with one instance for each GPRS PDP context.
* "Character Device" - a chr device, with one instance for each AT channel towards the modem.

BR/Sjur Brændeland

^ permalink raw reply

* Re: [RFC] CAIF Protocol Stack
From: Rémi Denis-Courmont @ 2009-09-18 12:31 UTC (permalink / raw)
  To: netdev
In-Reply-To: <61D8D34BB13CFE408D154529C120E07902DF9076@eseldmw101.eemea.ericsson.se>


    Hello,

On Wed, 16 Sep 2009 14:30:34 +0200, Sjur Brændeland
<sjur.brandeland@stericsson.com> wrote:
> The Implementation of CAIF is divided into:
> * CAIF Devices: Character Device, Net Device and Kernel API.
> * CAIF Protocol Implementation
> * CAIF Link Layer

I'm a bit confused here. What do you call a CAIF Device?

Do you mean a GPRS context is a network device, and an AT command interface
is a character device? Or is the CAIF modem a device? or what?

-- 
Rémi Denis-Courmont


^ permalink raw reply

* RE: [RFC] CAIF Protocol Stack
From: Sjur Brændeland @ 2009-09-18 12:01 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

Hi David.

I understand that you are one of the main Maintainers of netdev.
As explained below we have a largeish driver we would like to contribute.
I realize we should have started contributing this on a earlier stage...., but
What is the preferred way of doing this, i.e. how should we split it up?
   Submit the whole shebang,
Or
   Split Horizontally e.g. a) CAIF-Protocol, b) GPRS-Net-Device c) CAIF-Link Layer
Or
   Split Vertically e.g. a) Payload Path Net-Device, b) Payload Path AT-channel, c) Configuration

Which kernel GIT should we base the patch set on?

Any hints on this would be greatly appreciated.

Best Regards
Sjur Brændeland
ST-Ericsson


> -----Original Message-----
> From: Sjur Brændeland 
> Sent: 16. september 2009 14:31
> To: 'netdev@vger.kernel.org'
> Subject: [RFC] CAIF Protocol Stack
> 
> Hello,
> 
> We are currently working on a patch set in order to introduce 
> the CAIF protocol in Linux. CAIF (Communication CPU to 
> Application CPU Interface) is the primary protocol used to 
> communicate between to ST-Ericsson modem and external host system. 
> 
> The host processes can use CAIF to open virtual AT channels, 
> initiate GPRS Data connections, Video channels and Utility Channels.
> The Utility Channels are general purpose pipes between modem and host.
> 
> ST-Ericsson modems support a number of Link Layers between 
> modem and host, currently Uart and Shared Memory are 
> available for Linux.
> 
> Architecture:
> ------------
> The Implementation of CAIF is divided into:
> * CAIF Devices: Character Device, Net Device and Kernel API.
> * CAIF Protocol Implementation
> * CAIF Link Layer
> 
> In order to configure the devices a set of IOCTLs is used.
> 
> 
> 
>   IOCTL                                  
>    !                                     
>    !     +------+   +------+   +------+                 
>    !    +------+!  +------+!  +------+!    
>    !    ! Chr  !!  !Kernel!!  ! Net  !!
>    !    ! Dev  !+  ! API  !+  ! Dev  !+   <- CAIF Devices
>    !    +------+   +------!   +------+           
>    !       !          !          !       
>    !       +----------!----------+
>    !               +------+               <- CAIF Protocol 
> Implementation
>    +------->       ! CAIF !                  /dev/caifconfig
>                    +------+                  
>              +--------!--------+         
>              !                 !              
>           +------+          +-----+     
>           !ShMem !          ! TTY !       <- Link Layer          
>           +------+          +-----+           
> 
> Any comments welcome.
> 
> 
> 
> Files:
> -----
> 
>  net/caif/Kconfig                                   |   61 +
>  net/caif/Makefile                                  |   62 +
>  net/caif/caif_chnlif.c                             |  209 ++++
>  net/caif/caif_chr.c                                |  392 +++++++
>  net/caif/caif_config_util.c                        |  279 +++++
>  net/caif/chnl_chr.c                                | 1161 
> ++++++++++++++++++++
>  net/caif/chnl_net.c                                |  338 ++++++
>  net/caif/generic/cfcnfg.c                          |  722 
> ++++++++++++
>  net/caif/generic/cfctrl.c                          |  640 +++++++++++
>  net/caif/generic/cfdgml.c                          |  119 ++
>  net/caif/generic/cffrml.c                          |  144 +++
>  net/caif/generic/cflist.c                          |   99 ++
>  net/caif/generic/cfloopcfg.c                       |   93 ++
>  net/caif/generic/cflooplayer.c                     |  113 ++
>  net/caif/generic/cfmsll.c                          |   55 +
>  net/caif/generic/cfmuxl.c                          |  270 +++++
>  net/caif/generic/cfpkt_skbuff.c                    |  545 +++++++++
>  net/caif/generic/cfrfml.c                          |  112 ++
>  net/caif/generic/cfserl.c                          |  297 +++++
>  net/caif/generic/cfshml.c                          |   67 ++
>  net/caif/generic/cfspil.c                          |  245 ++++
>  net/caif/generic/cfsrvl.c                          |  177 +++
>  net/caif/generic/cfutill.c                         |  115 ++
>  net/caif/generic/cfveil.c                          |  118 ++
>  net/caif/generic/cfvidl.c                          |   68 ++
>  net/caif/generic/fcs.c                             |   58 +
> 
>  drivers/net/caif/Kconfig                           |   58 +
>  drivers/net/caif/Makefile                          |   29 +
>  drivers/net/caif/chnl_tty.c                        |  217 ++++
>  drivers/net/caif/phyif_loop.c                      |  418 +++++++
>  drivers/net/caif/phyif_ser.c                       |  182 +++
>  drivers/net/caif/phyif_shm.c                       |  838 
> ++++++++++++++
>  drivers/net/caif/shm.h                             |   95 ++
>  drivers/net/caif/shm_cfgifc.c                      |   63 ++
>  drivers/net/caif/shm_mbxifc.c                      |  104 ++
>  drivers/net/caif/shm_smbx.c                        |   78 ++
> 
>  include/linux/caif/caif_config.h                   |  231 ++++
>  include/linux/caif/caif_ioctl.h                    |  106 ++
>  include/net/caif/caif_actions.h                    |   81 ++
>  include/net/caif/caif_chr.h                        |   46 +
>  include/net/caif/caif_config_util.h                |   27 +
>  include/net/caif/caif_kernel.h                     |  324 ++++++
>  include/net/caif/caif_log.h                        |   57 +
>  include/net/caif/generic/caif_layer.h              |  476 ++++++++
>  include/net/caif/generic/cfcnfg.h                  |  223 ++++
>  include/net/caif/generic/cfctrl.h                  |  139 +++
>  include/net/caif/generic/cffrml.h                  |   29 +
>  include/net/caif/generic/cfglue.h                  |  387 +++++++
>  include/net/caif/generic/cfloopcfg.h               |   28 +
>  include/net/caif/generic/cflst.h                   |   27 +
>  include/net/caif/generic/cfmsll.h                  |   22 +
>  include/net/caif/generic/cfmuxl.h                  |   30 +
>  include/net/caif/generic/cfpkt.h                   |  246 +++++
>  include/net/caif/generic/cfserl.h                  |   22 +
>  include/net/caif/generic/cfshml.h                  |   21 +
>  include/net/caif/generic/cfspil.h                  |   80 ++
>  include/net/caif/generic/cfsrvl.h                  |   48 +
>  include/net/caif/generic/fcs.h                     |   22 +
> 
> 
> 
> Regards
> Sjur Brandeland
> ST-Ericsson

^ permalink raw reply

* [PATCH net-next-2.6] bonding: set primary param via sysfs
From: Jiri Pirko @ 2009-09-18 12:13 UTC (permalink / raw)
  To: netdev; +Cc: davem, fubar, bonding-devel

Primary module parameter passed to bonding is pernament. That means if you
release the primary slave and enslave it again, it becomes the primary slave
again. But if you set primary slave via sysfs, the primary slave is only set
once and it's not remembered in bond->params structure. Therefore the setting is
lost after releasing the primary slave. This simple one-liner fixes this.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>

diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 6044e12..ff449de 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1182,6 +1182,7 @@ static ssize_t bonding_store_primary(struct device *d,
 				       ": %s: Setting %s as primary slave.\n",
 				       bond->dev->name, slave->dev->name);
 				bond->primary_slave = slave;
+				strcpy(bond->params.primary, slave->dev->name);
 				bond_select_active_slave(bond);
 				goto out;
 			}

^ permalink raw reply related

* Re: tun: Return -EINVAL if neither IFF_TUN nor IFF_TAP is set.
From: Paul Moore @ 2009-09-18 11:54 UTC (permalink / raw)
  To: Kusanagi Kouichi; +Cc: netdev, linux-kernel
In-Reply-To: <20090917073614.15217260031@msa105lp.auone-net.jp>

On Thursday 17 September 2009 03:36:13 am Kusanagi Kouichi wrote:
> After commit 2b980dbd77d229eb60588802162c9659726b11f4
> ("lsm: Add hooks to the TUN driver") tun_set_iff doesn't
> return -EINVAL though neither IFF_TUN nor IFF_TAP is set.
> 
> Signed-off-by: Kusanagi Kouichi <slash@ma.neweb.ne.jp>

Sorry about that, my mistake, thanks for finding and fixing this.

Reviewed-by: Paul Moore <paul.moore@hp.com>

> ---
>  drivers/net/tun.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 3f5d288..e091756 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -946,8 +946,6 @@ static int tun_set_iff(struct net *net, struct file
>  *file, struct ifreq *ifr) char *name;
>  		unsigned long flags = 0;
> 
> -		err = -EINVAL;
> -
>  		if (!capable(CAP_NET_ADMIN))
>  			return -EPERM;
>  		err = security_tun_dev_create();
> @@ -964,7 +962,7 @@ static int tun_set_iff(struct net *net, struct file
>  *file, struct ifreq *ifr) flags |= TUN_TAP_DEV;
>  			name = "tap%d";
>  		} else
> -			goto failed;
> +			return -EINVAL;
> 
>  		if (*ifr->ifr_name)
>  			name = ifr->ifr_name;
> 

-- 
paul moore
linux @ hp

^ permalink raw reply

* tcp_sock variable initialization
From: Armin Abfalterer @ 2009-09-18  8:50 UTC (permalink / raw)
  To: netdev

Hi!

I need a control variable (ecnn_flags) in tcp_sock that should be set
properly after the 3-way-handshake in tcp_create_openreq_child(). If I
set the variable in its value is always 0 afterwards.

struct sock *tcp_create_openreq_child( ... )
{
	struct sock *newsk = inet_csk_clone(sk, req, GFP_ATOMIC);

	if (newsk != NULL) {
		struct tcp_sock *newtp;

		newtp = tcp_sk(newsk);
		newtp->ecnn_flags |= TCP_ECN_NONCE_OK;
	}
}

When I read the variable for the next outgoing segment the values is not
set.

static int tcp_transmit_skb( ... )
{
	struct tcp_sock *tp;

	
	if (tp->ecnn_flags & TCP_ECN_NONCE_OK) {
		/*
		* never entered!!!!
		*/
	}
}

I'm quite sure that it has to do with the creation of the big socket
when the connection enters TCP_ESTABLISHED but searching for hours
didn't help to find the right place where my variable is re-initialized.

Any hint in the right direction would greatly appreciated!!! Thanks!

Armin


^ permalink raw reply

* Re: ipv4 regression in 2.6.31 ?
From: Stephan von Krawczynski @ 2009-09-18  8:30 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Jarek Poplawski, David Miller, Eric Dumazet, linux-kernel,
	Linux Netdev List
In-Reply-To: <20090916100028.654f7893@nehalam>

On Wed, 16 Sep 2009 10:00:28 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> On Wed, 16 Sep 2009 05:23:04 +0000
> Jarek Poplawski <jarkao2@gmail.com> wrote:
> 
> > On Tue, Sep 15, 2009 at 03:57:19PM -0700, Stephen Hemminger wrote:
> > > On Tue, 15 Sep 2009 08:13:55 +0000
> > > Jarek Poplawski <jarkao2@gmail.com> wrote:
> > > 
> > > > On 14-09-2009 18:31, Stephen Hemminger wrote:
> > > > > On Mon, 14 Sep 2009 17:55:05 +0200
> > > > > Stephan von Krawczynski <skraw@ithnet.com> wrote:
> > > > > 
> > > > >> On Mon, 14 Sep 2009 15:57:03 +0200
> > > > >> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > > >>
> > > > >>> Stephan von Krawczynski a A~(c)crit :
> > > > >>>> Hello all,
> > > > ...
> > > > >>> rp_filter - INTEGER
> > > > >>>         0 - No source validation.
> > > > >>>         1 - Strict mode as defined in RFC3704 Strict Reverse Path
> > > > >>>             Each incoming packet is tested against the FIB and if the interface
> > > > >>>             is not the best reverse path the packet check will fail.
> > > > >>>             By default failed packets are discarded.
> > > > >>>         2 - Loose mode as defined in RFC3704 Loose Reverse Path
> > > > >>>             Each incoming packet's source address is also tested against the FIB
> > > > >>>             and if the source address is not reachable via any interface
> > > > >>>             the packet check will fail.
> > > > ...
> > > > > RP filter did not work correctly in 2.6.30. The code added to to the loose
> > > > > mode caused a bug; the rp_filter value was being computed as:
> > > > >   rp_filter = interface_value & all_value;
> > > > > So in order to get reverse path filter both would have to be set.
> > > > > 
> > > > > In 2.6.31 this was change to:
> > > > >    rp_filter = max(interface_value, all_value);
> > > > > 
> > > > > This was the intended behaviour, if user asks all interfaces to have rp
> > > > > filtering turned on, then set /proc/sys/net/ipv4/conf/all/rp_filter = 1
> > > > > or to turn on just one interface, set it for just that interface.
> > > > 
> > > > Alas this max() formula handles also cases where both values are set
> > > > and it doesn't look very natural/"user friendly" to me. Especially
> > > > with something like this: all_value = 2; interface_value = 1
> > > > Why would anybody care to bother with interface_value in such a case?
> > > > 
> > > > "All" suggests "default" in this context, so I'd rather expect
> > > > something like:
> > > >     rp_filter = interface_value ? : all_value;
> > > > which gives "the inteded behaviour" too, plus more...
> > > > 
> > > > We'd only need to add e.g.:
> > > >  0 - Default ("all") validation. (No source validation if "all" is 0).
> > > >  3 - No source validation on this interface.
> > > 
> > > More values == more confusion.
> > > I chose the maxconf() method to make rp_filter consistent with other
> > > multi valued variables (arp_announce and arp_ignore).
> > 
> > This additional value is not necessary (it'd give as superpowers).
> > Max seems logical to me only when values are sorted (especially if
> > max is the strictest).
> 
> The values had to be unsorted because of the requirement to retain
> interface compatibility with older releases.

The parameters are the same (I guess this is what you call interface
compatibility), but the function came out different, meaning you broke
functional compatibility with 2.6.31 instead. Just to mention that - though
the argument is leight-weight for the compatibility broke because the whole
thing was broken somehow before the bugfix.

-- 
Regards,
Stephan


^ permalink raw reply

* Re: [PATCH] ks8851_ml ethernet network driver
From: Greg KH @ 2009-09-18  5:27 UTC (permalink / raw)
  To: David Miller; +Cc: David.Choi, netdev, Charles.Li, Choi, jgarzik, shemminger
In-Reply-To: <20090917.164952.33104590.davem@davemloft.net>

On Thu, Sep 17, 2009 at 04:49:52PM -0700, David Miller wrote:
> From: "Choi, David" <David.Choi@Micrel.Com>
> Date: Thu, 17 Sep 2009 12:30:27 -0700
> 
> > --- linux-2.6.31-rc3/drivers/net/ks8851_mll.c.orig	2009-09-17
> > 10:18:56.000000000 -0700
> > +++ linux-2.6.31-rc3/drivers/net/ks8851_mll.c	2009-09-17
> > 10:09:37.000000000 -0700
> > @@ -21,8 +21,6 @@
> >   * KS8851 16bit MLL chip from Micrel Inc.
> 
> I can't use this patch or even test it, as your email client
> has corrupted it by, for example, breaking up long lines.

Yeah, that's why I had to post the original patch for David :(

I'm going to be away from email for the next 10 days due to conferences,
so hopefully David can fix the email issue so he can repost it
himself...

thanks,

greg k-h

^ permalink raw reply

* Re: Netlink API for bonding ?
From: Stephen Hemminger @ 2009-09-18  4:00 UTC (permalink / raw)
  To: Nicolas de Pesloüan; +Cc: Jay Vosburgh, bonding-devel, netdev, Jiri Pirko
In-Reply-To: <4AB2B3EF.50307@free.fr>

On Fri, 18 Sep 2009 00:10:55 +0200
Nicolas de Pesloüan <nicolas.2p.debian@free.fr> wrote:

> Stephen Hemminger wrote:
> > On Thu, 17 Sep 2009 23:44:30 +0200
> > Nicolas de Pesloüan <nicolas.2p.debian@free.fr> wrote:
> > 
> >> Stephen Hemminger wrote:
> >>> On Mon, 31 Aug 2009 22:34:50 +0200
> >>> Nicolas de Pesloüan <nicolas.2p.debian@free.fr> wrote:
> >>>
> >>>> Stephen,
> >>>>
> >>>> Can you please describe the netlink API you plan to implement for bonding ?
> >>>>
> >>>> Both Jiri Pirko and I plan to add some advanced active slave selection rules, 
> >>>> for more-than-two-slaves bonding configuration.
> >>>>
> >>>> Jay suggested that such advanced features be implemented in user space, using 
> >>>> netlink to notify a daemon when slaves come up or fall down. I agree with Jay, 
> >>>> but don't want to design something without having first a view at your proposed 
> >>>> API for bonding.
> >>>>
> >>>> Do you plan to have some notification to user space, or only the ability to read 
> >>>> and set bonding configuration using netlink ?
> >>>>
> >>>> Thanks,
> >>>>
> >>>> 	Nicolas.
> >>> No paper spec, but was looking to add interface similar to vlan and macvlan.
> >>> Just use (and extend if needed) existing rtnl_link_ops.
> >>>
> >>>
> >>> Was not planning on adding a notification interface, thats good idea but
> >>> really not what I was looking at.
> >> What kind of notification system would you suggest to notify userland that a 
> >> given bond device just lose the current active slave ?
> > 
> > First why should user land care?  Unless all slaves are gone maybe it
> > should just be transparent.
> 
> Because we try to design a notification from kernel to userland when current 
> active slave fail, to give an opportunity to userland to decide which non-failed 
> slave should become the new active one. This is in order to try and move complex 
> decisions to userland, only keeping very simple "two slaves" decisions into the 
> kernel.
> 
> Think of it as the bonding counter part of moving STP to userland for bridge. 
> Userland should be able to decide which slave should be the active one for the 
> same reasons userland is able to decide which bridge port should be forwarding 
> and which should be blocked.
> 
> I assume that we cannot just try to make the current bridge userland 
> notification system more generic. May be I'm wrong. May be the ability to notify 
> port failure, port coming back and BPDU for bridge is a superset of what we need 
> to notify port failure and port coming back for bonding.
> 
> > Use existing link ops mechanism (see vlan and macvlan). You may need
> > to add new operations, but these should be generic enough so that bonding and bridging
> > have same operations. 
> > 
> >      .newlink => create bond device
> >      .dellink => remove bond device
> >      .newport => add slave
> >      .delport => remove slave
> > 
> > Also, dellink should always work (even if slaves are present).
> 
> This sounds perfect for setup, but might not be good the elect the "best" port 
> (active slave). Also, I assume a new RTNETLINK operation needs to be added for 
> that. I thought that this was what you were working on. Do I miss something ? 
> Does brctl use RTNETLINK for port setup ? Or do you plan to use iproute2 to 
> replace brctl in the futur ?

I got to busy to get past making bonding amenable to using newlink/delink.
One common way to handle changes is to send another NEWXXX message with
different parameters (TLV values).

> > The terminology slave is not widely used outside of bonding, and so probably
> > shouldn't be buried in the API.
> 
> Yes, you are definitely right with this point.
> 
> 	Nicolas.


^ permalink raw reply

* Re: [net-2.6 PATCH 1/6] net: initialize rmem_alloc and omem_alloc to 0 in netlink socket
From: David Miller @ 2009-09-18  1:29 UTC (permalink / raw)
  To: jeffrey.t.kirsher; +Cc: netdev, gospo, linux-scsi, john.r.fastabend
In-Reply-To: <20090918005708.25594.52575.stgit@localhost.localdomain>

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 17 Sep 2009 17:57:09 -0700

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> The rmem_alloc and omem_alloc socket fields are not
> initialized.  This sets each variable to zero when a socket
> is created.  Note the sk_wmem_alloc is already initialized
> in sock_init_data.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

It's set to zero implicitly by the memset() done at sock_alloc()
time.

Re-setting it again here explicitly will just add unnecessary
memory traffic.

^ permalink raw reply

* Re: [net-2.6 PATCH 5/6] net: fix sock locking for sk_err field in netlink.
From: David Miller @ 2009-09-18  1:27 UTC (permalink / raw)
  To: jeffrey.t.kirsher; +Cc: netdev, gospo, linux-scsi, john.r.fastabend
In-Reply-To: <20090918005832.25594.45086.stgit@localhost.localdomain>

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 17 Sep 2009 17:58:32 -0700

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> This adds the sock lock around setting the sk_err field
> in sock struct. Without the lock multiple threads may
> write to this field.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

This isn't right.

Writes to sk->sk_err can occur asynchronously just fine and
without any locking.

The only requirement is that consumers of the sk_err value
use sock_error() which uses xchg() to get and clear the
value atomically.

^ permalink raw reply

* Re: [net-2.6 PATCH 2/6] net: remove kfree_skb on a NULL pointer in af_netlink.c
From: David Miller @ 2009-09-18  1:24 UTC (permalink / raw)
  To: jeffrey.t.kirsher; +Cc: netdev, gospo, linux-scsi, john.r.fastabend
In-Reply-To: <20090918005729.25594.14261.stgit@localhost.localdomain>

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 17 Sep 2009 17:57:29 -0700

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> This removes a kfree_skb that is being called on a NULL pointer when
> do_one_broadcast() is sucessful.  And moves the kfree_skb into
> do_one_broadcast() for the error case.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

kfree_skb() on a NULL pointer is completely legal.

^ permalink raw reply

* [net-2.6 PATCH 6/6] net: fix double skb free in dcbnl
From: Jeff Kirsher @ 2009-09-18  0:58 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, linux-scsi, John Fastabend, Jeff Kirsher
In-Reply-To: <20090918005708.25594.52575.stgit@localhost.localdomain>

From: John Fastabend <john.r.fastabend@intel.com>

netlink_unicast() calls kfree_skb even in the error case.

dcbnl calls netlink_unicast() which when it fails free's the
skb and returns an error value.  dcbnl is free'ing the skb
again when this error occurs.  This patch removes the double
free.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/dcb/dcbnl.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index e0879bf..ac1205d 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -194,7 +194,7 @@ static int dcbnl_reply(u8 value, u8 event, u8 cmd, u8 attr, u32 pid,
 	nlmsg_end(dcbnl_skb, nlh);
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		return -EINVAL;
 
 	return 0;
 nlmsg_failure:
@@ -275,7 +275,7 @@ static int dcbnl_getpfccfg(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 nlmsg_failure:
@@ -316,12 +316,11 @@ static int dcbnl_getperm_hwaddr(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 
 nlmsg_failure:
-err:
 	kfree_skb(dcbnl_skb);
 err_out:
 	return -EINVAL;
@@ -383,7 +382,7 @@ static int dcbnl_getcap(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 nlmsg_failure:
@@ -460,7 +459,7 @@ static int dcbnl_getnumtcs(struct net_device *netdev, struct nlattr **tb,
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret) {
 		ret = -EINVAL;
-		goto err;
+		goto err_out;
 	}
 
 	return 0;
@@ -799,7 +798,7 @@ static int __dcbnl_pg_getcfg(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 
@@ -1063,7 +1062,7 @@ static int dcbnl_bcn_getcfg(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 


^ permalink raw reply related

* [net-2.6 PATCH 5/6] net: fix sock locking for sk_err field in netlink.
From: Jeff Kirsher @ 2009-09-18  0:58 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, linux-scsi, John Fastabend, Jeff Kirsher
In-Reply-To: <20090918005708.25594.52575.stgit@localhost.localdomain>

From: John Fastabend <john.r.fastabend@intel.com>

This adds the sock lock around setting the sk_err field
in sock struct. Without the lock multiple threads may
write to this field.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/netlink/af_netlink.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index aa74011..1669dfc 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -732,7 +732,9 @@ static void netlink_overrun(struct sock *sk)
 
 	if (!(nlk->flags & NETLINK_RECV_NO_ENOBUFS)) {
 		if (!test_and_set_bit(0, &nlk_sk(sk)->state)) {
+			lock_sock(sk);
 			sk->sk_err = ENOBUFS;
+			release_sock(sk);
 			sk->sk_error_report(sk);
 		}
 	}
@@ -1101,7 +1103,9 @@ static inline int do_one_set_err(struct sock *sk,
 	    !test_bit(p->group - 1, nlk->groups))
 		goto out;
 
+	lock_sock(sk);
 	sk->sk_err = p->code;
+	release_sock(sk);
 	sk->sk_error_report(sk);
 out:
 	return 0;
@@ -1780,7 +1784,9 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 				    in_skb->sk->sk_protocol,
 				    NETLINK_CB(in_skb).pid);
 		if (sk) {
+			lock_sock(sk);
 			sk->sk_err = ENOBUFS;
+			release_sock(sk);
 			sk->sk_error_report(sk);
 			sock_put(sk);
 		}


^ permalink raw reply related

* [net-2.6 PATCH 4/6] net: fix nlmsg len size for skb when error bit is set.
From: Jeff Kirsher @ 2009-09-18  0:58 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, linux-scsi, John Fastabend, Jeff Kirsher
In-Reply-To: <20090918005708.25594.52575.stgit@localhost.localdomain>

From: John Fastabend <john.r.fastabend@intel.com>

Currently, the nlmsg->len field is not set correctly in  netlink_ack()
for ack messages that include the nlmsg of the error frame.  This
corrects the length field passed to __nlmsg_put to use the correct
payload size.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/netlink/af_netlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 9934847..aa74011 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1788,7 +1788,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 	}
 
 	rep = __nlmsg_put(skb, NETLINK_CB(in_skb).pid, nlh->nlmsg_seq,
-			  NLMSG_ERROR, sizeof(struct nlmsgerr), 0);
+			  NLMSG_ERROR, payload, 0);
 	errmsg = nlmsg_data(rep);
 	errmsg->error = err;
 	memcpy(&errmsg->msg, nlh, err ? nlh->nlmsg_len : sizeof(*nlh));


^ permalink raw reply related

* [net-2.6 PATCH 3/6] net: fix vlan_get_size to include vlan_flags size
From: Jeff Kirsher @ 2009-09-18  0:57 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, linux-scsi, John Fastabend, Jeff Kirsher
In-Reply-To: <20090918005708.25594.52575.stgit@localhost.localdomain>

From: John Fastabend <john.r.fastabend@intel.com>

Fix vlan_get_size to include vlan->flags.  Currently, the
size of the vlan flags is not included in the nlmsg size.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/8021q/vlan_netlink.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
index 343146e..a915048 100644
--- a/net/8021q/vlan_netlink.c
+++ b/net/8021q/vlan_netlink.c
@@ -169,6 +169,7 @@ static size_t vlan_get_size(const struct net_device *dev)
 	struct vlan_dev_info *vlan = vlan_dev_info(dev);
 
 	return nla_total_size(2) +	/* IFLA_VLAN_ID */
+	       sizeof(struct ifla_vlan_flags) + /* IFLA_VLAN_FLAGS */
 	       vlan_qos_map_size(vlan->nr_ingress_mappings) +
 	       vlan_qos_map_size(vlan->nr_egress_mappings);
 }


^ permalink raw reply related

* [net-2.6 PATCH 2/6] net: remove kfree_skb on a NULL pointer in af_netlink.c
From: Jeff Kirsher @ 2009-09-18  0:57 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, linux-scsi, John Fastabend, Jeff Kirsher
In-Reply-To: <20090918005708.25594.52575.stgit@localhost.localdomain>

From: John Fastabend <john.r.fastabend@intel.com>

This removes a kfree_skb that is being called on a NULL pointer when
do_one_broadcast() is sucessful.  And moves the kfree_skb into
do_one_broadcast() for the error case.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/netlink/af_netlink.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4e673d2..9934847 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1021,6 +1021,8 @@ static inline int do_one_broadcast(struct sock *sk,
 		netlink_overrun(sk);
 		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
 			p->delivery_failure = 1;
+		kfree_skb(p->skb2);
+		p->skb2 = NULL;
 	} else {
 		p->congested |= val;
 		p->delivered = 1;
@@ -1065,8 +1067,6 @@ int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 pid,
 
 	netlink_unlock_table();
 
-	kfree_skb(info.skb2);
-
 	if (info.delivery_failure)
 		return -ENOBUFS;
 


^ permalink raw reply related

* [net-2.6 PATCH 1/6] net: initialize rmem_alloc and omem_alloc to 0 in netlink socket
From: Jeff Kirsher @ 2009-09-18  0:57 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, linux-scsi, John Fastabend, Jeff Kirsher

From: John Fastabend <john.r.fastabend@intel.com>

The rmem_alloc and omem_alloc socket fields are not
initialized.  This sets each variable to zero when a socket
is created.  Note the sk_wmem_alloc is already initialized
in sock_init_data.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/netlink/af_netlink.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c5aab6a..4e673d2 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -423,6 +423,9 @@ static int __netlink_create(struct net *net, struct socket *sock,
 	}
 	init_waitqueue_head(&nlk->wait);
 
+	atomic_set(&sk->sk_rmem_alloc, 0);
+	atomic_set(&sk->sk_omem_alloc, 0);
+
 	sk->sk_destruct = netlink_sock_destruct;
 	sk->sk_protocol = protocol;
 	return 0;


^ permalink raw reply related

* [net-2.6 PATCH] igb: resolve namespacecheck warning for igb_hash_mc_addr
From: Jeff Kirsher @ 2009-09-18  0:52 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, Alexander Duyck, Jeff Kirsher

From: Alexander Duyck <alexander.h.duyck@intel.com>

This patch resolves a warning seen when doing namespace checking via
"make namespacecheck"

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/e1000_mac.c |   72 ++++++++++++++++++++++---------------------
 drivers/net/igb/e1000_mac.h |    1 -
 2 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/drivers/net/igb/e1000_mac.c b/drivers/net/igb/e1000_mac.c
index a0231cd..7d76bb0 100644
--- a/drivers/net/igb/e1000_mac.c
+++ b/drivers/net/igb/e1000_mac.c
@@ -286,41 +286,6 @@ void igb_mta_set(struct e1000_hw *hw, u32 hash_value)
 }
 
 /**
- *  igb_update_mc_addr_list - Update Multicast addresses
- *  @hw: pointer to the HW structure
- *  @mc_addr_list: array of multicast addresses to program
- *  @mc_addr_count: number of multicast addresses to program
- *
- *  Updates entire Multicast Table Array.
- *  The caller must have a packed mc_addr_list of multicast addresses.
- **/
-void igb_update_mc_addr_list(struct e1000_hw *hw,
-                             u8 *mc_addr_list, u32 mc_addr_count)
-{
-	u32 hash_value, hash_bit, hash_reg;
-	int i;
-
-	/* clear mta_shadow */
-	memset(&hw->mac.mta_shadow, 0, sizeof(hw->mac.mta_shadow));
-
-	/* update mta_shadow from mc_addr_list */
-	for (i = 0; (u32) i < mc_addr_count; i++) {
-		hash_value = igb_hash_mc_addr(hw, mc_addr_list);
-
-		hash_reg = (hash_value >> 5) & (hw->mac.mta_reg_count - 1);
-		hash_bit = hash_value & 0x1F;
-
-		hw->mac.mta_shadow[hash_reg] |= (1 << hash_bit);
-		mc_addr_list += (ETH_ALEN);
-	}
-
-	/* replace the entire MTA table */
-	for (i = hw->mac.mta_reg_count - 1; i >= 0; i--)
-		array_wr32(E1000_MTA, i, hw->mac.mta_shadow[i]);
-	wrfl();
-}
-
-/**
  *  igb_hash_mc_addr - Generate a multicast hash value
  *  @hw: pointer to the HW structure
  *  @mc_addr: pointer to a multicast address
@@ -329,7 +294,7 @@ void igb_update_mc_addr_list(struct e1000_hw *hw,
  *  the multicast filter table array address and new table value.  See
  *  igb_mta_set()
  **/
-u32 igb_hash_mc_addr(struct e1000_hw *hw, u8 *mc_addr)
+static u32 igb_hash_mc_addr(struct e1000_hw *hw, u8 *mc_addr)
 {
 	u32 hash_value, hash_mask;
 	u8 bit_shift = 0;
@@ -392,6 +357,41 @@ u32 igb_hash_mc_addr(struct e1000_hw *hw, u8 *mc_addr)
 }
 
 /**
+ *  igb_update_mc_addr_list - Update Multicast addresses
+ *  @hw: pointer to the HW structure
+ *  @mc_addr_list: array of multicast addresses to program
+ *  @mc_addr_count: number of multicast addresses to program
+ *
+ *  Updates entire Multicast Table Array.
+ *  The caller must have a packed mc_addr_list of multicast addresses.
+ **/
+void igb_update_mc_addr_list(struct e1000_hw *hw,
+                             u8 *mc_addr_list, u32 mc_addr_count)
+{
+	u32 hash_value, hash_bit, hash_reg;
+	int i;
+
+	/* clear mta_shadow */
+	memset(&hw->mac.mta_shadow, 0, sizeof(hw->mac.mta_shadow));
+
+	/* update mta_shadow from mc_addr_list */
+	for (i = 0; (u32) i < mc_addr_count; i++) {
+		hash_value = igb_hash_mc_addr(hw, mc_addr_list);
+
+		hash_reg = (hash_value >> 5) & (hw->mac.mta_reg_count - 1);
+		hash_bit = hash_value & 0x1F;
+
+		hw->mac.mta_shadow[hash_reg] |= (1 << hash_bit);
+		mc_addr_list += (ETH_ALEN);
+	}
+
+	/* replace the entire MTA table */
+	for (i = hw->mac.mta_reg_count - 1; i >= 0; i--)
+		array_wr32(E1000_MTA, i, hw->mac.mta_shadow[i]);
+	wrfl();
+}
+
+/**
  *  igb_clear_hw_cntrs_base - Clear base hardware counters
  *  @hw: pointer to the HW structure
  *
diff --git a/drivers/net/igb/e1000_mac.h b/drivers/net/igb/e1000_mac.h
index 7518af8..bca17d8 100644
--- a/drivers/net/igb/e1000_mac.h
+++ b/drivers/net/igb/e1000_mac.h
@@ -88,6 +88,5 @@ enum e1000_mng_mode {
 #define E1000_MNG_DHCP_COOKIE_STATUS_VLAN    0x2
 
 extern void e1000_init_function_pointers_82575(struct e1000_hw *hw);
-extern u32 igb_hash_mc_addr(struct e1000_hw *hw, u8 *mc_addr);
 
 #endif


^ permalink raw reply related

* Re: [PATCH] ks8851_ml ethernet network driver
From: David Miller @ 2009-09-17 23:49 UTC (permalink / raw)
  To: David.Choi; +Cc: greg, netdev, Charles.Li, Choi, jgarzik, shemminger
In-Reply-To: <C43529A246480145B0A6D0234BDB0F0DE8A1@MELANITE.micrel.com>

From: "Choi, David" <David.Choi@Micrel.Com>
Date: Thu, 17 Sep 2009 12:30:27 -0700

> --- linux-2.6.31-rc3/drivers/net/ks8851_mll.c.orig	2009-09-17
> 10:18:56.000000000 -0700
> +++ linux-2.6.31-rc3/drivers/net/ks8851_mll.c	2009-09-17
> 10:09:37.000000000 -0700
> @@ -21,8 +21,6 @@
>   * KS8851 16bit MLL chip from Micrel Inc.

I can't use this patch or even test it, as your email client
has corrupted it by, for example, breaking up long lines.

^ permalink raw reply

* Re: [ANNOUNCE] new iptables module match large amount of ip addresses
From: Mikulas Patocka @ 2009-09-17 23:46 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: netfilter-devel, netdev
In-Reply-To: <alpine.LSU.2.00.0909180130110.20781@obet.zrqbmnf.qr>

> >I was saying how OpenBSD is better than Linux because OpenBSD has 
> >tree-based firewall tables --- hmm --- well --- Linux has them too, except 
> >that noone can really find them because they are not in the kernel.
> 
> You can build trees of chains with iptables. (Which would be quite a 
> fast thing if you do not have modules at hand.)

I thought about this too but I realized that building the tree in kernel 
would be easier to write than building it with a shell script :)

Mikulas

^ permalink raw reply

* Re: [ANNOUNCE] new iptables module match large amount of ip addresses
From: Jan Engelhardt @ 2009-09-17 23:33 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: netfilter-devel, netdev
In-Reply-To: <Pine.LNX.4.64.0909180054410.21427@artax.karlin.mff.cuni.cz>

On Friday 2009-09-18 01:01, Mikulas Patocka wrote:
>> On Thursday 2009-09-17 21:15, Mikulas Patocka wrote:
>> >
>> >Here I submit an iptables module that can match large amounts (millions) 
>> >of ip addresses efficiently using binary search.
>> 
>> So you just reinvented xt_geoip...
>
>I am wondering, if there are two approaches for matching large amounts of 
>addresses (xt_geoip and ipset), why is none of them in the kernel?

Because, so I would estimate, Patrick would decline patches with the 
reasoning of redundant code. Especially so "IPMARK".

>I was saying how OpenBSD is better than Linux because OpenBSD has 
>tree-based firewall tables --- hmm --- well --- Linux has them too, except 
>that noone can really find them because they are not in the kernel.

You can build trees of chains with iptables. (Which would be quite a 
fast thing if you do not have modules at hand.)


^ permalink raw reply

* Re: [ANNOUNCE] new iptables module match large amount of ip addresses
From: Mikulas Patocka @ 2009-09-17 23:01 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: netfilter-devel, netdev
In-Reply-To: <alpine.LSU.2.00.0909180046410.20781@obet.zrqbmnf.qr>

On Fri, 18 Sep 2009, Jan Engelhardt wrote:

> On Thursday 2009-09-17 21:15, Mikulas Patocka wrote:
> >
> >Here I submit an iptables module that can match large amounts (millions) 
> >of ip addresses efficiently using binary search.
> 
> So you just reinvented xt_geoip...

I am wondering, if there are two approaches for matching large amounts of 
addresses (xt_geoip and ipset), why is none of them in the kernel?

I was saying how OpenBSD is better than Linux because OpenBSD has 
tree-based firewall tables --- hmm --- well --- Linux has them too, except 
that noone can really find them because they are not in the kernel.

> >- fast matching of large amount of ip addresses using binary search.
> >- an ability to match ranges of addresses or address/mask subnets.
> >- fast loading of the addresses (on Pentium 3 850, 2 million addresses 
> >load in 5.5s, if they are already sorted in the file, the load time is 
> >just 1.5s).
> >- memory efficient --- consumes only 8 bytes per address.
> 
> xt_geoip uses less than that -- 8 bytes per range. Of course it depends 
> on the data, but on the average, since large netblocks is used, it's 
> much better than 8 per address.

My code uses 8 bytes per range too, not really per address.

Mikulas

^ permalink raw reply

* Re: [ANNOUNCE] new iptables module match large amount of ip addresses
From: Jan Engelhardt @ 2009-09-17 22:50 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: netfilter-devel, netdev
In-Reply-To: <Pine.LNX.4.64.0909172100220.27299@artax.karlin.mff.cuni.cz>


On Thursday 2009-09-17 21:15, Mikulas Patocka wrote:
>
>Here I submit an iptables module that can match large amounts (millions) 
>of ip addresses efficiently using binary search.

So you just reinvented xt_geoip...

>- fast matching of large amount of ip addresses using binary search.
>- an ability to match ranges of addresses or address/mask subnets.
>- fast loading of the addresses (on Pentium 3 850, 2 million addresses 
>load in 5.5s, if they are already sorted in the file, the load time is 
>just 1.5s).
>- memory efficient --- consumes only 8 bytes per address.


xt_geoip uses less than that -- 8 bytes per range. Of course it depends 
on the data, but on the average, since large netblocks is used, it's 
much better than 8 per address.

^ permalink raw reply

* Re: Netlink API for bonding ?
From: Nicolas de Pesloüan @ 2009-09-17 22:10 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Jay Vosburgh, bonding-devel, netdev, Jiri Pirko
In-Reply-To: <20090917145120.5a3bb04b@nehalam>

Stephen Hemminger wrote:
> On Thu, 17 Sep 2009 23:44:30 +0200
> Nicolas de Pesloüan <nicolas.2p.debian@free.fr> wrote:
> 
>> Stephen Hemminger wrote:
>>> On Mon, 31 Aug 2009 22:34:50 +0200
>>> Nicolas de Pesloüan <nicolas.2p.debian@free.fr> wrote:
>>>
>>>> Stephen,
>>>>
>>>> Can you please describe the netlink API you plan to implement for bonding ?
>>>>
>>>> Both Jiri Pirko and I plan to add some advanced active slave selection rules, 
>>>> for more-than-two-slaves bonding configuration.
>>>>
>>>> Jay suggested that such advanced features be implemented in user space, using 
>>>> netlink to notify a daemon when slaves come up or fall down. I agree with Jay, 
>>>> but don't want to design something without having first a view at your proposed 
>>>> API for bonding.
>>>>
>>>> Do you plan to have some notification to user space, or only the ability to read 
>>>> and set bonding configuration using netlink ?
>>>>
>>>> Thanks,
>>>>
>>>> 	Nicolas.
>>> No paper spec, but was looking to add interface similar to vlan and macvlan.
>>> Just use (and extend if needed) existing rtnl_link_ops.
>>>
>>>
>>> Was not planning on adding a notification interface, thats good idea but
>>> really not what I was looking at.
>> What kind of notification system would you suggest to notify userland that a 
>> given bond device just lose the current active slave ?
> 
> First why should user land care?  Unless all slaves are gone maybe it
> should just be transparent.

Because we try to design a notification from kernel to userland when current 
active slave fail, to give an opportunity to userland to decide which non-failed 
slave should become the new active one. This is in order to try and move complex 
decisions to userland, only keeping very simple "two slaves" decisions into the 
kernel.

Think of it as the bonding counter part of moving STP to userland for bridge. 
Userland should be able to decide which slave should be the active one for the 
same reasons userland is able to decide which bridge port should be forwarding 
and which should be blocked.

I assume that we cannot just try to make the current bridge userland 
notification system more generic. May be I'm wrong. May be the ability to notify 
port failure, port coming back and BPDU for bridge is a superset of what we need 
to notify port failure and port coming back for bonding.

> Use existing link ops mechanism (see vlan and macvlan). You may need
> to add new operations, but these should be generic enough so that bonding and bridging
> have same operations. 
> 
>      .newlink => create bond device
>      .dellink => remove bond device
>      .newport => add slave
>      .delport => remove slave
> 
> Also, dellink should always work (even if slaves are present).

This sounds perfect for setup, but might not be good the elect the "best" port 
(active slave). Also, I assume a new RTNETLINK operation needs to be added for 
that. I thought that this was what you were working on. Do I miss something ? 
Does brctl use RTNETLINK for port setup ? Or do you plan to use iproute2 to 
replace brctl in the futur ?

> The terminology slave is not widely used outside of bonding, and so probably
> shouldn't be buried in the API.

Yes, you are definitely right with this point.

	Nicolas.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox