* [PATCH 28/28] batman-adv: Disallow regular interface as mesh device
From: Sven Eckelmann @ 2011-03-05 12:28 UTC (permalink / raw)
To: davem; +Cc: netdev, b.a.t.m.a.n, Sven Eckelmann, Marek Lindner
In-Reply-To: <1299328122-21468-1-git-send-email-sven@narfation.org>
When trying to associate a net_device with another net_device which
already exists, batman-adv assumes that this interface is a fully
initialized batman mesh interface without checking it. The behaviour
when accessing data behind netdev_priv of a random net_device is
undefined and potentially dangerous.
Reported-by: Linus Lüssing <linus.luessing@ascom.ch>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
net/batman-adv/hard-interface.c | 34 ++++++++++++++++++++++------------
net/batman-adv/soft-interface.c | 13 +++++++++++++
net/batman-adv/soft-interface.h | 1 +
3 files changed, 36 insertions(+), 12 deletions(-)
diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
index 95a35b6..b3058e4 100644
--- a/net/batman-adv/hard-interface.c
+++ b/net/batman-adv/hard-interface.c
@@ -79,13 +79,8 @@ static int is_valid_iface(struct net_device *net_dev)
return 0;
/* no batman over batman */
-#ifdef HAVE_NET_DEVICE_OPS
- if (net_dev->netdev_ops->ndo_start_xmit == interface_tx)
+ if (softif_is_valid(net_dev))
return 0;
-#else
- if (net_dev->hard_start_xmit == interface_tx)
- return 0;
-#endif
/* Device is being bridged */
/* if (net_dev->priv_flags & IFF_BRIDGE_PORT)
@@ -282,6 +277,8 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char *iface_name)
{
struct bat_priv *bat_priv;
struct batman_packet *batman_packet;
+ struct net_device *soft_iface;
+ int ret;
if (hard_iface->if_status != IF_NOT_IN_USE)
goto out;
@@ -289,18 +286,30 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char *iface_name)
if (!atomic_inc_not_zero(&hard_iface->refcount))
goto out;
- hard_iface->soft_iface = dev_get_by_name(&init_net, iface_name);
+ soft_iface = dev_get_by_name(&init_net, iface_name);
- if (!hard_iface->soft_iface) {
- hard_iface->soft_iface = softif_create(iface_name);
+ if (!soft_iface) {
+ soft_iface = softif_create(iface_name);
- if (!hard_iface->soft_iface)
+ if (!soft_iface) {
+ ret = -ENOMEM;
goto err;
+ }
/* dev_get_by_name() increases the reference counter for us */
- dev_hold(hard_iface->soft_iface);
+ dev_hold(soft_iface);
+ }
+
+ if (!softif_is_valid(soft_iface)) {
+ pr_err("Can't create batman mesh interface %s: "
+ "already exists as regular interface\n",
+ soft_iface->name);
+ dev_put(soft_iface);
+ ret = -EINVAL;
+ goto err;
}
+ hard_iface->soft_iface = soft_iface;
bat_priv = netdev_priv(hard_iface->soft_iface);
hard_iface->packet_len = BAT_PACKET_LEN;
hard_iface->packet_buff = kmalloc(hard_iface->packet_len, GFP_ATOMIC);
@@ -308,6 +317,7 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char *iface_name)
if (!hard_iface->packet_buff) {
bat_err(hard_iface->soft_iface, "Can't add interface packet "
"(%s): out of memory\n", hard_iface->net_dev->name);
+ ret = -ENOMEM;
goto err;
}
@@ -370,7 +380,7 @@ out:
err:
hardif_free_ref(hard_iface);
- return -ENOMEM;
+ return ret;
}
void hardif_disable_interface(struct hard_iface *hard_iface)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 6b514ec..9ed2614 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -622,6 +622,19 @@ void softif_destroy(struct net_device *soft_iface)
unregister_netdevice(soft_iface);
}
+int softif_is_valid(struct net_device *net_dev)
+{
+#ifdef HAVE_NET_DEVICE_OPS
+ if (net_dev->netdev_ops->ndo_start_xmit == interface_tx)
+ return 1;
+#else
+ if (net_dev->hard_start_xmit == interface_tx)
+ return 1;
+#endif
+
+ return 0;
+}
+
/* ethtool */
static int bat_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
{
diff --git a/net/batman-adv/soft-interface.h b/net/batman-adv/soft-interface.h
index 80a3607..4789b6f 100644
--- a/net/batman-adv/soft-interface.h
+++ b/net/batman-adv/soft-interface.h
@@ -31,5 +31,6 @@ void interface_rx(struct net_device *soft_iface,
int hdr_size);
struct net_device *softif_create(char *name);
void softif_destroy(struct net_device *soft_iface);
+int softif_is_valid(struct net_device *net_dev);
#endif /* _NET_BATMAN_ADV_SOFT_INTERFACE_H_ */
--
1.7.2.3
^ permalink raw reply related
* Re: [PATCH] net: mac80211: fix compilation warning
From: Ben Hutchings @ 2011-03-05 12:31 UTC (permalink / raw)
To: bookjovi
Cc: John W. Linville, Johannes Berg, David S. Miller,
open list:NETWORKING [WIREL..., open list:NETWORKING [GENERAL],
open list
In-Reply-To: <1299108754-5492-1-git-send-email-bookjovi@gmail.com>
On Wed, 2011-03-02 at 18:32 -0500, bookjovi@gmail.com wrote:
> From: Jovi Zhang <bookjovi@gmail.com>
>
> this commit fix compilation warning as following:
> net/mac80211/tx.c:1753: warning: unused variable mppath
[...]
You clearly didn't try building this with CONFIG_MAC80211_MESH enabled.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [patch net-next-2.6 7/8] net: introduce rx_handler results and logic around that
From: Ben Hutchings @ 2011-03-05 12:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet,
nicolas.2p.debian, andy
In-Reply-To: <1299320969-7951-8-git-send-email-jpirko@redhat.com>
On Sat, 2011-03-05 at 11:29 +0100, Jiri Pirko wrote:
> This patch allows rx_handlers to better signalize what to do next to
> it's caller. That makes skb->deliver_no_wcard no longer needed.
[...]
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -390,7 +390,14 @@ enum gro_result {
> };
> typedef enum gro_result gro_result_t;
>
> -typedef struct sk_buff *rx_handler_func_t(struct sk_buff *skb);
> +enum rx_handler_result {
> + RX_HANDLER_CONSUMED,
> + RX_HANDLER_ANOTHER,
> + RX_HANDLER_EXACT,
> + RX_HANDLER_PASS,
> +};
[...]
This should have a comment (preferably kernel-doc) clearly specifying
the meaning of each code, as the differences between ANOTHER/EXACT/PASS
are fairly subtle.
Ben,
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: bonding can't change to another slave if you ifdown the active slave
From: Nicolas de Pesloüan @ 2011-03-05 13:49 UTC (permalink / raw)
To: Weiping Pan; +Cc: Andy Gospodarek, netdev, bonding-devel, Linda Wang
In-Reply-To: <20110305025332.GR11864@gospo.rdu.redhat.com>
Le 05/03/2011 03:53, Andy Gospodarek a écrit :
> On Fri, Mar 04, 2011 at 10:15:17AM +0800, Weiping Pan wrote:
>> Hi,
>>
>> I'm doing some Linux bonding driver test, and I find a problem in
>> balance-rr mode.
>> That's it can't change to another slave if you ifdown the active slave.
>> Any comments are warmly welcomed!
>>
>> regards
>> Weiping Pan
>>
>> My host is Fedora 14, and I install VirtualBox (4.0.2), and enable 4
>> nics for the guest system.
>
> Does this mean you are passing 4 NICs from your host to your guest
> (maybe via direct pci-device assignment to the guest) or are you
> creating 4 virtual devices on the host that are in a bridge group on the
> host?
VirtualBox does not allow assignment of pci-device to the guest. The network interfaces on the guest
are pure virtual one, with several modes available. In order to help you trouble shooting this
problem, we need to know the mode form each of the virtual interfaces. Possible modes are NAT,
bridged, internal-network, and host-only-network.
Please provide the output of the following command:
VBoxManage showvminfo <your-vm-uuid> | grep ^NIC
To display your vm uuid, use the following command:
VBoxManage list vms
>
> [...]
>> [root@localhost ~]# ifconfig eth7 down
>
> This is not a great way to test link failure with bonding. The best way
> is to actually pull the cable so the interface is truly down.
To virtually plug or unplug the cable from a virtual interface, use the following command, replacing
the # with the interface number (from 1 to 8):
VBoxManage controlvm setlinkstate# on
VBoxManage controlvm setlinkstate# off
Nicolas.
^ permalink raw reply
* Re: [PATCH] net: mac80211: fix compilation warning
From: Jovi Zhang @ 2011-03-05 13:52 UTC (permalink / raw)
To: Ben Hutchings
Cc: John W. Linville, Johannes Berg, David S. Miller,
open list:NETWORKING [WIREL..., open list:NETWORKING [GENERAL],
open list
In-Reply-To: <1299328260.4277.92.camel@localhost>
On Sat, Mar 5, 2011 at 8:31 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Wed, 2011-03-02 at 18:32 -0500, bookjovi@gmail.com wrote:
>> From: Jovi Zhang <bookjovi@gmail.com>
>>
>> this commit fix compilation warning as following:
>> net/mac80211/tx.c:1753: warning: unused variable mppath
> [...]
>
> You clearly didn't try building this with CONFIG_MAC80211_MESH enabled.
>
Sorry, indeed, maybe should be like this:
+#ifdef CONFIG_MAC80211_MESH
struct mesh_path *mppath = NULL;
+#endif
^ permalink raw reply
* Re: [RFC PATCH] net/core: fix skb handling on netif serves for both bridge and vlan
From: Nicolas de Pesloüan @ 2011-03-05 13:53 UTC (permalink / raw)
To: Jiri Pirko
Cc: Xiaotian Feng, netdev, linux-kernel, David S. Miller,
Eric Dumazet, Tom Herbert
In-Reply-To: <20110305103613.GB7799@psychotron.redhat.com>
Le 05/03/2011 11:36, Jiri Pirko a écrit :
> Thu, Mar 03, 2011 at 11:55:13AM CET, dfeng@redhat.com wrote:
>> Consider network topology as follows:
>>
>> eth0 eth1
>> |_____|
>> |
>> bond0 --- br0
>> |
>> vlan0 --- br1
>>
>> bond0 serves for both br0 and vlan0, if a vlan tagged packet was sent
>> to br1 through bond0, bridge handling code is seeing the packet on bond0
>> and handing it off to my "legacy" bridge before vlan_tx_tag_present
>> and vlan_hwaccel_do_receive even haven't a chance to look at it.
>>
>> Moving the vlan_tx_tag_present before bridge/macvlan handling code could
>> cure this.
>
> Wouldn't this break "eth0 - br0 - br0.5"?
I think it would. One more reason to build a single interface stacking framework...
Nicolas.
^ permalink raw reply
* Re: [patch net-next-2.6 1/8] af_packet: use skb->skb_iif instead of orig_dev->ifindex
From: Nicolas de Pesloüan @ 2011-03-05 14:03 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <1299320969-7951-2-git-send-email-jpirko@redhat.com>
Le 05/03/2011 11:29, Jiri Pirko a écrit :
> Since skb_iif has the desired value (ifindex of physical device actually
> received the traffic) use that instead.
This one cause orig_dev to be useless from the af_packet point of view, which is really good.
>
> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
> ---
> net/packet/af_packet.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 5efef5b..b34294e 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -627,7 +627,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
> sll->sll_protocol = skb->protocol;
> sll->sll_pkttype = skb->pkt_type;
> if (unlikely(po->origdev))
> - sll->sll_ifindex = orig_dev->ifindex;
> + sll->sll_ifindex = skb->skb_iif;
> else
> sll->sll_ifindex = dev->ifindex;
>
> @@ -812,7 +812,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> sll->sll_protocol = skb->protocol;
> sll->sll_pkttype = skb->pkt_type;
> if (unlikely(po->origdev))
> - sll->sll_ifindex = orig_dev->ifindex;
> + sll->sll_ifindex = skb->skb_iif;
> else
> sll->sll_ifindex = dev->ifindex;
>
^ permalink raw reply
* Re: [patch net-next-2.6 2/8] bonding: register slave pointer for rx_handler
From: Nicolas de Pesloüan @ 2011-03-05 14:06 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <1299320969-7951-3-git-send-email-jpirko@redhat.com>
Le 05/03/2011 11:29, Jiri Pirko a écrit :
> Register slave pointer as rx_handler data. That would eventually prevent
> need to loop over slave devices to find the right slave.
>
> Use synchronize_net to ensure that bond_handle_frame does not get slave
> structure freed when working with that.
>
> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
> ---
> drivers/net/bonding/bond_main.c | 17 +++++++++++------
> drivers/net/bonding/bonding.h | 3 +++
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 0592e6d..1c19368 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1495,21 +1495,22 @@ static bool bond_should_deliver_exact_match(struct sk_buff *skb,
>
> static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
> {
> - struct net_device *slave_dev;
> + struct slave *slave;
> struct net_device *bond_dev;
>
> skb = skb_share_check(skb, GFP_ATOMIC);
> if (unlikely(!skb))
> return NULL;
> - slave_dev = skb->dev;
> - bond_dev = ACCESS_ONCE(slave_dev->master);
> +
> + slave = bond_slave_get_rcu(skb->dev);
> + bond_dev = ACCESS_ONCE(slave->dev->master);
> if (unlikely(!bond_dev))
> return skb;
>
> if (bond_dev->priv_flags& IFF_MASTER_ARPMON)
> - slave_dev->last_rx = jiffies;
> + slave->dev->last_rx = jiffies;
>
> - if (bond_should_deliver_exact_match(skb, slave_dev, bond_dev)) {
> + if (bond_should_deliver_exact_match(skb, slave->dev, bond_dev)) {
> skb->deliver_no_wcard = 1;
> return skb;
> }
Up to this point, it looks like cleanup, and unrelated to the title and description of the patch.
Anyway, the cleanup looks good to me. Should just be in a separate patch.
> @@ -1703,7 +1704,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> pr_debug("Error %d calling netdev_set_bond_master\n", res);
> goto err_restore_mac;
> }
> - res = netdev_rx_handler_register(slave_dev, bond_handle_frame, NULL);
> + res = netdev_rx_handler_register(slave_dev, bond_handle_frame,
> + new_slave);
And using rx_handler data for this purpose sounds good to me.
Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
> if (res) {
> pr_debug("Error %d calling netdev_rx_handler_register\n", res);
> goto err_unset_master;
> @@ -1925,6 +1927,7 @@ err_close:
>
> err_unreg_rxhandler:
> netdev_rx_handler_unregister(slave_dev);
> + synchronize_net();
>
> err_unset_master:
> netdev_set_bond_master(slave_dev, NULL);
> @@ -2108,6 +2111,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
> }
>
> netdev_rx_handler_unregister(slave_dev);
> + synchronize_net();
> netdev_set_bond_master(slave_dev, NULL);
>
> slave_disable_netpoll(slave);
> @@ -2222,6 +2226,7 @@ static int bond_release_all(struct net_device *bond_dev)
> }
>
> netdev_rx_handler_unregister(slave_dev);
> + synchronize_net();
> netdev_set_bond_master(slave_dev, NULL);
>
> slave_disable_netpoll(slave);
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index ff4e269..1aac5cd 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -264,6 +264,9 @@ struct bonding {
> #endif /* CONFIG_DEBUG_FS */
> };
>
> +#define bond_slave_get_rcu(dev) \
> + ((struct slave *) rcu_dereference(dev->rx_handler_data))
> +
> /**
> * Returns NULL if the net_device does not belong to any of the bond's slaves
> *
^ permalink raw reply
* Re: [B.A.T.M.A.N.] pull request: batman-adv 2011-03-05
From: Sven Eckelmann @ 2011-03-05 14:13 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: davem, netdev
In-Reply-To: <1299328122-21468-1-git-send-email-sven@narfation.org>
[-- Attachment #1: Type: Text/Plain, Size: 473 bytes --]
On Saturday 05 March 2011 13:28:14 Sven Eckelmann wrote:
> Hi,
>
> I was asked to submit following pull request for net-next-2.6/linux-2.6.39.
> It contains Marek's hard work (and of course related work by other people)
> to remove the orig_hash (aka the batman-adv private "big kernel lock").
Small correction: The orig_hash is still there, but the orig_hash lock was
replaced by finer grained locking and rcu protected datastructures.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [patch net-next-2.6 3/8] net: get rid of multiple bond-related netdevice->priv_flags
From: Nicolas de Pesloüan @ 2011-03-05 14:14 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <1299320969-7951-4-git-send-email-jpirko@redhat.com>
Le 05/03/2011 11:29, Jiri Pirko a écrit :
> Now when bond-related code is moved from net/core/dev.c into bonding
> code, multiple priv_flags are not needed anymore. So toss them out.
Agreed. See at the end for a single comment.
>
> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
> ---
> drivers/net/bonding/bond_main.c | 33 ++++++++++++++-------------------
> drivers/net/bonding/bond_sysfs.c | 8 --------
> drivers/net/bonding/bonding.h | 24 +-----------------------
> include/linux/if.h | 22 +++++++++-------------
> 4 files changed, 24 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1c19368..7923184 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1471,20 +1471,20 @@ static void bond_setup_by_slave(struct net_device *bond_dev,
> * ARP on active-backup slaves with arp_validate enabled.
> */
> static bool bond_should_deliver_exact_match(struct sk_buff *skb,
> - struct net_device *slave_dev,
> - struct net_device *bond_dev)
> + struct slave *slave,
> + struct bonding *bond)
> {
> - if (slave_dev->priv_flags& IFF_SLAVE_INACTIVE) {
> - if (slave_dev->priv_flags& IFF_SLAVE_NEEDARP&&
> + if (slave->dev->priv_flags& IFF_SLAVE_INACTIVE) {
> + if (slave_do_arp_validate(bond, slave)&&
> skb->protocol == __cpu_to_be16(ETH_P_ARP))
> return false;
>
> - if (bond_dev->priv_flags& IFF_MASTER_ALB&&
> + if (bond->params.mode == BOND_MODE_ALB&&
> skb->pkt_type != PACKET_BROADCAST&&
> skb->pkt_type != PACKET_MULTICAST)
> return false;
>
> - if (bond_dev->priv_flags& IFF_MASTER_8023AD&&
> + if (bond->params.mode == BOND_MODE_8023AD&&
> skb->protocol == __cpu_to_be16(ETH_P_SLOW))
> return false;
>
> @@ -1497,6 +1497,7 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
> {
> struct slave *slave;
> struct net_device *bond_dev;
> + struct bonding *bond;
>
> skb = skb_share_check(skb, GFP_ATOMIC);
> if (unlikely(!skb))
> @@ -1507,17 +1508,19 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
> if (unlikely(!bond_dev))
> return skb;
>
> - if (bond_dev->priv_flags& IFF_MASTER_ARPMON)
> + bond = netdev_priv(bond_dev);
> +
> + if (bond->params.arp_interval)
> slave->dev->last_rx = jiffies;
>
> - if (bond_should_deliver_exact_match(skb, slave->dev, bond_dev)) {
> + if (bond_should_deliver_exact_match(skb, slave, bond)) {
> skb->deliver_no_wcard = 1;
> return skb;
> }
>
> skb->dev = bond_dev;
>
> - if (bond_dev->priv_flags& IFF_MASTER_ALB&&
> + if (bond->params.mode == BOND_MODE_ALB&&
> bond_dev->priv_flags& IFF_BRIDGE_PORT&&
> skb->pkt_type == PACKET_HOST) {
> u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
> @@ -2128,9 +2131,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>
> dev_set_mtu(slave_dev, slave->original_mtu);
>
> - slave_dev->priv_flags&= ~(IFF_MASTER_8023AD | IFF_MASTER_ALB |
> - IFF_SLAVE_INACTIVE | IFF_BONDING |
> - IFF_SLAVE_NEEDARP);
> + slave_dev->priv_flags&= ~(IFF_SLAVE_INACTIVE | IFF_BONDING);
>
> kfree(slave);
>
> @@ -2241,8 +2242,7 @@ static int bond_release_all(struct net_device *bond_dev)
> dev_set_mac_address(slave_dev,&addr);
> }
>
> - slave_dev->priv_flags&= ~(IFF_MASTER_8023AD | IFF_MASTER_ALB |
> - IFF_SLAVE_INACTIVE);
> + slave_dev->priv_flags&= ~IFF_SLAVE_INACTIVE;
>
> kfree(slave);
>
> @@ -4718,11 +4718,9 @@ void bond_set_mode_ops(struct bonding *bond, int mode)
> case BOND_MODE_BROADCAST:
> break;
> case BOND_MODE_8023AD:
> - bond_set_master_3ad_flags(bond);
> bond_set_xmit_hash_policy(bond);
> break;
> case BOND_MODE_ALB:
> - bond_set_master_alb_flags(bond);
> /* FALLTHRU */
> case BOND_MODE_TLB:
> break;
> @@ -4813,9 +4811,6 @@ static void bond_setup(struct net_device *bond_dev)
> bond_dev->priv_flags |= IFF_BONDING;
> bond_dev->priv_flags&= ~IFF_XMIT_DST_RELEASE;
>
> - if (bond->params.arp_interval)
> - bond_dev->priv_flags |= IFF_MASTER_ARPMON;
> -
> /* At first, we block adding VLANs. That's the only way to
> * prevent problems that occur when adding VLANs over an
> * empty bond. The block will be removed once non-challenged
> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index 72bb0f6..05e0ae5 100644
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -322,11 +322,6 @@ static ssize_t bonding_store_mode(struct device *d,
> ret = -EINVAL;
> goto out;
> }
> - if (bond->params.mode == BOND_MODE_8023AD)
> - bond_unset_master_3ad_flags(bond);
> -
> - if (bond->params.mode == BOND_MODE_ALB)
> - bond_unset_master_alb_flags(bond);
>
> bond->params.mode = new_value;
> bond_set_mode_ops(bond, bond->params.mode);
> @@ -527,8 +522,6 @@ static ssize_t bonding_store_arp_interval(struct device *d,
> pr_info("%s: Setting ARP monitoring interval to %d.\n",
> bond->dev->name, new_value);
> bond->params.arp_interval = new_value;
> - if (bond->params.arp_interval)
> - bond->dev->priv_flags |= IFF_MASTER_ARPMON;
> if (bond->params.miimon) {
> pr_info("%s: ARP monitoring cannot be used with MII monitoring. %s Disabling MII monitoring.\n",
> bond->dev->name, bond->dev->name);
> @@ -1004,7 +997,6 @@ static ssize_t bonding_store_miimon(struct device *d,
> pr_info("%s: MII monitoring cannot be used with ARP monitoring. Disabling ARP monitoring...\n",
> bond->dev->name);
> bond->params.arp_interval = 0;
> - bond->dev->priv_flags&= ~IFF_MASTER_ARPMON;
> if (bond->params.arp_validate) {
> bond_unregister_arp(bond);
> bond->params.arp_validate =
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index 1aac5cd..ddee62f 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -354,34 +354,12 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave)
> slave->state = BOND_STATE_BACKUP;
> if (!bond->params.all_slaves_active)
> slave->dev->priv_flags |= IFF_SLAVE_INACTIVE;
> - if (slave_do_arp_validate(bond, slave))
> - slave->dev->priv_flags |= IFF_SLAVE_NEEDARP;
> }
>
> static inline void bond_set_slave_active_flags(struct slave *slave)
> {
> slave->state = BOND_STATE_ACTIVE;
> - slave->dev->priv_flags&= ~(IFF_SLAVE_INACTIVE | IFF_SLAVE_NEEDARP);
> -}
> -
> -static inline void bond_set_master_3ad_flags(struct bonding *bond)
> -{
> - bond->dev->priv_flags |= IFF_MASTER_8023AD;
> -}
> -
> -static inline void bond_unset_master_3ad_flags(struct bonding *bond)
> -{
> - bond->dev->priv_flags&= ~IFF_MASTER_8023AD;
> -}
> -
> -static inline void bond_set_master_alb_flags(struct bonding *bond)
> -{
> - bond->dev->priv_flags |= IFF_MASTER_ALB;
> -}
> -
> -static inline void bond_unset_master_alb_flags(struct bonding *bond)
> -{
> - bond->dev->priv_flags&= ~IFF_MASTER_ALB;
> + slave->dev->priv_flags&= ~IFF_SLAVE_INACTIVE;
> }
>
> struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
> diff --git a/include/linux/if.h b/include/linux/if.h
> index 3bc63e6..2fdd47a 100644
> --- a/include/linux/if.h
> +++ b/include/linux/if.h
> @@ -60,21 +60,17 @@
> #define IFF_802_1Q_VLAN 0x1 /* 802.1Q VLAN device. */
> #define IFF_EBRIDGE 0x2 /* Ethernet bridging device. */
> #define IFF_SLAVE_INACTIVE 0x4 /* bonding slave not the curr. active */
> -#define IFF_MASTER_8023AD 0x8 /* bonding master, 802.3ad. */
> -#define IFF_MASTER_ALB 0x10 /* bonding master, balance-alb. */
> -#define IFF_BONDING 0x20 /* bonding master or slave */
> -#define IFF_SLAVE_NEEDARP 0x40 /* need ARPs for validation */
> -#define IFF_ISATAP 0x80 /* ISATAP interface (RFC4214) */
> -#define IFF_MASTER_ARPMON 0x100 /* bonding master, ARP mon in use */
> -#define IFF_WAN_HDLC 0x200 /* WAN HDLC device */
> -#define IFF_XMIT_DST_RELEASE 0x400 /* dev_hard_start_xmit() is allowed to
> +#define IFF_BONDING 0x8 /* bonding master or slave */
> +#define IFF_ISATAP 0x10 /* ISATAP interface (RFC4214) */
> +#define IFF_WAN_HDLC 0x20 /* WAN HDLC device */
> +#define IFF_XMIT_DST_RELEASE 0x40 /* dev_hard_start_xmit() is allowed to
> * release skb->dst
> */
Why did you changed those values? Aren't those exposed to userland? Just removing the unused one
sounds good to me.
> -#define IFF_DONT_BRIDGE 0x800 /* disallow bridging this ether dev */
> -#define IFF_DISABLE_NETPOLL 0x1000 /* disable netpoll at run-time */
> -#define IFF_MACVLAN_PORT 0x2000 /* device used as macvlan port */
> -#define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */
> -#define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch
> +#define IFF_DONT_BRIDGE 0x80 /* disallow bridging this ether dev */
> +#define IFF_DISABLE_NETPOLL 0x100 /* disable netpoll at run-time */
> +#define IFF_MACVLAN_PORT 0x200 /* device used as macvlan port */
> +#define IFF_BRIDGE_PORT 0x400 /* device used as bridge port */
> +#define IFF_OVS_DATAPATH 0x800 /* device used as Open vSwitch
> * datapath port */
>
> #define IF_GET_IFACE 0x0001 /* for querying only */
^ permalink raw reply
* Re: [patch net-next-2.6 5/8] bonding: get rid of IFF_SLAVE_INACTIVE netdev->priv_flag
From: Nicolas de Pesloüan @ 2011-03-05 14:18 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <1299320969-7951-6-git-send-email-jpirko@redhat.com>
Le 05/03/2011 11:29, Jiri Pirko a écrit :
> Since bond-related code was moved from net/core/dev.c into bonding,
> IFF_SLAVE_INACTIVE is no longer needed. Replace is with flag "inactive"
> stored in slave structure
>
> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
> ---
> drivers/net/bonding/bond_main.c | 6 ++----
> drivers/net/bonding/bond_sysfs.c | 4 ++--
> drivers/net/bonding/bonding.h | 14 ++++++++++----
> include/linux/if.h | 19 +++++++++----------
> 4 files changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 62020a7..dbe182c 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1474,7 +1474,7 @@ static bool bond_should_deliver_exact_match(struct sk_buff *skb,
> struct slave *slave,
> struct bonding *bond)
> {
> - if (slave->dev->priv_flags& IFF_SLAVE_INACTIVE) {
> + if (bond_is_slave_inactive(slave)) {
> if (slave_do_arp_validate(bond, slave)&&
> skb->protocol == __cpu_to_be16(ETH_P_ARP))
> return false;
> @@ -2131,7 +2131,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>
> dev_set_mtu(slave_dev, slave->original_mtu);
>
> - slave_dev->priv_flags&= ~(IFF_SLAVE_INACTIVE | IFF_BONDING);
> + slave_dev->priv_flags&= ~IFF_BONDING;
>
> kfree(slave);
>
> @@ -2242,8 +2242,6 @@ static int bond_release_all(struct net_device *bond_dev)
> dev_set_mac_address(slave_dev,&addr);
> }
>
> - slave_dev->priv_flags&= ~IFF_SLAVE_INACTIVE;
> -
> kfree(slave);
>
> /* re-acquire the lock before getting the next slave */
> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index 344d23f..5161183 100644
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -1581,9 +1581,9 @@ static ssize_t bonding_store_slaves_active(struct device *d,
> bond_for_each_slave(bond, slave, i) {
> if (!bond_is_active_slave(slave)) {
> if (new_value)
> - slave->dev->priv_flags&= ~IFF_SLAVE_INACTIVE;
> + slave->inactive = 0;
> else
> - slave->dev->priv_flags |= IFF_SLAVE_INACTIVE;
> + slave->inactive = 1;
> }
> }
> out:
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index 8a3718b..8f78166 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -190,8 +190,9 @@ struct slave {
> unsigned long last_arp_rx;
> s8 link; /* one of BOND_LINK_XXXX */
> s8 new_link;
> - u8 backup; /* indicates backup slave. Value corresponds with
> - BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
> + u8 backup:1, /* indicates backup slave. Value corresponds with
> + BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
> + inactive:1; /* indicates inactive slave */
> u32 original_mtu;
> u32 link_failure_count;
> u8 perm_hwaddr[ETH_ALEN];
> @@ -374,13 +375,18 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave)
> if (!bond_is_lb(bond))
> bond_set_backup_slave(slave);
> if (!bond->params.all_slaves_active)
> - slave->dev->priv_flags |= IFF_SLAVE_INACTIVE;
> + slave->inactive = 1;
> }
>
> static inline void bond_set_slave_active_flags(struct slave *slave)
> {
> bond_set_active_slave(slave);
> - slave->dev->priv_flags&= ~IFF_SLAVE_INACTIVE;
> + slave->inactive = 0;
> +}
> +
> +static inline bool bond_is_slave_inactive(struct slave *slave)
> +{
> + return slave->inactive;
> }
>
> struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
> diff --git a/include/linux/if.h b/include/linux/if.h
> index 2fdd47a..0da7991 100644
> --- a/include/linux/if.h
> +++ b/include/linux/if.h
> @@ -59,18 +59,17 @@
> /* Private (from user) interface flags (netdevice->priv_flags). */
> #define IFF_802_1Q_VLAN 0x1 /* 802.1Q VLAN device. */
> #define IFF_EBRIDGE 0x2 /* Ethernet bridging device. */
> -#define IFF_SLAVE_INACTIVE 0x4 /* bonding slave not the curr. active */
> -#define IFF_BONDING 0x8 /* bonding master or slave */
> -#define IFF_ISATAP 0x10 /* ISATAP interface (RFC4214) */
> -#define IFF_WAN_HDLC 0x20 /* WAN HDLC device */
> -#define IFF_XMIT_DST_RELEASE 0x40 /* dev_hard_start_xmit() is allowed to
> +#define IFF_BONDING 0x4 /* bonding master or slave */
> +#define IFF_ISATAP 0x8 /* ISATAP interface (RFC4214) */
> +#define IFF_WAN_HDLC 0x10 /* WAN HDLC device */
> +#define IFF_XMIT_DST_RELEASE 0x20 /* dev_hard_start_xmit() is allowed to
> * release skb->dst
> */
> -#define IFF_DONT_BRIDGE 0x80 /* disallow bridging this ether dev */
> -#define IFF_DISABLE_NETPOLL 0x100 /* disable netpoll at run-time */
> -#define IFF_MACVLAN_PORT 0x200 /* device used as macvlan port */
> -#define IFF_BRIDGE_PORT 0x400 /* device used as bridge port */
> -#define IFF_OVS_DATAPATH 0x800 /* device used as Open vSwitch
> +#define IFF_DONT_BRIDGE 0x40 /* disallow bridging this ether dev */
> +#define IFF_DISABLE_NETPOLL 0x80 /* disable netpoll at run-time */
> +#define IFF_MACVLAN_PORT 0x100 /* device used as macvlan port */
> +#define IFF_BRIDGE_PORT 0x200 /* device used as bridge port */
> +#define IFF_OVS_DATAPATH 0x400 /* device used as Open vSwitch
> * datapath port */
>
> #define IF_GET_IFACE 0x0001 /* for querying only */
^ permalink raw reply
* Re: [RFC PATCH] net/core: fix skb handling on netif serves for both bridge and vlan
From: Jiri Pirko @ 2011-03-05 14:25 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: Xiaotian Feng, netdev, linux-kernel, David S. Miller,
Eric Dumazet, Tom Herbert
In-Reply-To: <4D72406B.6090504@gmail.com>
Sat, Mar 05, 2011 at 02:53:47PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 05/03/2011 11:36, Jiri Pirko a écrit :
>>Thu, Mar 03, 2011 at 11:55:13AM CET, dfeng@redhat.com wrote:
>>>Consider network topology as follows:
>>>
>>>eth0 eth1
>>>|_____|
>>> |
>>> bond0 --- br0
>>> |
>>> vlan0 --- br1
>>>
>>>bond0 serves for both br0 and vlan0, if a vlan tagged packet was sent
>>>to br1 through bond0, bridge handling code is seeing the packet on bond0
>>>and handing it off to my "legacy" bridge before vlan_tx_tag_present
>>>and vlan_hwaccel_do_receive even haven't a chance to look at it.
>>>
>>>Moving the vlan_tx_tag_present before bridge/macvlan handling code could
>>>cure this.
>>
>>Wouldn't this break "eth0 - br0 - br0.5"?
>
>I think it would. One more reason to build a single interface stacking framework...
I plan to deal with vlans later. This kind of topology would not be
possible after that :)
>
> Nicolas.
^ permalink raw reply
* Re: [patch net-next-2.6 2/8] bonding: register slave pointer for rx_handler
From: Jiri Pirko @ 2011-03-05 14:27 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <4D724382.7010705@gmail.com>
Sat, Mar 05, 2011 at 03:06:58PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 05/03/2011 11:29, Jiri Pirko a écrit :
>>Register slave pointer as rx_handler data. That would eventually prevent
>>need to loop over slave devices to find the right slave.
>>
>>Use synchronize_net to ensure that bond_handle_frame does not get slave
>>structure freed when working with that.
>>
>>Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>---
>> drivers/net/bonding/bond_main.c | 17 +++++++++++------
>> drivers/net/bonding/bonding.h | 3 +++
>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 0592e6d..1c19368 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1495,21 +1495,22 @@ static bool bond_should_deliver_exact_match(struct sk_buff *skb,
>>
>> static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
>> {
>>- struct net_device *slave_dev;
>>+ struct slave *slave;
>> struct net_device *bond_dev;
>>
>> skb = skb_share_check(skb, GFP_ATOMIC);
>> if (unlikely(!skb))
>> return NULL;
>>- slave_dev = skb->dev;
>>- bond_dev = ACCESS_ONCE(slave_dev->master);
>>+
>>+ slave = bond_slave_get_rcu(skb->dev);
>>+ bond_dev = ACCESS_ONCE(slave->dev->master);
>> if (unlikely(!bond_dev))
>> return skb;
>>
>> if (bond_dev->priv_flags& IFF_MASTER_ARPMON)
>>- slave_dev->last_rx = jiffies;
>>+ slave->dev->last_rx = jiffies;
>>
>>- if (bond_should_deliver_exact_match(skb, slave_dev, bond_dev)) {
>>+ if (bond_should_deliver_exact_match(skb, slave->dev, bond_dev)) {
>> skb->deliver_no_wcard = 1;
>> return skb;
>> }
>
>Up to this point, it looks like cleanup, and unrelated to the title and description of the patch.
Nope - these are changes connected to the ability to get slave struct
via bond_slave_get_rcu. Very related.
>
>Anyway, the cleanup looks good to me. Should just be in a separate patch.
>
>>@@ -1703,7 +1704,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> pr_debug("Error %d calling netdev_set_bond_master\n", res);
>> goto err_restore_mac;
>> }
>>- res = netdev_rx_handler_register(slave_dev, bond_handle_frame, NULL);
>>+ res = netdev_rx_handler_register(slave_dev, bond_handle_frame,
>>+ new_slave);
>
>And using rx_handler data for this purpose sounds good to me.
>
>Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
>
>> if (res) {
>> pr_debug("Error %d calling netdev_rx_handler_register\n", res);
>> goto err_unset_master;
>>@@ -1925,6 +1927,7 @@ err_close:
>>
>> err_unreg_rxhandler:
>> netdev_rx_handler_unregister(slave_dev);
>>+ synchronize_net();
>>
>> err_unset_master:
>> netdev_set_bond_master(slave_dev, NULL);
>>@@ -2108,6 +2111,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>> }
>>
>> netdev_rx_handler_unregister(slave_dev);
>>+ synchronize_net();
>> netdev_set_bond_master(slave_dev, NULL);
>>
>> slave_disable_netpoll(slave);
>>@@ -2222,6 +2226,7 @@ static int bond_release_all(struct net_device *bond_dev)
>> }
>>
>> netdev_rx_handler_unregister(slave_dev);
>>+ synchronize_net();
>> netdev_set_bond_master(slave_dev, NULL);
>>
>> slave_disable_netpoll(slave);
>>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>index ff4e269..1aac5cd 100644
>>--- a/drivers/net/bonding/bonding.h
>>+++ b/drivers/net/bonding/bonding.h
>>@@ -264,6 +264,9 @@ struct bonding {
>> #endif /* CONFIG_DEBUG_FS */
>> };
>>
>>+#define bond_slave_get_rcu(dev) \
>>+ ((struct slave *) rcu_dereference(dev->rx_handler_data))
>>+
>> /**
>> * Returns NULL if the net_device does not belong to any of the bond's slaves
>> *
>
^ permalink raw reply
* Re: [patch net-next-2.6 6/8] bonding: move processing of recv handlers into handle_frame()
From: Nicolas de Pesloüan @ 2011-03-05 14:33 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <1299320969-7951-7-git-send-email-jpirko@redhat.com>
Le 05/03/2011 11:29, Jiri Pirko a écrit :
> Since now when bonding uses rx_handler, all traffic going into bond
> device goes thru bond_handle_frame. So there's no need to go back into
> bonding code later via ptype handlers. This patch converts
> original ptype handlers into "bonding receive probes". These functions
> are called from bond_handle_frame and they are registered per-mode.
Does this still support having the arp_ip_target on a vlan?
(eth0 -> bond0 -> bond0.100, with arp_ip_target only reachable through bond0.100).
>
> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
> ---
> drivers/net/bonding/bond_3ad.c | 29 ++--------
> drivers/net/bonding/bond_3ad.h | 4 +-
> drivers/net/bonding/bond_alb.c | 46 ++++-----------
> drivers/net/bonding/bond_alb.h | 1 -
> drivers/net/bonding/bond_main.c | 121 +++++++------------------------------
> drivers/net/bonding/bond_sysfs.c | 6 --
> drivers/net/bonding/bonding.h | 4 +-
> 7 files changed, 43 insertions(+), 168 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 047af0b..194aaf7 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -2461,35 +2461,16 @@ out:
> return NETDEV_TX_OK;
> }
>
> -int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type* ptype, struct net_device *orig_dev)
> +void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
> + struct slave *slave)
> {
> - struct bonding *bond = netdev_priv(dev);
> - struct slave *slave = NULL;
> - int ret = NET_RX_DROP;
> -
> - if (!(dev->flags& IFF_MASTER))
> - goto out;
> -
> - skb = skb_share_check(skb, GFP_ATOMIC);
> - if (!skb)
> - goto out;
> + if (skb->protocol != PKT_TYPE_LACPDU)
> + return;
>
> if (!pskb_may_pull(skb, sizeof(struct lacpdu)))
> - goto out;
> + return;
>
> read_lock(&bond->lock);
> - slave = bond_get_slave_by_dev(netdev_priv(dev), orig_dev);
> - if (!slave)
> - goto out_unlock;
> -
> bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
> -
> - ret = NET_RX_SUCCESS;
> -
> -out_unlock:
> read_unlock(&bond->lock);
> -out:
> - dev_kfree_skb(skb);
> -
> - return ret;
> }
> diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
> index 2c46a154..56a88ff 100644
> --- a/drivers/net/bonding/bond_3ad.h
> +++ b/drivers/net/bonding/bond_3ad.h
> @@ -258,7 +258,6 @@ struct ad_bond_info {
> * requested
> */
> struct timer_list ad_timer;
> - struct packet_type ad_pkt_type;
> };
>
> struct ad_slave_info {
> @@ -279,7 +278,8 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave);
> void bond_3ad_handle_link_change(struct slave *slave, char link);
> int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
> -int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type* ptype, struct net_device *orig_dev);
> +void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
> + struct slave *slave);
> int bond_3ad_set_carrier(struct bonding *bond);
> #endif //__BOND_3AD_H__
>
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 9bc5de3..96d28f9 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -308,49 +308,33 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
> _unlock_rx_hashtbl(bond);
> }
>
> -static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct packet_type *ptype, struct net_device *orig_dev)
> +static void rlb_arp_recv(struct sk_buff *skb, struct bonding *bond,
> + struct slave *slave)
> {
> - struct bonding *bond;
> - struct arp_pkt *arp = (struct arp_pkt *)skb->data;
> - int res = NET_RX_DROP;
> + struct arp_pkt *arp;
>
> - while (bond_dev->priv_flags& IFF_802_1Q_VLAN)
> - bond_dev = vlan_dev_real_dev(bond_dev);
> -
> - if (!(bond_dev->priv_flags& IFF_BONDING) ||
> - !(bond_dev->flags& IFF_MASTER))
> - goto out;
> + if (skb->protocol != cpu_to_be16(ETH_P_ARP))
> + return;
>
> + arp = (struct arp_pkt *) skb->data;
> if (!arp) {
> pr_debug("Packet has no ARP data\n");
> - goto out;
> + return;
> }
>
> - skb = skb_share_check(skb, GFP_ATOMIC);
> - if (!skb)
> - goto out;
> -
> - if (!pskb_may_pull(skb, arp_hdr_len(bond_dev)))
> - goto out;
> + if (!pskb_may_pull(skb, arp_hdr_len(bond->dev)))
> + return;
>
> if (skb->len< sizeof(struct arp_pkt)) {
> pr_debug("Packet is too small to be an ARP\n");
> - goto out;
> + return;
> }
>
> if (arp->op_code == htons(ARPOP_REPLY)) {
> /* update rx hash table for this ARP */
> - bond = netdev_priv(bond_dev);
> rlb_update_entry_from_arp(bond, arp);
> pr_debug("Server received an ARP Reply from client\n");
> }
> -
> - res = NET_RX_SUCCESS;
> -
> -out:
> - dev_kfree_skb(skb);
> -
> - return res;
> }
>
> /* Caller must hold bond lock for read */
> @@ -759,7 +743,6 @@ static void rlb_init_table_entry(struct rlb_client_info *entry)
> static int rlb_initialize(struct bonding *bond)
> {
> struct alb_bond_info *bond_info =&(BOND_ALB_INFO(bond));
> - struct packet_type *pk_type =&(BOND_ALB_INFO(bond).rlb_pkt_type);
> struct rlb_client_info *new_hashtbl;
> int size = RLB_HASH_TABLE_SIZE * sizeof(struct rlb_client_info);
> int i;
> @@ -784,13 +767,8 @@ static int rlb_initialize(struct bonding *bond)
>
> _unlock_rx_hashtbl(bond);
>
> - /*initialize packet type*/
> - pk_type->type = cpu_to_be16(ETH_P_ARP);
> - pk_type->dev = bond->dev;
> - pk_type->func = rlb_arp_recv;
> -
> /* register to receive ARPs */
> - dev_add_pack(pk_type);
> + bond->recv_probe = rlb_arp_recv;
>
> return 0;
> }
> @@ -799,8 +777,6 @@ static void rlb_deinitialize(struct bonding *bond)
> {
> struct alb_bond_info *bond_info =&(BOND_ALB_INFO(bond));
>
> - dev_remove_pack(&(bond_info->rlb_pkt_type));
> -
> _lock_rx_hashtbl(bond);
>
> kfree(bond_info->rx_hashtbl);
> diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
> index 118c28a..6605e9e 100644
> --- a/drivers/net/bonding/bond_alb.h
> +++ b/drivers/net/bonding/bond_alb.h
> @@ -130,7 +130,6 @@ struct alb_bond_info {
> int lp_counter;
> /* -------- rlb parameters -------- */
> int rlb_enabled;
> - struct packet_type rlb_pkt_type;
> struct rlb_client_info *rx_hashtbl; /* Receive hash table */
> spinlock_t rx_hashtbl_lock;
> u32 rx_hashtbl_head;
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index dbe182c..8ba7faa 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1467,27 +1467,17 @@ static void bond_setup_by_slave(struct net_device *bond_dev,
> }
>
> /* On bonding slaves other than the currently active slave, suppress
> - * duplicates except for 802.3ad ETH_P_SLOW, alb non-mcast/bcast, and
> - * ARP on active-backup slaves with arp_validate enabled.
> + * duplicates except for alb non-mcast/bcast.
> */
> static bool bond_should_deliver_exact_match(struct sk_buff *skb,
> struct slave *slave,
> struct bonding *bond)
> {
> if (bond_is_slave_inactive(slave)) {
> - if (slave_do_arp_validate(bond, slave)&&
> - skb->protocol == __cpu_to_be16(ETH_P_ARP))
> - return false;
> -
> if (bond->params.mode == BOND_MODE_ALB&&
> skb->pkt_type != PACKET_BROADCAST&&
> skb->pkt_type != PACKET_MULTICAST)
> - return false;
> -
> - if (bond->params.mode == BOND_MODE_8023AD&&
> - skb->protocol == __cpu_to_be16(ETH_P_SLOW))
> return false;
> -
> return true;
> }
> return false;
> @@ -1513,6 +1503,15 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
> if (bond->params.arp_interval)
> slave->dev->last_rx = jiffies;
>
> + if (bond->recv_probe) {
> + struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
> +
> + if (likely(nskb)) {
> + bond->recv_probe(nskb, bond, slave);
> + dev_kfree_skb(nskb);
> + }
> + }
> +
> if (bond_should_deliver_exact_match(skb, slave, bond)) {
> skb->deliver_no_wcard = 1;
> return skb;
> @@ -2813,48 +2812,26 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
> }
> }
>
> -static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt, struct net_device *orig_dev)
> +static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
> + struct slave *slave)
> {
> struct arphdr *arp;
> - struct slave *slave;
> - struct bonding *bond;
> unsigned char *arp_ptr;
> __be32 sip, tip;
>
> - if (dev->priv_flags& IFF_802_1Q_VLAN) {
> - /*
> - * When using VLANS and bonding, dev and oriv_dev may be
> - * incorrect if the physical interface supports VLAN
> - * acceleration. With this change ARP validation now
> - * works for hosts only reachable on the VLAN interface.
> - */
> - dev = vlan_dev_real_dev(dev);
> - orig_dev = dev_get_by_index_rcu(dev_net(skb->dev),skb->skb_iif);
> - }
> -
> - if (!(dev->priv_flags& IFF_BONDING) || !(dev->flags& IFF_MASTER))
> - goto out;
> + if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
> + return;
What happens if the skb that hold the ARP request/reply is vlan tagged? The ARP ptype_base handler
for bonding used to be called after vlan processing.
> - bond = netdev_priv(dev);
> read_lock(&bond->lock);
>
> - pr_debug("bond_arp_rcv: bond %s skb->dev %s orig_dev %s\n",
> - bond->dev->name, skb->dev ? skb->dev->name : "NULL",
> - orig_dev ? orig_dev->name : "NULL");
> + pr_debug("bond_arp_rcv: bond %s skb->dev %s\n",
> + bond->dev->name, skb->dev->name);
>
> - slave = bond_get_slave_by_dev(bond, orig_dev);
> - if (!slave || !slave_do_arp_validate(bond, slave))
> - goto out_unlock;
> -
> - skb = skb_share_check(skb, GFP_ATOMIC);
> - if (!skb)
> - goto out_unlock;
> -
> - if (!pskb_may_pull(skb, arp_hdr_len(dev)))
> + if (!pskb_may_pull(skb, arp_hdr_len(bond->dev)))
> goto out_unlock;
>
> arp = arp_hdr(skb);
> - if (arp->ar_hln != dev->addr_len ||
> + if (arp->ar_hln != bond->dev->addr_len ||
> skb->pkt_type == PACKET_OTHERHOST ||
> skb->pkt_type == PACKET_LOOPBACK ||
> arp->ar_hrd != htons(ARPHRD_ETHER) ||
> @@ -2863,9 +2840,9 @@ static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct pack
> goto out_unlock;
>
> arp_ptr = (unsigned char *)(arp + 1);
> - arp_ptr += dev->addr_len;
> + arp_ptr += bond->dev->addr_len;
> memcpy(&sip, arp_ptr, 4);
> - arp_ptr += 4 + dev->addr_len;
> + arp_ptr += 4 + bond->dev->addr_len;
> memcpy(&tip, arp_ptr, 4);
>
> pr_debug("bond_arp_rcv: %s %s/%d av %d sv %d sip %pI4 tip %pI4\n",
> @@ -2888,9 +2865,6 @@ static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct pack
>
> out_unlock:
> read_unlock(&bond->lock);
> -out:
> - dev_kfree_skb(skb);
> - return NET_RX_SUCCESS;
> }
>
> /*
> @@ -3782,48 +3756,6 @@ static struct notifier_block bond_inetaddr_notifier = {
> .notifier_call = bond_inetaddr_event,
> };
>
> -/*-------------------------- Packet type handling ---------------------------*/
> -
> -/* register to receive lacpdus on a bond */
> -static void bond_register_lacpdu(struct bonding *bond)
> -{
> - struct packet_type *pk_type =&(BOND_AD_INFO(bond).ad_pkt_type);
> -
> - /* initialize packet type */
> - pk_type->type = PKT_TYPE_LACPDU;
> - pk_type->dev = bond->dev;
> - pk_type->func = bond_3ad_lacpdu_recv;
> -
> - dev_add_pack(pk_type);
> -}
> -
> -/* unregister to receive lacpdus on a bond */
> -static void bond_unregister_lacpdu(struct bonding *bond)
> -{
> - dev_remove_pack(&(BOND_AD_INFO(bond).ad_pkt_type));
> -}
> -
> -void bond_register_arp(struct bonding *bond)
> -{
> - struct packet_type *pt =&bond->arp_mon_pt;
> -
> - if (pt->type)
> - return;
> -
> - pt->type = htons(ETH_P_ARP);
> - pt->dev = bond->dev;
> - pt->func = bond_arp_rcv;
> - dev_add_pack(pt);
> -}
> -
> -void bond_unregister_arp(struct bonding *bond)
> -{
> - struct packet_type *pt =&bond->arp_mon_pt;
> -
> - dev_remove_pack(pt);
> - pt->type = 0;
> -}
> -
> /*---------------------------- Hashing Policies -----------------------------*/
>
> /*
> @@ -3917,14 +3849,14 @@ static int bond_open(struct net_device *bond_dev)
>
> queue_delayed_work(bond->wq,&bond->arp_work, 0);
> if (bond->params.arp_validate)
> - bond_register_arp(bond);
> + bond->recv_probe = bond_arp_rcv;
> }
>
> if (bond->params.mode == BOND_MODE_8023AD) {
> INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler);
> queue_delayed_work(bond->wq,&bond->ad_work, 0);
> /* register to receive LACPDUs */
> - bond_register_lacpdu(bond);
> + bond->recv_probe = bond_3ad_lacpdu_recv;
> bond_3ad_initiate_agg_selection(bond, 1);
> }
>
> @@ -3935,14 +3867,6 @@ static int bond_close(struct net_device *bond_dev)
> {
> struct bonding *bond = netdev_priv(bond_dev);
>
> - if (bond->params.mode == BOND_MODE_8023AD) {
> - /* Unregister the receive of LACPDUs */
> - bond_unregister_lacpdu(bond);
> - }
> -
> - if (bond->params.arp_validate)
> - bond_unregister_arp(bond);
> -
> write_lock_bh(&bond->lock);
>
> bond->send_grat_arp = 0;
> @@ -3982,6 +3906,7 @@ static int bond_close(struct net_device *bond_dev)
> */
> bond_alb_deinitialize(bond);
> }
> + bond->recv_probe = NULL;
>
> return 0;
> }
> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index 5161183..6804efe 100644
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -419,11 +419,6 @@ static ssize_t bonding_store_arp_validate(struct device *d,
> bond->dev->name, arp_validate_tbl[new_value].modename,
> new_value);
>
> - if (!bond->params.arp_validate&& new_value)
> - bond_register_arp(bond);
> - else if (bond->params.arp_validate&& !new_value)
> - bond_unregister_arp(bond);
> -
> bond->params.arp_validate = new_value;
>
> return count;
> @@ -998,7 +993,6 @@ static ssize_t bonding_store_miimon(struct device *d,
> bond->dev->name);
> bond->params.arp_interval = 0;
> if (bond->params.arp_validate) {
> - bond_unregister_arp(bond);
> bond->params.arp_validate =
> BOND_ARP_VALIDATE_NONE;
> }
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index 8f78166..75a1308 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -228,6 +228,8 @@ struct bonding {
> struct slave *primary_slave;
> bool force_primary;
> s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
> + void (*recv_probe)(struct sk_buff *, struct bonding *,
> + struct slave *);
> rwlock_t lock;
> rwlock_t curr_slave_lock;
> s8 kill_timers;
> @@ -406,8 +408,6 @@ void bond_set_mode_ops(struct bonding *bond, int mode);
> int bond_parse_parm(const char *mode_arg, const struct bond_parm_tbl *tbl);
> void bond_select_active_slave(struct bonding *bond);
> void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
> -void bond_register_arp(struct bonding *);
> -void bond_unregister_arp(struct bonding *);
> void bond_create_debugfs(void);
> void bond_destroy_debugfs(void);
> void bond_debug_register(struct bonding *bond);
^ permalink raw reply
* Re: [patch net-next-2.6 3/8] net: get rid of multiple bond-related netdevice->priv_flags
From: Ben Hutchings @ 2011-03-05 14:37 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: Jiri Pirko, netdev, davem, shemminger, kaber, fubar, eric.dumazet,
andy
In-Reply-To: <4D72453F.10909@gmail.com>
On Sat, 2011-03-05 at 15:14 +0100, Nicolas de Pesloüan wrote:
> Le 05/03/2011 11:29, Jiri Pirko a écrit :
[...]
> > --- a/include/linux/if.h
> > +++ b/include/linux/if.h
> > @@ -60,21 +60,17 @@
> > #define IFF_802_1Q_VLAN 0x1 /* 802.1Q VLAN device. */
> > #define IFF_EBRIDGE 0x2 /* Ethernet bridging device. */
> > #define IFF_SLAVE_INACTIVE 0x4 /* bonding slave not the curr. active */
> > -#define IFF_MASTER_8023AD 0x8 /* bonding master, 802.3ad. */
> > -#define IFF_MASTER_ALB 0x10 /* bonding master, balance-alb. */
> > -#define IFF_BONDING 0x20 /* bonding master or slave */
> > -#define IFF_SLAVE_NEEDARP 0x40 /* need ARPs for validation */
> > -#define IFF_ISATAP 0x80 /* ISATAP interface (RFC4214) */
> > -#define IFF_MASTER_ARPMON 0x100 /* bonding master, ARP mon in use */
> > -#define IFF_WAN_HDLC 0x200 /* WAN HDLC device */
> > -#define IFF_XMIT_DST_RELEASE 0x400 /* dev_hard_start_xmit() is allowed to
> > +#define IFF_BONDING 0x8 /* bonding master or slave */
> > +#define IFF_ISATAP 0x10 /* ISATAP interface (RFC4214) */
> > +#define IFF_WAN_HDLC 0x20 /* WAN HDLC device */
> > +#define IFF_XMIT_DST_RELEASE 0x40 /* dev_hard_start_xmit() is allowed to
> > * release skb->dst
> > */
>
> Why did you changed those values? Aren't those exposed to userland? Just removing the unused one
> sounds good to me.
[...]
Look just one line further up:
/* Private (from user) interface flags (netdevice->priv_flags). */
Maybe they should have a prefix IFPF_ instead of IFF_ though.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [patch net-next-2.6 2/8] bonding: register slave pointer for rx_handler
From: Nicolas de Pesloüan @ 2011-03-05 14:38 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <20110305142732.GB8573@psychotron.redhat.com>
Le 05/03/2011 15:27, Jiri Pirko a écrit :
> Sat, Mar 05, 2011 at 03:06:58PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 05/03/2011 11:29, Jiri Pirko a écrit :
>>> Register slave pointer as rx_handler data. That would eventually prevent
>>> need to loop over slave devices to find the right slave.
>>>
>>> Use synchronize_net to ensure that bond_handle_frame does not get slave
>>> structure freed when working with that.
>>>
>>> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>> ---
>>> drivers/net/bonding/bond_main.c | 17 +++++++++++------
>>> drivers/net/bonding/bonding.h | 3 +++
>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 0592e6d..1c19368 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1495,21 +1495,22 @@ static bool bond_should_deliver_exact_match(struct sk_buff *skb,
>>>
>>> static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
>>> {
>>> - struct net_device *slave_dev;
>>> + struct slave *slave;
>>> struct net_device *bond_dev;
>>>
>>> skb = skb_share_check(skb, GFP_ATOMIC);
>>> if (unlikely(!skb))
>>> return NULL;
>>> - slave_dev = skb->dev;
>>> - bond_dev = ACCESS_ONCE(slave_dev->master);
>>> +
>>> + slave = bond_slave_get_rcu(skb->dev);
>>> + bond_dev = ACCESS_ONCE(slave->dev->master);
>>> if (unlikely(!bond_dev))
>>> return skb;
>>>
>>> if (bond_dev->priv_flags& IFF_MASTER_ARPMON)
>>> - slave_dev->last_rx = jiffies;
>>> + slave->dev->last_rx = jiffies;
>>>
>>> - if (bond_should_deliver_exact_match(skb, slave_dev, bond_dev)) {
>>> + if (bond_should_deliver_exact_match(skb, slave->dev, bond_dev)) {
>>> skb->deliver_no_wcard = 1;
>>> return skb;
>>> }
>>
>> Up to this point, it looks like cleanup, and unrelated to the title and description of the patch.
>
> Nope - these are changes connected to the ability to get slave struct
> via bond_slave_get_rcu. Very related.
Agreed.
>>
>> Anyway, the cleanup looks good to me. Should just be in a separate patch.
>>
>>> @@ -1703,7 +1704,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>> pr_debug("Error %d calling netdev_set_bond_master\n", res);
>>> goto err_restore_mac;
>>> }
>>> - res = netdev_rx_handler_register(slave_dev, bond_handle_frame, NULL);
>>> + res = netdev_rx_handler_register(slave_dev, bond_handle_frame,
>>> + new_slave);
>>
>> And using rx_handler data for this purpose sounds good to me.
>>
>> Reviewed-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
>>
>>> if (res) {
>>> pr_debug("Error %d calling netdev_rx_handler_register\n", res);
>>> goto err_unset_master;
>>> @@ -1925,6 +1927,7 @@ err_close:
>>>
>>> err_unreg_rxhandler:
>>> netdev_rx_handler_unregister(slave_dev);
>>> + synchronize_net();
>>>
>>> err_unset_master:
>>> netdev_set_bond_master(slave_dev, NULL);
>>> @@ -2108,6 +2111,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>>> }
>>>
>>> netdev_rx_handler_unregister(slave_dev);
>>> + synchronize_net();
>>> netdev_set_bond_master(slave_dev, NULL);
>>>
>>> slave_disable_netpoll(slave);
>>> @@ -2222,6 +2226,7 @@ static int bond_release_all(struct net_device *bond_dev)
>>> }
>>>
>>> netdev_rx_handler_unregister(slave_dev);
>>> + synchronize_net();
>>> netdev_set_bond_master(slave_dev, NULL);
>>>
>>> slave_disable_netpoll(slave);
>>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>> index ff4e269..1aac5cd 100644
>>> --- a/drivers/net/bonding/bonding.h
>>> +++ b/drivers/net/bonding/bonding.h
>>> @@ -264,6 +264,9 @@ struct bonding {
>>> #endif /* CONFIG_DEBUG_FS */
>>> };
>>>
>>> +#define bond_slave_get_rcu(dev) \
>>> + ((struct slave *) rcu_dereference(dev->rx_handler_data))
>>> +
>>> /**
>>> * Returns NULL if the net_device does not belong to any of the bond's slaves
>>> *
>>
>
^ permalink raw reply
* Re: [patch net-next-2.6 6/8] bonding: move processing of recv handlers into handle_frame()
From: Jiri Pirko @ 2011-03-05 14:43 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <4D7249BA.8030401@gmail.com>
Sat, Mar 05, 2011 at 03:33:30PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 05/03/2011 11:29, Jiri Pirko a écrit :
>>Since now when bonding uses rx_handler, all traffic going into bond
>>device goes thru bond_handle_frame. So there's no need to go back into
>>bonding code later via ptype handlers. This patch converts
>>original ptype handlers into "bonding receive probes". These functions
>>are called from bond_handle_frame and they are registered per-mode.
>
>Does this still support having the arp_ip_target on a vlan?
>
>(eth0 -> bond0 -> bond0.100, with arp_ip_target only reachable through bond0.100).
This case is still covered with vlan_on_bond_hook
eth0->
bond_handle_frame
bond0->
vlan_hwaccel_do_receive
bond0.5->
vlan_on_bond_hook -> reinject into bond0
-> bond_handle_frame (here it is processed)
>
>>
>>Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>---
>> drivers/net/bonding/bond_3ad.c | 29 ++--------
>> drivers/net/bonding/bond_3ad.h | 4 +-
>> drivers/net/bonding/bond_alb.c | 46 ++++-----------
>> drivers/net/bonding/bond_alb.h | 1 -
>> drivers/net/bonding/bond_main.c | 121 +++++++------------------------------
>> drivers/net/bonding/bond_sysfs.c | 6 --
>> drivers/net/bonding/bonding.h | 4 +-
>> 7 files changed, 43 insertions(+), 168 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>index 047af0b..194aaf7 100644
>>--- a/drivers/net/bonding/bond_3ad.c
>>+++ b/drivers/net/bonding/bond_3ad.c
>>@@ -2461,35 +2461,16 @@ out:
>> return NETDEV_TX_OK;
>> }
>>
>>-int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type* ptype, struct net_device *orig_dev)
>>+void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>>+ struct slave *slave)
>> {
>>- struct bonding *bond = netdev_priv(dev);
>>- struct slave *slave = NULL;
>>- int ret = NET_RX_DROP;
>>-
>>- if (!(dev->flags& IFF_MASTER))
>>- goto out;
>>-
>>- skb = skb_share_check(skb, GFP_ATOMIC);
>>- if (!skb)
>>- goto out;
>>+ if (skb->protocol != PKT_TYPE_LACPDU)
>>+ return;
>>
>> if (!pskb_may_pull(skb, sizeof(struct lacpdu)))
>>- goto out;
>>+ return;
>>
>> read_lock(&bond->lock);
>>- slave = bond_get_slave_by_dev(netdev_priv(dev), orig_dev);
>>- if (!slave)
>>- goto out_unlock;
>>-
>> bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
>>-
>>- ret = NET_RX_SUCCESS;
>>-
>>-out_unlock:
>> read_unlock(&bond->lock);
>>-out:
>>- dev_kfree_skb(skb);
>>-
>>- return ret;
>> }
>>diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
>>index 2c46a154..56a88ff 100644
>>--- a/drivers/net/bonding/bond_3ad.h
>>+++ b/drivers/net/bonding/bond_3ad.h
>>@@ -258,7 +258,6 @@ struct ad_bond_info {
>> * requested
>> */
>> struct timer_list ad_timer;
>>- struct packet_type ad_pkt_type;
>> };
>>
>> struct ad_slave_info {
>>@@ -279,7 +278,8 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave);
>> void bond_3ad_handle_link_change(struct slave *slave, char link);
>> int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
>> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
>>-int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type* ptype, struct net_device *orig_dev);
>>+void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>>+ struct slave *slave);
>> int bond_3ad_set_carrier(struct bonding *bond);
>> #endif //__BOND_3AD_H__
>>
>>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>>index 9bc5de3..96d28f9 100644
>>--- a/drivers/net/bonding/bond_alb.c
>>+++ b/drivers/net/bonding/bond_alb.c
>>@@ -308,49 +308,33 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
>> _unlock_rx_hashtbl(bond);
>> }
>>
>>-static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct packet_type *ptype, struct net_device *orig_dev)
>>+static void rlb_arp_recv(struct sk_buff *skb, struct bonding *bond,
>>+ struct slave *slave)
>> {
>>- struct bonding *bond;
>>- struct arp_pkt *arp = (struct arp_pkt *)skb->data;
>>- int res = NET_RX_DROP;
>>+ struct arp_pkt *arp;
>>
>>- while (bond_dev->priv_flags& IFF_802_1Q_VLAN)
>>- bond_dev = vlan_dev_real_dev(bond_dev);
>>-
>>- if (!(bond_dev->priv_flags& IFF_BONDING) ||
>>- !(bond_dev->flags& IFF_MASTER))
>>- goto out;
>>+ if (skb->protocol != cpu_to_be16(ETH_P_ARP))
>>+ return;
>>
>>+ arp = (struct arp_pkt *) skb->data;
>> if (!arp) {
>> pr_debug("Packet has no ARP data\n");
>>- goto out;
>>+ return;
>> }
>>
>>- skb = skb_share_check(skb, GFP_ATOMIC);
>>- if (!skb)
>>- goto out;
>>-
>>- if (!pskb_may_pull(skb, arp_hdr_len(bond_dev)))
>>- goto out;
>>+ if (!pskb_may_pull(skb, arp_hdr_len(bond->dev)))
>>+ return;
>>
>> if (skb->len< sizeof(struct arp_pkt)) {
>> pr_debug("Packet is too small to be an ARP\n");
>>- goto out;
>>+ return;
>> }
>>
>> if (arp->op_code == htons(ARPOP_REPLY)) {
>> /* update rx hash table for this ARP */
>>- bond = netdev_priv(bond_dev);
>> rlb_update_entry_from_arp(bond, arp);
>> pr_debug("Server received an ARP Reply from client\n");
>> }
>>-
>>- res = NET_RX_SUCCESS;
>>-
>>-out:
>>- dev_kfree_skb(skb);
>>-
>>- return res;
>> }
>>
>> /* Caller must hold bond lock for read */
>>@@ -759,7 +743,6 @@ static void rlb_init_table_entry(struct rlb_client_info *entry)
>> static int rlb_initialize(struct bonding *bond)
>> {
>> struct alb_bond_info *bond_info =&(BOND_ALB_INFO(bond));
>>- struct packet_type *pk_type =&(BOND_ALB_INFO(bond).rlb_pkt_type);
>> struct rlb_client_info *new_hashtbl;
>> int size = RLB_HASH_TABLE_SIZE * sizeof(struct rlb_client_info);
>> int i;
>>@@ -784,13 +767,8 @@ static int rlb_initialize(struct bonding *bond)
>>
>> _unlock_rx_hashtbl(bond);
>>
>>- /*initialize packet type*/
>>- pk_type->type = cpu_to_be16(ETH_P_ARP);
>>- pk_type->dev = bond->dev;
>>- pk_type->func = rlb_arp_recv;
>>-
>> /* register to receive ARPs */
>>- dev_add_pack(pk_type);
>>+ bond->recv_probe = rlb_arp_recv;
>>
>> return 0;
>> }
>>@@ -799,8 +777,6 @@ static void rlb_deinitialize(struct bonding *bond)
>> {
>> struct alb_bond_info *bond_info =&(BOND_ALB_INFO(bond));
>>
>>- dev_remove_pack(&(bond_info->rlb_pkt_type));
>>-
>> _lock_rx_hashtbl(bond);
>>
>> kfree(bond_info->rx_hashtbl);
>>diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
>>index 118c28a..6605e9e 100644
>>--- a/drivers/net/bonding/bond_alb.h
>>+++ b/drivers/net/bonding/bond_alb.h
>>@@ -130,7 +130,6 @@ struct alb_bond_info {
>> int lp_counter;
>> /* -------- rlb parameters -------- */
>> int rlb_enabled;
>>- struct packet_type rlb_pkt_type;
>> struct rlb_client_info *rx_hashtbl; /* Receive hash table */
>> spinlock_t rx_hashtbl_lock;
>> u32 rx_hashtbl_head;
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index dbe182c..8ba7faa 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1467,27 +1467,17 @@ static void bond_setup_by_slave(struct net_device *bond_dev,
>> }
>>
>> /* On bonding slaves other than the currently active slave, suppress
>>- * duplicates except for 802.3ad ETH_P_SLOW, alb non-mcast/bcast, and
>>- * ARP on active-backup slaves with arp_validate enabled.
>>+ * duplicates except for alb non-mcast/bcast.
>> */
>> static bool bond_should_deliver_exact_match(struct sk_buff *skb,
>> struct slave *slave,
>> struct bonding *bond)
>> {
>> if (bond_is_slave_inactive(slave)) {
>>- if (slave_do_arp_validate(bond, slave)&&
>>- skb->protocol == __cpu_to_be16(ETH_P_ARP))
>>- return false;
>>-
>> if (bond->params.mode == BOND_MODE_ALB&&
>> skb->pkt_type != PACKET_BROADCAST&&
>> skb->pkt_type != PACKET_MULTICAST)
>>- return false;
>>-
>>- if (bond->params.mode == BOND_MODE_8023AD&&
>>- skb->protocol == __cpu_to_be16(ETH_P_SLOW))
>> return false;
>>-
>> return true;
>> }
>> return false;
>>@@ -1513,6 +1503,15 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
>> if (bond->params.arp_interval)
>> slave->dev->last_rx = jiffies;
>>
>>+ if (bond->recv_probe) {
>>+ struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>>+
>>+ if (likely(nskb)) {
>>+ bond->recv_probe(nskb, bond, slave);
>>+ dev_kfree_skb(nskb);
>>+ }
>>+ }
>>+
>> if (bond_should_deliver_exact_match(skb, slave, bond)) {
>> skb->deliver_no_wcard = 1;
>> return skb;
>>@@ -2813,48 +2812,26 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
>> }
>> }
>>
>>-static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt, struct net_device *orig_dev)
>>+static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
>>+ struct slave *slave)
>> {
>> struct arphdr *arp;
>>- struct slave *slave;
>>- struct bonding *bond;
>> unsigned char *arp_ptr;
>> __be32 sip, tip;
>>
>>- if (dev->priv_flags& IFF_802_1Q_VLAN) {
>>- /*
>>- * When using VLANS and bonding, dev and oriv_dev may be
>>- * incorrect if the physical interface supports VLAN
>>- * acceleration. With this change ARP validation now
>>- * works for hosts only reachable on the VLAN interface.
>>- */
>>- dev = vlan_dev_real_dev(dev);
>>- orig_dev = dev_get_by_index_rcu(dev_net(skb->dev),skb->skb_iif);
>>- }
>>-
>>- if (!(dev->priv_flags& IFF_BONDING) || !(dev->flags& IFF_MASTER))
>>- goto out;
>>+ if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
>>+ return;
>
>What happens if the skb that hold the ARP request/reply is vlan
>tagged? The ARP ptype_base handler for bonding used to be called
>after vlan processing.
>
>>- bond = netdev_priv(dev);
>> read_lock(&bond->lock);
>>
>>- pr_debug("bond_arp_rcv: bond %s skb->dev %s orig_dev %s\n",
>>- bond->dev->name, skb->dev ? skb->dev->name : "NULL",
>>- orig_dev ? orig_dev->name : "NULL");
>>+ pr_debug("bond_arp_rcv: bond %s skb->dev %s\n",
>>+ bond->dev->name, skb->dev->name);
>>
>>- slave = bond_get_slave_by_dev(bond, orig_dev);
>>- if (!slave || !slave_do_arp_validate(bond, slave))
>>- goto out_unlock;
>>-
>>- skb = skb_share_check(skb, GFP_ATOMIC);
>>- if (!skb)
>>- goto out_unlock;
>>-
>>- if (!pskb_may_pull(skb, arp_hdr_len(dev)))
>>+ if (!pskb_may_pull(skb, arp_hdr_len(bond->dev)))
>> goto out_unlock;
>>
>> arp = arp_hdr(skb);
>>- if (arp->ar_hln != dev->addr_len ||
>>+ if (arp->ar_hln != bond->dev->addr_len ||
>> skb->pkt_type == PACKET_OTHERHOST ||
>> skb->pkt_type == PACKET_LOOPBACK ||
>> arp->ar_hrd != htons(ARPHRD_ETHER) ||
>>@@ -2863,9 +2840,9 @@ static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct pack
>> goto out_unlock;
>>
>> arp_ptr = (unsigned char *)(arp + 1);
>>- arp_ptr += dev->addr_len;
>>+ arp_ptr += bond->dev->addr_len;
>> memcpy(&sip, arp_ptr, 4);
>>- arp_ptr += 4 + dev->addr_len;
>>+ arp_ptr += 4 + bond->dev->addr_len;
>> memcpy(&tip, arp_ptr, 4);
>>
>> pr_debug("bond_arp_rcv: %s %s/%d av %d sv %d sip %pI4 tip %pI4\n",
>>@@ -2888,9 +2865,6 @@ static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct pack
>>
>> out_unlock:
>> read_unlock(&bond->lock);
>>-out:
>>- dev_kfree_skb(skb);
>>- return NET_RX_SUCCESS;
>> }
>>
>> /*
>>@@ -3782,48 +3756,6 @@ static struct notifier_block bond_inetaddr_notifier = {
>> .notifier_call = bond_inetaddr_event,
>> };
>>
>>-/*-------------------------- Packet type handling ---------------------------*/
>>-
>>-/* register to receive lacpdus on a bond */
>>-static void bond_register_lacpdu(struct bonding *bond)
>>-{
>>- struct packet_type *pk_type =&(BOND_AD_INFO(bond).ad_pkt_type);
>>-
>>- /* initialize packet type */
>>- pk_type->type = PKT_TYPE_LACPDU;
>>- pk_type->dev = bond->dev;
>>- pk_type->func = bond_3ad_lacpdu_recv;
>>-
>>- dev_add_pack(pk_type);
>>-}
>>-
>>-/* unregister to receive lacpdus on a bond */
>>-static void bond_unregister_lacpdu(struct bonding *bond)
>>-{
>>- dev_remove_pack(&(BOND_AD_INFO(bond).ad_pkt_type));
>>-}
>>-
>>-void bond_register_arp(struct bonding *bond)
>>-{
>>- struct packet_type *pt =&bond->arp_mon_pt;
>>-
>>- if (pt->type)
>>- return;
>>-
>>- pt->type = htons(ETH_P_ARP);
>>- pt->dev = bond->dev;
>>- pt->func = bond_arp_rcv;
>>- dev_add_pack(pt);
>>-}
>>-
>>-void bond_unregister_arp(struct bonding *bond)
>>-{
>>- struct packet_type *pt =&bond->arp_mon_pt;
>>-
>>- dev_remove_pack(pt);
>>- pt->type = 0;
>>-}
>>-
>> /*---------------------------- Hashing Policies -----------------------------*/
>>
>> /*
>>@@ -3917,14 +3849,14 @@ static int bond_open(struct net_device *bond_dev)
>>
>> queue_delayed_work(bond->wq,&bond->arp_work, 0);
>> if (bond->params.arp_validate)
>>- bond_register_arp(bond);
>>+ bond->recv_probe = bond_arp_rcv;
>> }
>>
>> if (bond->params.mode == BOND_MODE_8023AD) {
>> INIT_DELAYED_WORK(&bond->ad_work, bond_3ad_state_machine_handler);
>> queue_delayed_work(bond->wq,&bond->ad_work, 0);
>> /* register to receive LACPDUs */
>>- bond_register_lacpdu(bond);
>>+ bond->recv_probe = bond_3ad_lacpdu_recv;
>> bond_3ad_initiate_agg_selection(bond, 1);
>> }
>>
>>@@ -3935,14 +3867,6 @@ static int bond_close(struct net_device *bond_dev)
>> {
>> struct bonding *bond = netdev_priv(bond_dev);
>>
>>- if (bond->params.mode == BOND_MODE_8023AD) {
>>- /* Unregister the receive of LACPDUs */
>>- bond_unregister_lacpdu(bond);
>>- }
>>-
>>- if (bond->params.arp_validate)
>>- bond_unregister_arp(bond);
>>-
>> write_lock_bh(&bond->lock);
>>
>> bond->send_grat_arp = 0;
>>@@ -3982,6 +3906,7 @@ static int bond_close(struct net_device *bond_dev)
>> */
>> bond_alb_deinitialize(bond);
>> }
>>+ bond->recv_probe = NULL;
>>
>> return 0;
>> }
>>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>>index 5161183..6804efe 100644
>>--- a/drivers/net/bonding/bond_sysfs.c
>>+++ b/drivers/net/bonding/bond_sysfs.c
>>@@ -419,11 +419,6 @@ static ssize_t bonding_store_arp_validate(struct device *d,
>> bond->dev->name, arp_validate_tbl[new_value].modename,
>> new_value);
>>
>>- if (!bond->params.arp_validate&& new_value)
>>- bond_register_arp(bond);
>>- else if (bond->params.arp_validate&& !new_value)
>>- bond_unregister_arp(bond);
>>-
>> bond->params.arp_validate = new_value;
>>
>> return count;
>>@@ -998,7 +993,6 @@ static ssize_t bonding_store_miimon(struct device *d,
>> bond->dev->name);
>> bond->params.arp_interval = 0;
>> if (bond->params.arp_validate) {
>>- bond_unregister_arp(bond);
>> bond->params.arp_validate =
>> BOND_ARP_VALIDATE_NONE;
>> }
>>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>index 8f78166..75a1308 100644
>>--- a/drivers/net/bonding/bonding.h
>>+++ b/drivers/net/bonding/bonding.h
>>@@ -228,6 +228,8 @@ struct bonding {
>> struct slave *primary_slave;
>> bool force_primary;
>> s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
>>+ void (*recv_probe)(struct sk_buff *, struct bonding *,
>>+ struct slave *);
>> rwlock_t lock;
>> rwlock_t curr_slave_lock;
>> s8 kill_timers;
>>@@ -406,8 +408,6 @@ void bond_set_mode_ops(struct bonding *bond, int mode);
>> int bond_parse_parm(const char *mode_arg, const struct bond_parm_tbl *tbl);
>> void bond_select_active_slave(struct bonding *bond);
>> void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
>>-void bond_register_arp(struct bonding *);
>>-void bond_unregister_arp(struct bonding *);
>> void bond_create_debugfs(void);
>> void bond_destroy_debugfs(void);
>> void bond_debug_register(struct bonding *bond);
>
^ permalink raw reply
* Re: [patch net-next-2.6 3/8] net: get rid of multiple bond-related netdevice->priv_flags
From: Nicolas de Pesloüan @ 2011-03-05 14:46 UTC (permalink / raw)
To: Ben Hutchings
Cc: Jiri Pirko, netdev, davem, shemminger, kaber, fubar, eric.dumazet,
andy
In-Reply-To: <1299335862.4277.99.camel@localhost>
Le 05/03/2011 15:37, Ben Hutchings a écrit :
> On Sat, 2011-03-05 at 15:14 +0100, Nicolas de Pesloüan wrote:
>> Le 05/03/2011 11:29, Jiri Pirko a écrit :
> [...]
>>> --- a/include/linux/if.h
>>> +++ b/include/linux/if.h
>>> @@ -60,21 +60,17 @@
>>> #define IFF_802_1Q_VLAN 0x1 /* 802.1Q VLAN device. */
>>> #define IFF_EBRIDGE 0x2 /* Ethernet bridging device. */
>>> #define IFF_SLAVE_INACTIVE 0x4 /* bonding slave not the curr. active */
>>> -#define IFF_MASTER_8023AD 0x8 /* bonding master, 802.3ad. */
>>> -#define IFF_MASTER_ALB 0x10 /* bonding master, balance-alb. */
>>> -#define IFF_BONDING 0x20 /* bonding master or slave */
>>> -#define IFF_SLAVE_NEEDARP 0x40 /* need ARPs for validation */
>>> -#define IFF_ISATAP 0x80 /* ISATAP interface (RFC4214) */
>>> -#define IFF_MASTER_ARPMON 0x100 /* bonding master, ARP mon in use */
>>> -#define IFF_WAN_HDLC 0x200 /* WAN HDLC device */
>>> -#define IFF_XMIT_DST_RELEASE 0x400 /* dev_hard_start_xmit() is allowed to
>>> +#define IFF_BONDING 0x8 /* bonding master or slave */
>>> +#define IFF_ISATAP 0x10 /* ISATAP interface (RFC4214) */
>>> +#define IFF_WAN_HDLC 0x20 /* WAN HDLC device */
>>> +#define IFF_XMIT_DST_RELEASE 0x40 /* dev_hard_start_xmit() is allowed to
>>> * release skb->dst
>>> */
>>
>> Why did you changed those values? Aren't those exposed to userland? Just removing the unused one
>> sounds good to me.
> [...]
>
> Look just one line further up:
>
> /* Private (from user) interface flags (netdevice->priv_flags). */
>
> Maybe they should have a prefix IFPF_ instead of IFF_ though.
>
> Ben.
Yes, I noticed it while reviewing the next one. Sorry for the noise.
So,
Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
^ permalink raw reply
* Re: [patch net-next-2.6 6/8] bonding: move processing of recv handlers into handle_frame()
From: Nicolas de Pesloüan @ 2011-03-05 14:50 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <20110305144314.GC8573@psychotron.redhat.com>
Le 05/03/2011 15:43, Jiri Pirko a écrit :
> Sat, Mar 05, 2011 at 03:33:30PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 05/03/2011 11:29, Jiri Pirko a écrit :
>>> Since now when bonding uses rx_handler, all traffic going into bond
>>> device goes thru bond_handle_frame. So there's no need to go back into
>>> bonding code later via ptype handlers. This patch converts
>>> original ptype handlers into "bonding receive probes". These functions
>>> are called from bond_handle_frame and they are registered per-mode.
>>
>> Does this still support having the arp_ip_target on a vlan?
>>
>> (eth0 -> bond0 -> bond0.100, with arp_ip_target only reachable through bond0.100).
>
> This case is still covered with vlan_on_bond_hook
> eth0->
> bond_handle_frame
> bond0->
> vlan_hwaccel_do_receive
> bond0.5->
> vlan_on_bond_hook -> reinject into bond0
> -> bond_handle_frame (here it is processed)
Sound good to me.
Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
^ permalink raw reply
* Re: [patch net-next-2.6 7/8] net: introduce rx_handler results and logic around that
From: Nicolas de Pesloüan @ 2011-03-05 14:52 UTC (permalink / raw)
To: Jiri Pirko
Cc: Ben Hutchings, netdev, davem, shemminger, kaber, fubar,
eric.dumazet, andy
In-Reply-To: <1299329330.4277.97.camel@localhost>
Le 05/03/2011 13:48, Ben Hutchings a écrit :
> On Sat, 2011-03-05 at 11:29 +0100, Jiri Pirko wrote:
>> This patch allows rx_handlers to better signalize what to do next to
>> it's caller. That makes skb->deliver_no_wcard no longer needed.
> [...]
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -390,7 +390,14 @@ enum gro_result {
>> };
>> typedef enum gro_result gro_result_t;
>>
>> -typedef struct sk_buff *rx_handler_func_t(struct sk_buff *skb);
>> +enum rx_handler_result {
>> + RX_HANDLER_CONSUMED,
>> + RX_HANDLER_ANOTHER,
>> + RX_HANDLER_EXACT,
>> + RX_HANDLER_PASS,
>> +};
> [...]
>
> This should have a comment (preferably kernel-doc) clearly specifying
> the meaning of each code, as the differences between ANOTHER/EXACT/PASS
> are fairly subtle.
>
> Ben,
Except from the lack of proper documentation, this patch looks very good.
Nicolas.
^ permalink raw reply
* Re: [patch net-next-2.6 7/8] net: introduce rx_handler results and logic around that
From: Jiri Pirko @ 2011-03-05 14:54 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: Ben Hutchings, netdev, davem, shemminger, kaber, fubar,
eric.dumazet, andy
In-Reply-To: <4D724E23.7020904@gmail.com>
Sat, Mar 05, 2011 at 03:52:19PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 05/03/2011 13:48, Ben Hutchings a écrit :
>>On Sat, 2011-03-05 at 11:29 +0100, Jiri Pirko wrote:
>>>This patch allows rx_handlers to better signalize what to do next to
>>>it's caller. That makes skb->deliver_no_wcard no longer needed.
>>[...]
>>>--- a/include/linux/netdevice.h
>>>+++ b/include/linux/netdevice.h
>>>@@ -390,7 +390,14 @@ enum gro_result {
>>> };
>>> typedef enum gro_result gro_result_t;
>>>
>>>-typedef struct sk_buff *rx_handler_func_t(struct sk_buff *skb);
>>>+enum rx_handler_result {
>>>+ RX_HANDLER_CONSUMED,
>>>+ RX_HANDLER_ANOTHER,
>>>+ RX_HANDLER_EXACT,
>>>+ RX_HANDLER_PASS,
>>>+};
>>[...]
>>
>>This should have a comment (preferably kernel-doc) clearly specifying
>>the meaning of each code, as the differences between ANOTHER/EXACT/PASS
>>are fairly subtle.
>>
>>Ben,
>
>Except from the lack of proper documentation, this patch looks very good.
Okay guys, I'll write something about that and send it in another patch.
>
> Nicolas.
^ permalink raw reply
* Re: [patch net-next-2.6 8/8] net: get rid of orig_dev parameter of packet handlers
From: Nicolas de Pesloüan @ 2011-03-05 15:05 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <1299320969-7951-9-git-send-email-jpirko@redhat.com>
Le 05/03/2011 11:29, Jiri Pirko a écrit :
> Since orig_dev was used in af_packet (where it was replaced) and by
> bonding ptype handlers (which are no longer with us) only, orig_dev
> parameter is no longer needed -> toss it out
>
> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
[snip]
> - new->deliver_no_wcard = old->deliver_no_wcard;
deliver_no_wcard was removed from sk_buff in your #7 patch. Shouldn't the above line be part of the
#7 patch?
Except from this point,
Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
Nicolas.
^ permalink raw reply
* Re: [patch net-next-2.6 7/8] net: introduce rx_handler results and logic around that
From: Nicolas de Pesloüan @ 2011-03-05 15:06 UTC (permalink / raw)
To: Jiri Pirko
Cc: Ben Hutchings, netdev, davem, shemminger, kaber, fubar,
eric.dumazet, andy
In-Reply-To: <20110305145408.GD8573@psychotron.redhat.com>
Le 05/03/2011 15:54, Jiri Pirko a écrit :
> Sat, Mar 05, 2011 at 03:52:19PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 05/03/2011 13:48, Ben Hutchings a écrit :
>>> On Sat, 2011-03-05 at 11:29 +0100, Jiri Pirko wrote:
>>>> This patch allows rx_handlers to better signalize what to do next to
>>>> it's caller. That makes skb->deliver_no_wcard no longer needed.
>>> [...]
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -390,7 +390,14 @@ enum gro_result {
>>>> };
>>>> typedef enum gro_result gro_result_t;
>>>>
>>>> -typedef struct sk_buff *rx_handler_func_t(struct sk_buff *skb);
>>>> +enum rx_handler_result {
>>>> + RX_HANDLER_CONSUMED,
>>>> + RX_HANDLER_ANOTHER,
>>>> + RX_HANDLER_EXACT,
>>>> + RX_HANDLER_PASS,
>>>> +};
>>> [...]
>>>
>>> This should have a comment (preferably kernel-doc) clearly specifying
>>> the meaning of each code, as the differences between ANOTHER/EXACT/PASS
>>> are fairly subtle.
>>>
>>> Ben,
>>
>> Except from the lack of proper documentation, this patch looks very good.
>
> Okay guys, I'll write something about that and send it in another patch.
Agreed that it can be in a follow-up patch.
Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
^ permalink raw reply
* [patch net-next-2.6] net: comment rx_handler results
From: Jiri Pirko @ 2011-03-05 15:13 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet,
nicolas.2p.debian, andy
In-Reply-To: <1299329330.4277.97.camel@localhost>
Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
include/linux/netdevice.h | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 48a9638..26e03f9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -391,10 +391,14 @@ enum gro_result {
typedef enum gro_result gro_result_t;
enum rx_handler_result {
- RX_HANDLER_CONSUMED,
- RX_HANDLER_ANOTHER,
- RX_HANDLER_EXACT,
- RX_HANDLER_PASS,
+ RX_HANDLER_CONSUMED, /* skb was consumed by rx_handler,
+ do not process it further. */
+ RX_HANDLER_ANOTHER, /* Do another round in receive path.
+ This is indicated in case skb->dev
+ was changed by rx_handler */
+ RX_HANDLER_EXACT, /* Force exact delivery, no wildcard */
+ RX_HANDLER_PASS, /* Do nothing, pass the skb as if
+ no rx_handler was called */
};
typedef enum rx_handler_result rx_handler_result_t;
typedef rx_handler_result_t rx_handler_func_t(struct sk_buff **pskb);
--
1.7.4
^ permalink raw reply related
* Re: [patch net-next-2.6 8/8] net: get rid of orig_dev parameter of packet handlers
From: Jiri Pirko @ 2011-03-05 15:15 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <4D72512C.2060802@gmail.com>
Sat, Mar 05, 2011 at 04:05:16PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 05/03/2011 11:29, Jiri Pirko a écrit :
>>Since orig_dev was used in af_packet (where it was replaced) and by
>>bonding ptype handlers (which are no longer with us) only, orig_dev
>>parameter is no longer needed -> toss it out
>>
>>Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>
>[snip]
>>- new->deliver_no_wcard = old->deliver_no_wcard;
>
>deliver_no_wcard was removed from sk_buff in your #7 patch. Shouldn't
>the above line be part of the #7 patch?
Indeed, I wonder how if got here. Will repost to correct this.
>
>Except from this point,
>
>Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
>
> Nicolas.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox