* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Dmitry Torokhov @ 2014-09-11 20:23 UTC (permalink / raw)
To: James Bottomley
Cc: Tejun Heo, Luis R. Rodriguez, Lennart Poettering, Kay Sievers,
Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai, Arjan van de Ven,
linux-kernel@vger.kernel.org, Oleg Nesterov, hare, Andrew Morton,
Tetsuo Handa, Joseph Salisbury, Benjamin Poirier,
Santosh Rastapur, One Thousand Gnomes, Tim Gardner,
Pierre Fersing, Nagalakshmi Nandigama, Praveen Krishnamoorthy
In-Reply-To: <1410465565.10549.19.camel@jarvis>
On Thu, Sep 11, 2014 at 12:59:25PM -0700, James Bottomley wrote:
>
> On Tue, 2014-09-09 at 16:01 -0700, Dmitry Torokhov wrote:
> > On Tuesday, September 09, 2014 03:46:23 PM James Bottomley wrote:
> > > On Wed, 2014-09-10 at 07:41 +0900, Tejun Heo wrote:
> > > >
> > > > The thing is that we have to have dynamic mechanism to listen for
> > > > device attachments no matter what and such mechanism has been in place
> > > > for a long time at this point. The synchronous wait simply doesn't
> > > > serve any purpose anymore and kinda gets in the way in that it makes
> > > > it a possibly extremely slow process to tell whether loading of a
> > > > module succeeded or not because the wait for the initial round of
> > > > probe is piggybacked.
> > >
> > > OK, so we just fire and forget in userland ... why bother inventing an
> > > elaborate new infrastructure in the kernel to do exactly what
> > >
> > > modprobe <mod> &
> > >
> > > would do?
> >
> > Just so we do not forget: we also want the no-modules case to also be able
> > to probe asynchronously so that a slow device does not stall kernel booting.
>
> Yes, but we mostly do this anyway. SCSI for instance does asynchronous
> scanning of attached devices (once the cards are probed)
What would it do it card was a bit slow to probe?
> but has a sync
> point for ordering.
Quite often we do not really care about ordering of devices. I mean,
does it matter if your mouse is discovered before your keyboard or
after?
>
> The problem of speeding up boot is different from the one of init
> processes killing modprobes.
Right. One is systemd doing stupid things, another is kernel could be
smarter.
> There are elements in common, but by and
> large the biggest headaches at least in large device number boots have
> already been tackled by the enterprise crowd (they don't like their
> S390's or 1024 core NUMA systems taking half an hour to come up).
Please do not position this as a mostly solved large systems problem,
For us it is touchpad detection stalling kernel for 0.5-1 sec. Which is
a lot given that we boot in seconds.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-11 20:33 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <5411FC42.3070505-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thu, Sep 11, 2014 at 12:47 PM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/10/2014 07:32 PM, Alexei Starovoitov wrote:
>>
>> On Wed, Sep 10, 2014 at 2:03 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> wrote:
>>>>
>>>> struct { /* anonymous struct used by BPF_PROG_LOAD command
>>>> */
>>>> enum bpf_prog_type prog_type;
>>>> __u32 insn_cnt;
>>>> const struct bpf_insn *insns;
>>>> const char *license;
>>>> __u32 log_level; /* verbosity level of
>>>> eBPF verifier */
>>>> __u32 log_size; /* size of user buffer
>>>> */
>>>> void *log_buf; /* user supplied
>>>> buffer
>>>> */
>>>
>>>
>>>
>>> What is log buffer? Would that mean the verifier will return an error
>>> string if the program will not pass it, or if not, what other data?
>>> I think the man page is missing how to examine the returned verifier
>>> log buffer data.
>>
>>
>> yes. it's an error log (as text string for humans) from verifier.
>
> I was confused due to the void pointer. But that also means that the text
ahh. ok. will change it to 'char *' then.
> string becomes part of the ABI; aren't eBPF specific error codes (perhaps
> a tuple of [line + error code]), though ugly as well, but perhaps the better
> solution to this [which user space can then map to an actual string]?
the verifier log contains full trace. Last unsafe instruction + error
in many cases is useless. What we found empirically from using
it over last 2 years is that developers have different learning curve
to adjust to 'safe' style of C. Pretty much everyone couldn't
figure out why program is rejected based on last error. Therefore
verifier emits full log. From the 1st insn all the way till the last
'unsafe' instruction. So the log is multiline output.
'Understanding eBPF verifier messages' section of
Documentation/networking/filter.txt provides few trivial
examples of these multiline messages.
Like for the program:
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
BPF_LD_MAP_FD(BPF_REG_1, 0),
BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
BPF_EXIT_INSN(),
the verifier log_buf is:
0: (7a) *(u64 *)(r10 -8) = 0
1: (bf) r2 = r10
2: (07) r2 += -8
3: (b7) r1 = 0
4: (85) call 1
5: (15) if r0 == 0x0 goto pc+1
R0=map_ptr R10=fp
6: (7a) *(u64 *)(r0 +4) = 0
misaligned access off 4 size 8
It will surely change over time as verifier becomes smarter,
supports new types, optimizations and so on.
So this log is not an ABI. It's for humans to read.
The log explains _how_ verifier came to conclusion
that the program is unsafe.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-11 20:35 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andy Lutomirski, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <5411FDE1.3060302-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thu, Sep 11, 2014 at 12:54 PM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/10/2014 10:21 PM, Alexei Starovoitov wrote:
> ...
>>>>
>>>> char bpf_log_buf[LOG_BUF_SIZE];
>>>
>>>
>>> What happens if the size isn't LOG_BUF_SIZE?
>>
>>
>> would do you mean?
>> LOG_BUF_SIZE is just a user defined macro.
>> Can be anything.
>
> I believe, Andy means, what would happen if log_level > 0 but
> the log_size is not big enough so that the human-readable error
> text string from the verifier will fit into it? I presume that
> will be truncated then ... hm.
Correct. It will be truncated and ENOSPC returned from syscall.
Just noticed that I didn't mention that in the manpage... will do.
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Luis R. Rodriguez @ 2014-09-11 20:42 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: James Bottomley, Tejun Heo, Lennart Poettering, Kay Sievers,
Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai, Arjan van de Ven,
linux-kernel@vger.kernel.org, Oleg Nesterov, hare, Andrew Morton,
Tetsuo Handa, Joseph Salisbury, Benjamin Poirier,
Santosh Rastapur, One Thousand Gnomes, Tim Gardner,
Pierre Fersing, Nagalakshmi Nandigama,
Praveen Krishnamoorthy <praveen.krishn
In-Reply-To: <20140911202354.GA2598@core.coreip.homeip.net>
On Thu, Sep 11, 2014 at 1:23 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
>> There are elements in common, but by and
>> large the biggest headaches at least in large device number boots have
>> already been tackled by the enterprise crowd (they don't like their
>> S390's or 1024 core NUMA systems taking half an hour to come up).
>
> Please do not position this as a mostly solved large systems problem,
> For us it is touchpad detection stalling kernel for 0.5-1 sec. Which is
> a lot given that we boot in seconds.
Dmitry, would working on top of the aysnc series be reasonable? Then
we could address these as separate things which we'd build on top of.
The one aspect I see us needing to share is the "async probe universe
is OK" flag.
Luis
^ permalink raw reply
* [PATCH net-next] Allow to set net namespace for wireless device via RTM_LINK
From: Vadim Kochan @ 2014-09-11 20:35 UTC (permalink / raw)
To: netdev; +Cc: linux-wireless, Vadim Kochan
Added new netdev_ops callback for setting namespace in specific
for this device way
Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
include/linux/netdevice.h | 4 ++++
include/net/cfg80211.h | 3 +++
net/core/rtnetlink.c | 7 ++++++-
net/mac80211/iface.c | 6 ++++++
net/wireless/core.c | 8 ++++++--
5 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ba72f6b..e5cc49c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -997,6 +997,8 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
* Callback to use for xmit over the accelerated station. This
* is used in place of ndo_start_xmit on accelerated net
* devices.
+ * int (*ndo_set_netns)(struct net_device *dev, struct net *net);
+ * Callback to set net namespace in specific way for this device.
*/
struct net_device_ops {
int (*ndo_init)(struct net_device *dev);
@@ -1146,6 +1148,8 @@ struct net_device_ops {
struct net_device *dev,
void *priv);
int (*ndo_get_lock_subclass)(struct net_device *dev);
+ int (*ndo_set_netns)(struct net_device *dev,
+ struct net *net);
};
/**
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ab21299..853f97c 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4844,6 +4844,9 @@ void cfg80211_shutdown_all_interfaces(struct wiphy *wiphy);
/* ethtool helper */
void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info);
+/* cfg80211_wiphy_switch_netns - switch wiphy dev to net namespace */
+int cfg80211_wiphy_switch_netns(struct wiphy *wiphy, struct net *net);
+
/* Logging, debugging and troubleshooting/diagnostic helpers. */
/* wiphy_printk helpers, similar to dev_printk */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a688268..3c2e5e3 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1501,7 +1501,12 @@ static int do_setlink(const struct sk_buff *skb,
err = -EPERM;
goto errout;
}
- err = dev_change_net_namespace(dev, net, ifname);
+
+ if (dev->netdev_ops->ndo_set_netns)
+ err = dev->netdev_ops->ndo_set_netns(dev, net);
+ else
+ err = dev_change_net_namespace(dev, net, ifname);
+
put_net(net);
if (err)
goto errout;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index f75e5f1..7d60367 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1080,6 +1080,11 @@ static u16 ieee80211_netdev_select_queue(struct net_device *dev,
return ieee80211_select_queue(IEEE80211_DEV_TO_SUB_IF(dev), skb);
}
+static int ieee80211_set_netns(struct net_device *dev, struct net *net)
+{
+ return cfg80211_wiphy_switch_netns(dev->ieee80211_ptr->wiphy, net);
+}
+
static const struct net_device_ops ieee80211_dataif_ops = {
.ndo_open = ieee80211_open,
.ndo_stop = ieee80211_stop,
@@ -1089,6 +1094,7 @@ static const struct net_device_ops ieee80211_dataif_ops = {
.ndo_change_mtu = ieee80211_change_mtu,
.ndo_set_mac_address = ieee80211_change_mac,
.ndo_select_queue = ieee80211_netdev_select_queue,
+ .ndo_set_netns = ieee80211_set_netns,
};
static u16 ieee80211_monitor_select_queue(struct net_device *dev,
diff --git a/net/wireless/core.c b/net/wireless/core.c
index c6620aa..f4742e7 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -179,6 +179,12 @@ int cfg80211_switch_netns(struct cfg80211_registered_device *rdev,
return 0;
}
+int cfg80211_wiphy_switch_netns(struct wiphy *wiphy, struct net *net)
+{
+ return cfg80211_switch_netns(wiphy_to_rdev(wiphy), net);
+}
+EXPORT_SYMBOL(cfg80211_wiphy_switch_netns);
+
static void cfg80211_rfkill_poll(struct rfkill *rfkill, void *data)
{
struct cfg80211_registered_device *rdev = data;
@@ -898,8 +904,6 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
wdev->identifier = ++rdev->wdev_id;
list_add_rcu(&wdev->list, &rdev->wdev_list);
rdev->devlist_generation++;
- /* can only change netns with wiphy */
- dev->features |= NETIF_F_NETNS_LOCAL;
if (sysfs_create_link(&dev->dev.kobj, &rdev->wiphy.dev.kobj,
"phy80211")) {
--
2.1.0
^ permalink raw reply related
* [PATCH net-next v2 0/7] bonding: get rid of curr_slave_lock
From: Nikolay Aleksandrov @ 2014-09-11 20:49 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
Hi all,
This is the second patch-set dealing with bond locking and the purpose here
is to convert curr_slave_lock into a spinlock called "mode_lock" which can
be used in the various modes for their specific needs. The first three
patches cleanup the use of curr_slave_lock and prepare it for the
conversion which is done in patch 4 and then the modes that were using
their own locks are converted to use the new "mode_lock" giving us the
opportunity to remove their locks.
This patch-set has been tested in each mode by running enslave/release of
slaves in parallel with traffic transmission and miimon=1 i.e. running
all the time. In fact this lead to the discovery of a subtle bug related to
RCU which will be fixed in -net.
Also did an allmodconfig test just in case :-)
v2: fix bond_3ad_state_machine_handler's use of mode_lock and
curr_slave_lock
Best regards,
Nikolay Aleksandrov
Nikolay Aleksandrov (7):
bonding: 3ad: clean up curr_slave_lock usage
bonding: alb: remove curr_slave_lock
bonding: clean curr_slave_lock use
bonding: convert curr_slave_lock to a spinlock and rename it
bonding: alb: convert to bond->mode_lock
bonding: 3ad: convert to bond->mode_lock
bonding: adjust locking comments
drivers/net/bonding/bond_3ad.c | 84 ++++++--------------
drivers/net/bonding/bond_3ad.h | 1 -
drivers/net/bonding/bond_alb.c | 159 +++++++++----------------------------
drivers/net/bonding/bond_alb.h | 2 -
drivers/net/bonding/bond_debugfs.c | 4 +-
drivers/net/bonding/bond_main.c | 89 ++++-----------------
drivers/net/bonding/bond_options.c | 10 +--
drivers/net/bonding/bonding.h | 16 ++--
8 files changed, 91 insertions(+), 274 deletions(-)
--
1.9.3
^ permalink raw reply
* [PATCH net-next v2 1/7] bonding: 3ad: clean up curr_slave_lock usage
From: Nikolay Aleksandrov @ 2014-09-11 20:49 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410468568-13781-1-git-send-email-nikolay@redhat.com>
Remove the read_lock in bond_3ad_lacpdu_recv() since when the slave is
being released its rx_handler is removed before 3ad unbind, so even if
packets arrive, they won't see the slave in an inconsistent state.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: Only remove the curr_slave_lock from bond_3ad_lacpdu_recv, leave it in
bond_3ad_state_machine_handler
drivers/net/bonding/bond_3ad.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 5d27a6207384..dfd3a7835d17 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2476,20 +2476,16 @@ err_free:
int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
struct slave *slave)
{
- int ret = RX_HANDLER_ANOTHER;
struct lacpdu *lacpdu, _lacpdu;
if (skb->protocol != PKT_TYPE_LACPDU)
- return ret;
+ return RX_HANDLER_ANOTHER;
lacpdu = skb_header_pointer(skb, 0, sizeof(_lacpdu), &_lacpdu);
if (!lacpdu)
- return ret;
+ return RX_HANDLER_ANOTHER;
- read_lock(&bond->curr_slave_lock);
- ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
- read_unlock(&bond->curr_slave_lock);
- return ret;
+ return bond_3ad_rx_indication(lacpdu, slave, skb->len);
}
/**
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v2 2/7] bonding: alb: remove curr_slave_lock
From: Nikolay Aleksandrov @ 2014-09-11 20:49 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410468568-13781-1-git-send-email-nikolay@redhat.com>
First in rlb_teach_disabled_mac_on_primary() it's okay to remove
curr_slave_lock as all callers except bond_alb_monitor() already hold
RTNL, and in case bond_alb_monitor() is executing we can at most have a
period with bad throughput (very unlikely though).
In bond_alb_monitor() it's okay to remove the read_lock as the slave
list is walked with RCU and the worst that could happen is another
transmitter at the same time and thus for a period which currently is 10
seconds (bond_alb.h: BOND_ALB_LP_TICKS).
And bond_alb_handle_active_change() is okay because it's always called
with RTNL. Removed the ASSERT_RTNL() because it'll be inserted in the
parent function in a following patch.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_alb.c | 39 +++------------------------------------
1 file changed, 3 insertions(+), 36 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 028496205f39..cf4ede8594ff 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -447,7 +447,7 @@ static struct slave *__rlb_next_rx_slave(struct bonding *bond)
/* teach the switch the mac of a disabled slave
* on the primary for fault tolerance
*
- * Caller must hold bond->curr_slave_lock for write or bond lock for write
+ * Caller must hold RTNL
*/
static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
{
@@ -512,12 +512,8 @@ static void rlb_clear_slave(struct bonding *bond, struct slave *slave)
_unlock_rx_hashtbl_bh(bond);
- write_lock_bh(&bond->curr_slave_lock);
-
if (slave != bond_deref_active_protected(bond))
rlb_teach_disabled_mac_on_primary(bond, slave->dev->dev_addr);
-
- write_unlock_bh(&bond->curr_slave_lock);
}
static void rlb_update_client(struct rlb_client_info *client_info)
@@ -1595,13 +1591,6 @@ void bond_alb_monitor(struct work_struct *work)
if (bond_info->lp_counter >= BOND_ALB_LP_TICKS(bond)) {
bool strict_match;
- /* change of curr_active_slave involves swapping of mac addresses.
- * in order to avoid this swapping from happening while
- * sending the learning packets, the curr_slave_lock must be held for
- * read.
- */
- read_lock(&bond->curr_slave_lock);
-
bond_for_each_slave_rcu(bond, slave, iter) {
/* If updating current_active, use all currently
* user mac addreses (!strict_match). Otherwise, only
@@ -1613,17 +1602,11 @@ void bond_alb_monitor(struct work_struct *work)
alb_send_learning_packets(slave, slave->dev->dev_addr,
strict_match);
}
-
- read_unlock(&bond->curr_slave_lock);
-
bond_info->lp_counter = 0;
}
/* rebalance tx traffic */
if (bond_info->tx_rebalance_counter >= BOND_TLB_REBALANCE_TICKS) {
-
- read_lock(&bond->curr_slave_lock);
-
bond_for_each_slave_rcu(bond, slave, iter) {
tlb_clear_slave(bond, slave, 1);
if (slave == rcu_access_pointer(bond->curr_active_slave)) {
@@ -1633,9 +1616,6 @@ void bond_alb_monitor(struct work_struct *work)
bond_info->unbalanced_load = 0;
}
}
-
- read_unlock(&bond->curr_slave_lock);
-
bond_info->tx_rebalance_counter = 0;
}
@@ -1775,21 +1755,14 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char
* Set the bond->curr_active_slave to @new_slave and handle
* mac address swapping and promiscuity changes as needed.
*
- * If new_slave is NULL, caller must hold curr_slave_lock for write
- *
- * If new_slave is not NULL, caller must hold RTNL, curr_slave_lock
- * for write. Processing here may sleep, so no other locks may be held.
+ * Caller must hold RTNL
*/
void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave)
- __releases(&bond->curr_slave_lock)
- __acquires(&bond->curr_slave_lock)
{
struct slave *swap_slave;
struct slave *curr_active;
- curr_active = rcu_dereference_protected(bond->curr_active_slave,
- !new_slave ||
- lockdep_is_held(&bond->curr_slave_lock));
+ curr_active = rtnl_dereference(bond->curr_active_slave);
if (curr_active == new_slave)
return;
@@ -1820,10 +1793,6 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
tlb_clear_slave(bond, swap_slave, 1);
tlb_clear_slave(bond, new_slave, 1);
- write_unlock_bh(&bond->curr_slave_lock);
-
- ASSERT_RTNL();
-
/* in TLB mode, the slave might flip down/up with the old dev_addr,
* and thus filter bond->dev_addr's packets, so force bond's mac
*/
@@ -1852,8 +1821,6 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
alb_send_learning_packets(new_slave, bond->dev->dev_addr,
false);
}
-
- write_lock_bh(&bond->curr_slave_lock);
}
/* Called with RTNL */
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v2 3/7] bonding: clean curr_slave_lock use
From: Nikolay Aleksandrov @ 2014-09-11 20:49 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410468568-13781-1-git-send-email-nikolay@redhat.com>
Mostly all users of curr_slave_lock already have RTNL as we've discussed
previously so there's no point in using it, the one case where the lock
must stay is the 3ad code, in fact it's the only one.
It's okay to remove it from bond_do_fail_over_mac() as it's called with
RTNL and drops the curr_slave_lock anyway.
bond_change_active_slave() is one of the main places where
curr_slave_lock was used, it's okay to remove it as all callers use RTNL
these days before calling it, that's why we move the ASSERT_RTNL() in
the beginning to catch any potential offenders to this rule.
The RTNL argument actually applies to all of the places where
curr_slave_lock has been removed from in this patch.
Also remove the unnecessary bond_deref_active_protected() macro and use
rtnl_dereference() instead.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_alb.c | 4 +--
drivers/net/bonding/bond_main.c | 62 ++++++--------------------------------
drivers/net/bonding/bond_options.c | 10 +-----
drivers/net/bonding/bonding.h | 8 +----
4 files changed, 14 insertions(+), 70 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index cf4ede8594ff..b755659ddfdc 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -451,7 +451,7 @@ static struct slave *__rlb_next_rx_slave(struct bonding *bond)
*/
static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
{
- struct slave *curr_active = bond_deref_active_protected(bond);
+ struct slave *curr_active = rtnl_dereference(bond->curr_active_slave);
if (!curr_active)
return;
@@ -512,7 +512,7 @@ static void rlb_clear_slave(struct bonding *bond, struct slave *slave)
_unlock_rx_hashtbl_bh(bond);
- if (slave != bond_deref_active_protected(bond))
+ if (slave != rtnl_dereference(bond->curr_active_slave))
rlb_teach_disabled_mac_on_primary(bond, slave->dev->dev_addr);
}
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b43b2df9e5d1..3b06685260b8 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -637,13 +637,11 @@ static void bond_set_dev_addr(struct net_device *bond_dev,
*
* Perform special MAC address swapping for fail_over_mac settings
*
- * Called with RTNL, curr_slave_lock for write_bh.
+ * Called with RTNL
*/
static void bond_do_fail_over_mac(struct bonding *bond,
struct slave *new_active,
struct slave *old_active)
- __releases(&bond->curr_slave_lock)
- __acquires(&bond->curr_slave_lock)
{
u8 tmp_mac[ETH_ALEN];
struct sockaddr saddr;
@@ -651,11 +649,8 @@ static void bond_do_fail_over_mac(struct bonding *bond,
switch (bond->params.fail_over_mac) {
case BOND_FOM_ACTIVE:
- if (new_active) {
- write_unlock_bh(&bond->curr_slave_lock);
+ if (new_active)
bond_set_dev_addr(bond->dev, new_active->dev);
- write_lock_bh(&bond->curr_slave_lock);
- }
break;
case BOND_FOM_FOLLOW:
/*
@@ -666,8 +661,6 @@ static void bond_do_fail_over_mac(struct bonding *bond,
if (!new_active)
return;
- write_unlock_bh(&bond->curr_slave_lock);
-
if (old_active) {
ether_addr_copy(tmp_mac, new_active->dev->dev_addr);
ether_addr_copy(saddr.sa_data,
@@ -696,7 +689,6 @@ static void bond_do_fail_over_mac(struct bonding *bond,
netdev_err(bond->dev, "Error %d setting MAC of slave %s\n",
-rv, new_active->dev->name);
out:
- write_lock_bh(&bond->curr_slave_lock);
break;
default:
netdev_err(bond->dev, "bond_do_fail_over_mac impossible: bad policy %d\n",
@@ -709,7 +701,7 @@ out:
static bool bond_should_change_active(struct bonding *bond)
{
struct slave *prim = rtnl_dereference(bond->primary_slave);
- struct slave *curr = bond_deref_active_protected(bond);
+ struct slave *curr = rtnl_dereference(bond->curr_active_slave);
if (!prim || !curr || curr->link != BOND_LINK_UP)
return true;
@@ -785,15 +777,15 @@ static bool bond_should_notify_peers(struct bonding *bond)
* because it is apparently the best available slave we have, even though its
* updelay hasn't timed out yet.
*
- * If new_active is not NULL, caller must hold curr_slave_lock for write_bh.
+ * Caller must hold RTNL.
*/
void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
{
struct slave *old_active;
- old_active = rcu_dereference_protected(bond->curr_active_slave,
- !new_active ||
- lockdep_is_held(&bond->curr_slave_lock));
+ ASSERT_RTNL();
+
+ old_active = rtnl_dereference(bond->curr_active_slave);
if (old_active == new_active)
return;
@@ -861,14 +853,10 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
bond_should_notify_peers(bond);
}
- write_unlock_bh(&bond->curr_slave_lock);
-
call_netdevice_notifiers(NETDEV_BONDING_FAILOVER, bond->dev);
if (should_notify_peers)
call_netdevice_notifiers(NETDEV_NOTIFY_PEERS,
bond->dev);
-
- write_lock_bh(&bond->curr_slave_lock);
}
}
@@ -893,7 +881,7 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
* - The primary_slave has got its link back.
* - A slave has got its link back and there's no old curr_active_slave.
*
- * Caller must hold curr_slave_lock for write_bh.
+ * Caller must hold RTNL.
*/
void bond_select_active_slave(struct bonding *bond)
{
@@ -901,7 +889,7 @@ void bond_select_active_slave(struct bonding *bond)
int rv;
best_slave = bond_find_best_slave(bond);
- if (best_slave != bond_deref_active_protected(bond)) {
+ if (best_slave != rtnl_dereference(bond->curr_active_slave)) {
bond_change_active_slave(bond, best_slave);
rv = bond_set_carrier(bond);
if (!rv)
@@ -1571,9 +1559,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
if (bond_uses_primary(bond)) {
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
bond_select_active_slave(bond);
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
}
@@ -1601,10 +1587,8 @@ err_detach:
RCU_INIT_POINTER(bond->primary_slave, NULL);
if (rcu_access_pointer(bond->curr_active_slave) == new_slave) {
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
bond_change_active_slave(bond, NULL);
bond_select_active_slave(bond);
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
}
/* either primary_slave or curr_active_slave might've changed */
@@ -1720,11 +1704,8 @@ static int __bond_release_one(struct net_device *bond_dev,
if (rtnl_dereference(bond->primary_slave) == slave)
RCU_INIT_POINTER(bond->primary_slave, NULL);
- if (oldcurrent == slave) {
- write_lock_bh(&bond->curr_slave_lock);
+ if (oldcurrent == slave)
bond_change_active_slave(bond, NULL);
- write_unlock_bh(&bond->curr_slave_lock);
- }
if (bond_is_lb(bond)) {
/* Must be called only after the slave has been
@@ -1743,11 +1724,7 @@ static int __bond_release_one(struct net_device *bond_dev,
* is no concern that another slave add/remove event
* will interfere.
*/
- write_lock_bh(&bond->curr_slave_lock);
-
bond_select_active_slave(bond);
-
- write_unlock_bh(&bond->curr_slave_lock);
}
if (!bond_has_slaves(bond)) {
@@ -2058,9 +2035,7 @@ static void bond_miimon_commit(struct bonding *bond)
do_failover:
ASSERT_RTNL();
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
bond_select_active_slave(bond);
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
}
@@ -2506,15 +2481,8 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
if (slave_state_changed) {
bond_slave_state_change(bond);
} else if (do_failover) {
- /* the bond_select_active_slave must hold RTNL
- * and curr_slave_lock for write.
- */
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
-
bond_select_active_slave(bond);
-
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
}
rtnl_unlock();
@@ -2670,9 +2638,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
do_failover:
ASSERT_RTNL();
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
bond_select_active_slave(bond);
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
}
@@ -2939,9 +2905,7 @@ static int bond_slave_netdev_event(unsigned long event,
primary ? slave_dev->name : "none");
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
bond_select_active_slave(bond);
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
break;
case NETDEV_FEAT_CHANGE:
@@ -3106,7 +3070,6 @@ static int bond_open(struct net_device *bond_dev)
/* reset slave->backup and slave->inactive */
if (bond_has_slaves(bond)) {
- read_lock(&bond->curr_slave_lock);
bond_for_each_slave(bond, slave, iter) {
if (bond_uses_primary(bond) &&
slave != rcu_access_pointer(bond->curr_active_slave)) {
@@ -3117,7 +3080,6 @@ static int bond_open(struct net_device *bond_dev)
BOND_SLAVE_NOTIFY_NOW);
}
}
- read_unlock(&bond->curr_slave_lock);
}
bond_work_init_all(bond);
@@ -3239,14 +3201,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
if (!mii)
return -EINVAL;
-
if (mii->reg_num == 1) {
mii->val_out = 0;
- read_lock(&bond->curr_slave_lock);
if (netif_carrier_ok(bond->dev))
mii->val_out = BMSR_LSTATUS;
-
- read_unlock(&bond->curr_slave_lock);
}
return 0;
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 534c0600484e..b62697f4a3de 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -734,15 +734,13 @@ static int bond_option_active_slave_set(struct bonding *bond,
}
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
-
/* check to see if we are clearing active */
if (!slave_dev) {
netdev_info(bond->dev, "Clearing current active slave\n");
RCU_INIT_POINTER(bond->curr_active_slave, NULL);
bond_select_active_slave(bond);
} else {
- struct slave *old_active = bond_deref_active_protected(bond);
+ struct slave *old_active = rtnl_dereference(bond->curr_active_slave);
struct slave *new_active = bond_slave_get_rtnl(slave_dev);
BUG_ON(!new_active);
@@ -765,8 +763,6 @@ static int bond_option_active_slave_set(struct bonding *bond,
}
}
}
-
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
return ret;
@@ -1066,7 +1062,6 @@ static int bond_option_primary_set(struct bonding *bond,
struct slave *slave;
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
p = strchr(primary, '\n');
if (p)
@@ -1103,7 +1098,6 @@ static int bond_option_primary_set(struct bonding *bond,
primary, bond->dev->name);
out:
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
return 0;
@@ -1117,9 +1111,7 @@ static int bond_option_primary_reselect_set(struct bonding *bond,
bond->params.primary_reselect = newval->value;
block_netpoll_tx();
- write_lock_bh(&bond->curr_slave_lock);
bond_select_active_slave(bond);
- write_unlock_bh(&bond->curr_slave_lock);
unblock_netpoll_tx();
return 0;
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 78c461abaa09..02afdeb08765 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -184,9 +184,7 @@ struct slave {
/*
* Here are the locking policies for the two bonding locks:
- *
- * 1) Get rcu_read_lock when reading or RTNL when writing slave list.
- * 2) Get bond->curr_slave_lock when reading/writing bond->curr_active_slave.
+ * Get rcu_read_lock when reading or RTNL when writing slave list.
*/
struct bonding {
struct net_device *dev; /* first - useful for panic debug */
@@ -227,10 +225,6 @@ struct bonding {
#define bond_slave_get_rtnl(dev) \
((struct slave *) rtnl_dereference(dev->rx_handler_data))
-#define bond_deref_active_protected(bond) \
- rcu_dereference_protected(bond->curr_active_slave, \
- lockdep_is_held(&bond->curr_slave_lock))
-
struct bond_vlan_tag {
__be16 vlan_proto;
unsigned short vlan_id;
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v2 5/7] bonding: alb: convert to bond->mode_lock
From: Nikolay Aleksandrov @ 2014-09-11 20:49 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410468568-13781-1-git-send-email-nikolay@redhat.com>
The ALB/TLB specific spinlocks are no longer necessary as we now have
bond->mode_lock for this purpose, so convert them and remove them from
struct alb_bond_info.
Also remove the unneeded lock/unlock functions and use spin_lock/unlock
directly.
Suggested-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_alb.c | 108 ++++++++++++-------------------------
drivers/net/bonding/bond_alb.h | 2 -
drivers/net/bonding/bond_debugfs.c | 4 +-
drivers/net/bonding/bond_main.c | 10 ----
4 files changed, 35 insertions(+), 89 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index b755659ddfdc..876b97fb55e9 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -100,27 +100,6 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
/*********************** tlb specific functions ***************************/
-static inline void _lock_tx_hashtbl_bh(struct bonding *bond)
-{
- spin_lock_bh(&(BOND_ALB_INFO(bond).tx_hashtbl_lock));
-}
-
-static inline void _unlock_tx_hashtbl_bh(struct bonding *bond)
-{
- spin_unlock_bh(&(BOND_ALB_INFO(bond).tx_hashtbl_lock));
-}
-
-static inline void _lock_tx_hashtbl(struct bonding *bond)
-{
- spin_lock(&(BOND_ALB_INFO(bond).tx_hashtbl_lock));
-}
-
-static inline void _unlock_tx_hashtbl(struct bonding *bond)
-{
- spin_unlock(&(BOND_ALB_INFO(bond).tx_hashtbl_lock));
-}
-
-/* Caller must hold tx_hashtbl lock */
static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
{
if (save_load) {
@@ -167,9 +146,9 @@ static void __tlb_clear_slave(struct bonding *bond, struct slave *slave,
static void tlb_clear_slave(struct bonding *bond, struct slave *slave,
int save_load)
{
- _lock_tx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
__tlb_clear_slave(bond, slave, save_load);
- _unlock_tx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
}
/* Must be called before starting the monitor timer */
@@ -184,14 +163,14 @@ static int tlb_initialize(struct bonding *bond)
if (!new_hashtbl)
return -1;
- _lock_tx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
bond_info->tx_hashtbl = new_hashtbl;
for (i = 0; i < TLB_HASH_TABLE_SIZE; i++)
tlb_init_table_entry(&bond_info->tx_hashtbl[i], 0);
- _unlock_tx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
return 0;
}
@@ -202,12 +181,12 @@ static void tlb_deinitialize(struct bonding *bond)
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
struct tlb_up_slave *arr;
- _lock_tx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
kfree(bond_info->tx_hashtbl);
bond_info->tx_hashtbl = NULL;
- _unlock_tx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
arr = rtnl_dereference(bond_info->slave_arr);
if (arr)
@@ -281,7 +260,6 @@ static struct slave *__tlb_choose_channel(struct bonding *bond, u32 hash_index,
return assigned_slave;
}
-/* Caller must hold bond lock for read */
static struct slave *tlb_choose_channel(struct bonding *bond, u32 hash_index,
u32 skb_len)
{
@@ -291,32 +269,13 @@ static struct slave *tlb_choose_channel(struct bonding *bond, u32 hash_index,
* tlb_choose_channel() is only called by bond_alb_xmit()
* which already has softirq disabled.
*/
- _lock_tx_hashtbl(bond);
+ spin_lock(&bond->mode_lock);
tx_slave = __tlb_choose_channel(bond, hash_index, skb_len);
- _unlock_tx_hashtbl(bond);
+ spin_unlock(&bond->mode_lock);
return tx_slave;
}
/*********************** rlb specific functions ***************************/
-static inline void _lock_rx_hashtbl_bh(struct bonding *bond)
-{
- spin_lock_bh(&(BOND_ALB_INFO(bond).rx_hashtbl_lock));
-}
-
-static inline void _unlock_rx_hashtbl_bh(struct bonding *bond)
-{
- spin_unlock_bh(&(BOND_ALB_INFO(bond).rx_hashtbl_lock));
-}
-
-static inline void _lock_rx_hashtbl(struct bonding *bond)
-{
- spin_lock(&(BOND_ALB_INFO(bond).rx_hashtbl_lock));
-}
-
-static inline void _unlock_rx_hashtbl(struct bonding *bond)
-{
- spin_unlock(&(BOND_ALB_INFO(bond).rx_hashtbl_lock));
-}
/* when an ARP REPLY is received from a client update its info
* in the rx_hashtbl
@@ -327,7 +286,7 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
struct rlb_client_info *client_info;
u32 hash_index;
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
hash_index = _simple_hash((u8 *)&(arp->ip_src), sizeof(arp->ip_src));
client_info = &(bond_info->rx_hashtbl[hash_index]);
@@ -342,7 +301,7 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
bond_info->rx_ntt = 1;
}
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
}
static int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
@@ -479,7 +438,7 @@ static void rlb_clear_slave(struct bonding *bond, struct slave *slave)
u32 index, next_index;
/* clear slave from rx_hashtbl */
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
rx_hash_table = bond_info->rx_hashtbl;
index = bond_info->rx_hashtbl_used_head;
@@ -510,7 +469,7 @@ static void rlb_clear_slave(struct bonding *bond, struct slave *slave)
}
}
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
if (slave != rtnl_dereference(bond->curr_active_slave))
rlb_teach_disabled_mac_on_primary(bond, slave->dev->dev_addr);
@@ -561,7 +520,7 @@ static void rlb_update_rx_clients(struct bonding *bond)
struct rlb_client_info *client_info;
u32 hash_index;
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
hash_index = bond_info->rx_hashtbl_used_head;
for (; hash_index != RLB_NULL_INDEX;
@@ -579,7 +538,7 @@ static void rlb_update_rx_clients(struct bonding *bond)
*/
bond_info->rlb_update_delay_counter = RLB_UPDATE_DELAY;
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
}
/* The slave was assigned a new mac address - update the clients */
@@ -590,7 +549,7 @@ static void rlb_req_update_slave_clients(struct bonding *bond, struct slave *sla
int ntt = 0;
u32 hash_index;
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
hash_index = bond_info->rx_hashtbl_used_head;
for (; hash_index != RLB_NULL_INDEX;
@@ -611,7 +570,7 @@ static void rlb_req_update_slave_clients(struct bonding *bond, struct slave *sla
bond_info->rlb_update_retry_counter = RLB_UPDATE_RETRY;
}
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
}
/* mark all clients using src_ip to be updated */
@@ -621,7 +580,7 @@ static void rlb_req_update_subnet_clients(struct bonding *bond, __be32 src_ip)
struct rlb_client_info *client_info;
u32 hash_index;
- _lock_rx_hashtbl(bond);
+ spin_lock(&bond->mode_lock);
hash_index = bond_info->rx_hashtbl_used_head;
for (; hash_index != RLB_NULL_INDEX;
@@ -645,10 +604,9 @@ static void rlb_req_update_subnet_clients(struct bonding *bond, __be32 src_ip)
}
}
- _unlock_rx_hashtbl(bond);
+ spin_unlock(&bond->mode_lock);
}
-/* Caller must hold both bond and ptr locks for read */
static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bond)
{
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
@@ -657,7 +615,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
struct rlb_client_info *client_info;
u32 hash_index = 0;
- _lock_rx_hashtbl(bond);
+ spin_lock(&bond->mode_lock);
curr_active_slave = rcu_dereference(bond->curr_active_slave);
@@ -676,7 +634,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
assigned_slave = client_info->slave;
if (assigned_slave) {
- _unlock_rx_hashtbl(bond);
+ spin_unlock(&bond->mode_lock);
return assigned_slave;
}
} else {
@@ -738,7 +696,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
}
}
- _unlock_rx_hashtbl(bond);
+ spin_unlock(&bond->mode_lock);
return assigned_slave;
}
@@ -800,7 +758,7 @@ static void rlb_rebalance(struct bonding *bond)
int ntt;
u32 hash_index;
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
ntt = 0;
hash_index = bond_info->rx_hashtbl_used_head;
@@ -818,7 +776,7 @@ static void rlb_rebalance(struct bonding *bond)
/* update the team's flag only after the whole iteration */
if (ntt)
bond_info->rx_ntt = 1;
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
}
/* Caller must hold rx_hashtbl lock */
@@ -917,7 +875,7 @@ static void rlb_purge_src_ip(struct bonding *bond, struct arp_pkt *arp)
u32 ip_src_hash = _simple_hash((u8 *)&(arp->ip_src), sizeof(arp->ip_src));
u32 index;
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
index = bond_info->rx_hashtbl[ip_src_hash].src_first;
while (index != RLB_NULL_INDEX) {
@@ -928,7 +886,7 @@ static void rlb_purge_src_ip(struct bonding *bond, struct arp_pkt *arp)
rlb_delete_table_entry(bond, index);
index = next_index;
}
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
}
static int rlb_initialize(struct bonding *bond)
@@ -942,7 +900,7 @@ static int rlb_initialize(struct bonding *bond)
if (!new_hashtbl)
return -1;
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
bond_info->rx_hashtbl = new_hashtbl;
@@ -951,7 +909,7 @@ static int rlb_initialize(struct bonding *bond)
for (i = 0; i < RLB_HASH_TABLE_SIZE; i++)
rlb_init_table_entry(bond_info->rx_hashtbl + i);
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
/* register to receive ARPs */
bond->recv_probe = rlb_arp_recv;
@@ -963,13 +921,13 @@ static void rlb_deinitialize(struct bonding *bond)
{
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
kfree(bond_info->rx_hashtbl);
bond_info->rx_hashtbl = NULL;
bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
}
static void rlb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
@@ -977,7 +935,7 @@ static void rlb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
u32 curr_index;
- _lock_rx_hashtbl_bh(bond);
+ spin_lock_bh(&bond->mode_lock);
curr_index = bond_info->rx_hashtbl_used_head;
while (curr_index != RLB_NULL_INDEX) {
@@ -990,7 +948,7 @@ static void rlb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
curr_index = next_index;
}
- _unlock_rx_hashtbl_bh(bond);
+ spin_unlock_bh(&bond->mode_lock);
}
/*********************** tlb/rlb shared functions *********************/
@@ -1394,9 +1352,9 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
}
if (tx_slave && bond->params.tlb_dynamic_lb) {
- _lock_tx_hashtbl(bond);
+ spin_lock(&bond->mode_lock);
__tlb_clear_slave(bond, tx_slave, 0);
- _unlock_tx_hashtbl(bond);
+ spin_unlock(&bond->mode_lock);
}
/* no suitable interface, frame not sent */
diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
index aaeac61d03cf..3c6a7ff974d7 100644
--- a/drivers/net/bonding/bond_alb.h
+++ b/drivers/net/bonding/bond_alb.h
@@ -147,7 +147,6 @@ struct tlb_up_slave {
struct alb_bond_info {
struct tlb_client_info *tx_hashtbl; /* Dynamically allocated */
- spinlock_t tx_hashtbl_lock;
u32 unbalanced_load;
int tx_rebalance_counter;
int lp_counter;
@@ -156,7 +155,6 @@ struct alb_bond_info {
/* -------- rlb parameters -------- */
int rlb_enabled;
struct rlb_client_info *rx_hashtbl; /* Receive hash table */
- spinlock_t rx_hashtbl_lock;
u32 rx_hashtbl_used_head;
u8 rx_ntt; /* flag - need to transmit
* to all rx clients
diff --git a/drivers/net/bonding/bond_debugfs.c b/drivers/net/bonding/bond_debugfs.c
index 280971b227ea..652f6c5d1bf7 100644
--- a/drivers/net/bonding/bond_debugfs.c
+++ b/drivers/net/bonding/bond_debugfs.c
@@ -29,7 +29,7 @@ static int bond_debug_rlb_hash_show(struct seq_file *m, void *v)
seq_printf(m, "SourceIP DestinationIP "
"Destination MAC DEV\n");
- spin_lock_bh(&(BOND_ALB_INFO(bond).rx_hashtbl_lock));
+ spin_lock_bh(&bond->mode_lock);
hash_index = bond_info->rx_hashtbl_used_head;
for (; hash_index != RLB_NULL_INDEX;
@@ -42,7 +42,7 @@ static int bond_debug_rlb_hash_show(struct seq_file *m, void *v)
client_info->slave->dev->name);
}
- spin_unlock_bh(&(BOND_ALB_INFO(bond).rx_hashtbl_lock));
+ spin_unlock_bh(&bond->mode_lock);
return 0;
}
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 99d21c2fd44f..e06251417a7d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4297,19 +4297,9 @@ static int bond_init(struct net_device *bond_dev)
{
struct bonding *bond = netdev_priv(bond_dev);
struct bond_net *bn = net_generic(dev_net(bond_dev), bond_net_id);
- struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
netdev_dbg(bond_dev, "Begin bond_init\n");
- /*
- * Initialize locks that may be required during
- * en/deslave operations. All of the bond_open work
- * (of which this is part) should really be moved to
- * a phase prior to dev_open
- */
- spin_lock_init(&(bond_info->tx_hashtbl_lock));
- spin_lock_init(&(bond_info->rx_hashtbl_lock));
-
bond->wq = create_singlethread_workqueue(bond_dev->name);
if (!bond->wq)
return -ENOMEM;
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v2 4/7] bonding: convert curr_slave_lock to a spinlock and rename it
From: Nikolay Aleksandrov @ 2014-09-11 20:49 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410468568-13781-1-git-send-email-nikolay@redhat.com>
curr_slave_lock is now a misleading name, a much better name is
mode_lock as it'll be used for each mode's purposes and it's no longer
necessary to use a rwlock, a simple spinlock is enough.
Suggested-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 4 ++--
drivers/net/bonding/bond_main.c | 7 +++----
drivers/net/bonding/bonding.h | 2 +-
3 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index dfd3a7835d17..1824d1df4d09 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2057,7 +2057,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
struct port *port;
bool should_notify_rtnl = BOND_SLAVE_NOTIFY_LATER;
- read_lock(&bond->curr_slave_lock);
+ spin_lock_bh(&bond->mode_lock);
rcu_read_lock();
/* check if there are any slaves */
@@ -2120,7 +2120,7 @@ re_arm:
}
}
rcu_read_unlock();
- read_unlock(&bond->curr_slave_lock);
+ spin_unlock_bh(&bond->mode_lock);
if (should_notify_rtnl && rtnl_trylock()) {
bond_slave_state_notify(bond);
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3b06685260b8..99d21c2fd44f 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1679,9 +1679,9 @@ static int __bond_release_one(struct net_device *bond_dev,
/* Sync against bond_3ad_rx_indication and
* bond_3ad_state_machine_handler
*/
- write_lock_bh(&bond->curr_slave_lock);
+ spin_lock_bh(&bond->mode_lock);
bond_3ad_unbind_slave(slave);
- write_unlock_bh(&bond->curr_slave_lock);
+ spin_unlock_bh(&bond->mode_lock);
}
netdev_info(bond_dev, "Releasing %s interface %s\n",
@@ -3850,8 +3850,7 @@ void bond_setup(struct net_device *bond_dev)
{
struct bonding *bond = netdev_priv(bond_dev);
- /* initialize rwlocks */
- rwlock_init(&bond->curr_slave_lock);
+ spin_lock_init(&bond->mode_lock);
bond->params = bonding_defaults;
/* Initialize pointers */
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 02afdeb08765..0cda34b827f8 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -195,7 +195,7 @@ struct bonding {
s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
int (*recv_probe)(const struct sk_buff *, struct bonding *,
struct slave *);
- rwlock_t curr_slave_lock;
+ spinlock_t mode_lock;
u8 send_peer_notif;
u8 igmp_retrans;
#ifdef CONFIG_PROC_FS
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v2 6/7] bonding: 3ad: convert to bond->mode_lock
From: Nikolay Aleksandrov @ 2014-09-11 20:49 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410468568-13781-1-git-send-email-nikolay@redhat.com>
Now that we have bond->mode_lock, we can remove the state_machine_lock
and use it in its place. There're no fast paths requiring the per-port
spinlocks so it should be okay to consolidate them into mode_lock.
Also move it inside the unbinding function as we don't want to expose
mode_lock outside of the specific modes.
Suggested-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: remove the extra state machine locking in bond_3ad_state_machine_handler
as now mode_lock is used while the whole function is executing.
drivers/net/bonding/bond_3ad.c | 70 +++++++++++++----------------------------
drivers/net/bonding/bond_3ad.h | 1 -
drivers/net/bonding/bond_main.c | 8 +----
3 files changed, 22 insertions(+), 57 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 1824d1df4d09..2bb360f32a64 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -234,24 +234,6 @@ static inline int __check_agg_selection_timer(struct port *port)
}
/**
- * __get_state_machine_lock - lock the port's state machines
- * @port: the port we're looking at
- */
-static inline void __get_state_machine_lock(struct port *port)
-{
- spin_lock_bh(&(SLAVE_AD_INFO(port->slave)->state_machine_lock));
-}
-
-/**
- * __release_state_machine_lock - unlock the port's state machines
- * @port: the port we're looking at
- */
-static inline void __release_state_machine_lock(struct port *port)
-{
- spin_unlock_bh(&(SLAVE_AD_INFO(port->slave)->state_machine_lock));
-}
-
-/**
* __get_link_speed - get a port's speed
* @port: the port we're looking at
*
@@ -341,16 +323,6 @@ static u8 __get_duplex(struct port *port)
return retval;
}
-/**
- * __initialize_port_locks - initialize a port's STATE machine spinlock
- * @port: the slave of the port we're looking at
- */
-static inline void __initialize_port_locks(struct slave *slave)
-{
- /* make sure it isn't called twice */
- spin_lock_init(&(SLAVE_AD_INFO(slave)->state_machine_lock));
-}
-
/* Conversions */
/**
@@ -1843,7 +1815,6 @@ void bond_3ad_bind_slave(struct slave *slave)
ad_initialize_port(port, bond->params.lacp_fast);
- __initialize_port_locks(slave);
port->slave = slave;
port->actor_port_number = SLAVE_AD_INFO(slave)->id;
/* key is determined according to the link speed, duplex and user key(which
@@ -1899,6 +1870,8 @@ void bond_3ad_unbind_slave(struct slave *slave)
struct slave *slave_iter;
struct list_head *iter;
+ /* Sync against bond_3ad_state_machine_handler() */
+ spin_lock_bh(&bond->mode_lock);
aggregator = &(SLAVE_AD_INFO(slave)->aggregator);
port = &(SLAVE_AD_INFO(slave)->port);
@@ -1906,7 +1879,7 @@ void bond_3ad_unbind_slave(struct slave *slave)
if (!port->slave) {
netdev_warn(bond->dev, "Trying to unbind an uninitialized port on %s\n",
slave->dev->name);
- return;
+ goto out;
}
netdev_dbg(bond->dev, "Unbinding Link Aggregation Group %d\n",
@@ -2032,6 +2005,9 @@ void bond_3ad_unbind_slave(struct slave *slave)
}
}
port->slave = NULL;
+
+out:
+ spin_unlock_bh(&bond->mode_lock);
}
/**
@@ -2057,6 +2033,10 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
struct port *port;
bool should_notify_rtnl = BOND_SLAVE_NOTIFY_LATER;
+ /* Lock to protect data accessed by all (e.g., port->sm_vars) and
+ * against running with bond_3ad_unbind_slave. ad_rx_machine may run
+ * concurrently due to incoming LACPDU as well.
+ */
spin_lock_bh(&bond->mode_lock);
rcu_read_lock();
@@ -2093,12 +2073,6 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
goto re_arm;
}
- /* Lock around state machines to protect data accessed
- * by all (e.g., port->sm_vars). ad_rx_machine may run
- * concurrently due to incoming LACPDU.
- */
- __get_state_machine_lock(port);
-
ad_rx_machine(NULL, port);
ad_periodic_machine(port);
ad_port_selection_logic(port);
@@ -2108,8 +2082,6 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
/* turn off the BEGIN bit, since we already handled it */
if (port->sm_vars & AD_PORT_BEGIN)
port->sm_vars &= ~AD_PORT_BEGIN;
-
- __release_state_machine_lock(port);
}
re_arm:
@@ -2161,9 +2133,9 @@ static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave,
netdev_dbg(slave->bond->dev, "Received LACPDU on port %d\n",
port->actor_port_number);
/* Protect against concurrent state machines */
- __get_state_machine_lock(port);
+ spin_lock(&slave->bond->mode_lock);
ad_rx_machine(lacpdu, port);
- __release_state_machine_lock(port);
+ spin_unlock(&slave->bond->mode_lock);
break;
case AD_TYPE_MARKER:
@@ -2213,7 +2185,7 @@ void bond_3ad_adapter_speed_changed(struct slave *slave)
return;
}
- __get_state_machine_lock(port);
+ spin_lock_bh(&slave->bond->mode_lock);
port->actor_admin_port_key &= ~AD_SPEED_KEY_BITS;
port->actor_oper_port_key = port->actor_admin_port_key |=
@@ -2224,7 +2196,7 @@ void bond_3ad_adapter_speed_changed(struct slave *slave)
*/
port->sm_vars |= AD_PORT_BEGIN;
- __release_state_machine_lock(port);
+ spin_unlock_bh(&slave->bond->mode_lock);
}
/**
@@ -2246,7 +2218,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
return;
}
- __get_state_machine_lock(port);
+ spin_lock_bh(&slave->bond->mode_lock);
port->actor_admin_port_key &= ~AD_DUPLEX_KEY_BITS;
port->actor_oper_port_key = port->actor_admin_port_key |=
@@ -2257,7 +2229,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
*/
port->sm_vars |= AD_PORT_BEGIN;
- __release_state_machine_lock(port);
+ spin_unlock_bh(&slave->bond->mode_lock);
}
/**
@@ -2280,7 +2252,7 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
return;
}
- __get_state_machine_lock(port);
+ spin_lock_bh(&slave->bond->mode_lock);
/* on link down we are zeroing duplex and speed since
* some of the adaptors(ce1000.lan) report full duplex/speed
* instead of N/A(duplex) / 0(speed).
@@ -2311,7 +2283,7 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
*/
port->sm_vars |= AD_PORT_BEGIN;
- __release_state_machine_lock(port);
+ spin_unlock_bh(&slave->bond->mode_lock);
}
/**
@@ -2495,7 +2467,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
* When modify lacp_rate parameter via sysfs,
* update actor_oper_port_state of each port.
*
- * Hold slave->state_machine_lock,
+ * Hold bond->mode_lock,
* so we can modify port->actor_oper_port_state,
* no matter bond is up or down.
*/
@@ -2507,13 +2479,13 @@ void bond_3ad_update_lacp_rate(struct bonding *bond)
int lacp_fast;
lacp_fast = bond->params.lacp_fast;
+ spin_lock_bh(&bond->mode_lock);
bond_for_each_slave(bond, slave, iter) {
port = &(SLAVE_AD_INFO(slave)->port);
- __get_state_machine_lock(port);
if (lacp_fast)
port->actor_oper_port_state |= AD_STATE_LACP_TIMEOUT;
else
port->actor_oper_port_state &= ~AD_STATE_LACP_TIMEOUT;
- __release_state_machine_lock(port);
}
+ spin_unlock_bh(&bond->mode_lock);
}
diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
index bb03b1df2f3e..c5f14ac63f3e 100644
--- a/drivers/net/bonding/bond_3ad.h
+++ b/drivers/net/bonding/bond_3ad.h
@@ -259,7 +259,6 @@ struct ad_bond_info {
struct ad_slave_info {
struct aggregator aggregator; /* 802.3ad aggregator structure */
struct port port; /* 802.3ad port structure */
- spinlock_t state_machine_lock; /* mutex state machines vs. incoming LACPDU */
u16 id;
};
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e06251417a7d..116cf6965bc5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1675,14 +1675,8 @@ static int __bond_release_one(struct net_device *bond_dev,
*/
netdev_rx_handler_unregister(slave_dev);
- if (BOND_MODE(bond) == BOND_MODE_8023AD) {
- /* Sync against bond_3ad_rx_indication and
- * bond_3ad_state_machine_handler
- */
- spin_lock_bh(&bond->mode_lock);
+ if (BOND_MODE(bond) == BOND_MODE_8023AD)
bond_3ad_unbind_slave(slave);
- spin_unlock_bh(&bond->mode_lock);
- }
netdev_info(bond_dev, "Releasing %s interface %s\n",
bond_is_active_slave(slave) ? "active" : "backup",
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v2 7/7] bonding: adjust locking comments
From: Nikolay Aleksandrov @ 2014-09-11 20:49 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410468568-13781-1-git-send-email-nikolay@redhat.com>
Now that locks have been removed, remove some unnecessary comments and
adjust others to reflect reality. Also add a comment to "mode_lock" to
describe its current users and give a brief summary why they need it.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_alb.c | 8 +-------
drivers/net/bonding/bond_main.c | 6 +++---
drivers/net/bonding/bonding.h | 6 ++++++
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 876b97fb55e9..85af961f1317 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -119,7 +119,6 @@ static inline void tlb_init_slave(struct slave *slave)
SLAVE_TLB_INFO(slave).head = TLB_NULL_INDEX;
}
-/* Caller must hold bond lock for read, BH disabled */
static void __tlb_clear_slave(struct bonding *bond, struct slave *slave,
int save_load)
{
@@ -142,7 +141,6 @@ static void __tlb_clear_slave(struct bonding *bond, struct slave *slave,
tlb_init_slave(slave);
}
-/* Caller must hold bond lock for read */
static void tlb_clear_slave(struct bonding *bond, struct slave *slave,
int save_load)
{
@@ -199,7 +197,6 @@ static long long compute_gap(struct slave *slave)
(s64) (SLAVE_TLB_INFO(slave).load << 3); /* Bytes to bits */
}
-/* Caller must hold bond lock for read */
static struct slave *tlb_get_least_loaded_slave(struct bonding *bond)
{
struct slave *slave, *least_loaded;
@@ -337,7 +334,6 @@ out:
return RX_HANDLER_ANOTHER;
}
-/* Caller must hold bond lock for read */
static struct slave *rlb_next_rx_slave(struct bonding *bond)
{
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
@@ -370,7 +366,7 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
return rx_slave;
}
-/* Caller must hold rcu_read_lock() for read */
+/* Caller must hold rcu_read_lock() */
static struct slave *__rlb_next_rx_slave(struct bonding *bond)
{
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
@@ -749,7 +745,6 @@ static struct slave *rlb_arp_xmit(struct sk_buff *skb, struct bonding *bond)
return tx_slave;
}
-/* Caller must hold bond lock for read */
static void rlb_rebalance(struct bonding *bond)
{
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
@@ -1677,7 +1672,6 @@ void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave)
}
-/* Caller must hold bond lock for read */
void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char link)
{
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 116cf6965bc5..2d90a8b7f62e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1629,7 +1629,7 @@ err_undo_flags:
/*
* Try to release the slave device <slave> from the bond device <master>
* It is legal to access curr_active_slave without a lock because all the function
- * is write-locked. If "all" is true it means that the function is being called
+ * is RTNL-locked. If "all" is true it means that the function is being called
* while destroying a bond interface and all slaves are being released.
*
* The rules for slave state should be:
@@ -2494,7 +2494,7 @@ re_arm:
* place for the slave. Returns 0 if no changes are found, >0 if changes
* to link states must be committed.
*
- * Called with rcu_read_lock hold.
+ * Called with rcu_read_lock held.
*/
static int bond_ab_arp_inspect(struct bonding *bond)
{
@@ -2642,7 +2642,7 @@ do_failover:
/*
* Send ARP probes for active-backup mode ARP monitor.
*
- * Called with rcu_read_lock hold.
+ * Called with rcu_read_lock held.
*/
static bool bond_ab_arp_probe(struct bonding *bond)
{
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 0cda34b827f8..3aff1a815e89 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -195,6 +195,12 @@ struct bonding {
s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
int (*recv_probe)(const struct sk_buff *, struct bonding *,
struct slave *);
+ /* mode_lock is used for mode-specific locking needs, currently used by:
+ * 3ad mode (4) - protect against running bond_3ad_unbind_slave() and
+ * bond_3ad_state_machine_handler() concurrently.
+ * TLB mode (5) - to sync the use and modifications of its hash table
+ * ALB mode (6) - to sync the use and modifications of its hash table
+ */
spinlock_t mode_lock;
u8 send_peer_notif;
u8 igmp_retrans;
--
1.9.3
^ permalink raw reply related
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Dmitry Torokhov @ 2014-09-11 20:53 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: James Bottomley, Tejun Heo, Lennart Poettering, Kay Sievers,
Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai, Arjan van de Ven,
linux-kernel@vger.kernel.org, Oleg Nesterov, hare, Andrew Morton,
Tetsuo Handa, Joseph Salisbury, Benjamin Poirier,
Santosh Rastapur, One Thousand Gnomes, Tim Gardner,
Pierre Fersing, Nagalakshmi Nandigama,
Praveen Krishnamoorthy <praveen.krishn
In-Reply-To: <CAB=NE6XfUAm=iVr=qfu5i9QW7BpViXfJ6wsmEZOOmS=rBF1bdA@mail.gmail.com>
On Thu, Sep 11, 2014 at 01:42:20PM -0700, Luis R. Rodriguez wrote:
> On Thu, Sep 11, 2014 at 1:23 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> >> There are elements in common, but by and
> >> large the biggest headaches at least in large device number boots have
> >> already been tackled by the enterprise crowd (they don't like their
> >> S390's or 1024 core NUMA systems taking half an hour to come up).
> >
> > Please do not position this as a mostly solved large systems problem,
> > For us it is touchpad detection stalling kernel for 0.5-1 sec. Which is
> > a lot given that we boot in seconds.
>
> Dmitry, would working on top of the aysnc series be reasonable? Then
> we could address these as separate things which we'd build on top of.
> The one aspect I see us needing to share is the "async probe universe
> is OK" flag.
Sure. Are you planning on refreshing your series? I think the
code-related discussion kind of stalled...
--
Dmitry
^ permalink raw reply
* Re: [net-next v5 1/3] udp-tunnel: Expand UDP tunnel APIs
From: Tom Herbert @ 2014-09-11 21:04 UTC (permalink / raw)
To: Andy Zhou; +Cc: David Miller, Linux Netdev List
In-Reply-To: <1410406193-6185-2-git-send-email-azhou@nicira.com>
On Wed, Sep 10, 2014 at 8:29 PM, Andy Zhou <azhou@nicira.com> wrote:
> Added common udp tunnel socket creation, and packet transmission APIs
> API that can be used by other UDP based tunneling protocol
> implementation.
>
> Signed-off-by: Andy Zhou <azhou@nicira.com>
> ---
> include/net/udp_tunnel.h | 73 +++++++++++++++++++++++++++
> net/ipv4/Kconfig | 1 +
> net/ipv4/udp_tunnel.c | 108 ++++++++++++++++++++++++++--------------
> net/ipv6/Makefile | 1 +
> net/ipv6/ip6_udp_tunnel.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 268 insertions(+), 36 deletions(-)
> create mode 100644 net/ipv6/ip6_udp_tunnel.c
>
> diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
> index ffd69cb..e9dcf83 100644
> --- a/include/net/udp_tunnel.h
> +++ b/include/net/udp_tunnel.h
> @@ -1,6 +1,14 @@
> #ifndef __NET_UDP_TUNNEL_H
> #define __NET_UDP_TUNNEL_H
>
> +#include <net/ip_tunnels.h>
> +#include <net/udp.h>
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +#include <net/ipv6.h>
> +#include <net/addrconf.h>
> +#endif
> +
> struct udp_port_cfg {
> u8 family;
>
> @@ -29,4 +37,69 @@ struct udp_port_cfg {
> int udp_sock_create(struct net *net, struct udp_port_cfg *cfg,
> struct socket **sockp);
>
> +#if IS_ENABLED(CONFIG_IPV6)
> +int udp_sock_create6(struct net *net, struct udp_port_cfg *cfg,
> + struct socket **sockp);
> +#else
> +static inline int udp_sock_create6(struct net *net, struct udp_port_cfg *cfg,
> + struct socket **sockp)
> +{
> + return 0;
> +}
> +#endif
> +
> +struct udp_tunnel_sock;
> +
> +typedef int (*udp_tunnel_encap_rcv_t)(struct sock *sk, struct sk_buff *skb);
> +typedef void (*udp_tunnel_encap_destroy_t)(struct sock *sk);
> +
> +struct udp_tunnel_sock_cfg {
> + struct socket *sock; /* The socket UDP tunnel will attach to */
> + /* Used for setting up udp_sock fields, see udp.h for details */
> + __u8 encap_type;
> + udp_tunnel_encap_rcv_t encap_rcv;
> + udp_tunnel_encap_destroy_t encap_destroy;
> +};
> +
> +struct udp_tunnel_sock {
> + struct socket *sock;
> +};
> +
> +struct udp_tunnel_sock *create_udp_tunnel_sock(struct net *net, size_t size,
> + struct udp_tunnel_sock_cfg
> + *sock_cfg);
> +
> +int udp_tunnel_xmit_skb(struct udp_tunnel_sock *uts, struct rtable *rt,
> + struct sk_buff *skb, __be32 src, __be32 dst,
> + __u8 tos, __u8 ttl, __be16 df, __be16 src_port,
> + __be16 dst_port, bool xnet);
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +int udp_tunnel6_xmit_skb(struct udp_tunnel_sock *uts, struct dst_entry *dst,
> + struct sk_buff *skb, struct net_device *dev,
> + struct in6_addr *saddr, struct in6_addr *daddr,
> + __u8 prio, __u8 ttl, __be16 src_port,
> + __be16 dst_port);
> +#endif
> +
> +void udp_tunnel_sock_release(struct udp_tunnel_sock *uts);
> +void udp_tunnel_sock_free(struct udp_tunnel_sock *uts);
> +
> +static inline struct sk_buff *udp_tunnel_handle_offloads(struct sk_buff *skb,
> + bool udp_csum)
> +{
> + int type = udp_csum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
> +
> + return iptunnel_handle_offloads(skb, udp_csum, type);
> +}
> +
> +static inline void udp_tunnel_encap_enable(struct socket *sock)
> +{
> +#if IS_ENABLED(CONFIG_IPV6)
> + if (sock->sk->sk_family == PF_INET6)
> + ipv6_stub->udpv6_encap_enable();
> + else
> +#endif
> + udp_encap_enable();
> +}
> #endif
> diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
> index dbc10d8..cccb95f 100644
> --- a/net/ipv4/Kconfig
> +++ b/net/ipv4/Kconfig
> @@ -308,6 +308,7 @@ config NET_IPVTI
> on top.
>
> config NET_UDP_TUNNEL
> + depends on (IPV6 || IPV6=n)
> tristate
> default n
>
> diff --git a/net/ipv4/udp_tunnel.c b/net/ipv4/udp_tunnel.c
> index 61ec1a6..d60c1a0 100644
> --- a/net/ipv4/udp_tunnel.c
> +++ b/net/ipv4/udp_tunnel.c
> @@ -14,42 +14,9 @@ int udp_sock_create(struct net *net, struct udp_port_cfg *cfg,
> int err = -EINVAL;
> struct socket *sock = NULL;
>
> -#if IS_ENABLED(CONFIG_IPV6)
> if (cfg->family == AF_INET6) {
> - struct sockaddr_in6 udp6_addr;
> -
> - err = sock_create_kern(AF_INET6, SOCK_DGRAM, 0, &sock);
> - if (err < 0)
> - goto error;
> -
> - sk_change_net(sock->sk, net);
> -
> - udp6_addr.sin6_family = AF_INET6;
> - memcpy(&udp6_addr.sin6_addr, &cfg->local_ip6,
> - sizeof(udp6_addr.sin6_addr));
> - udp6_addr.sin6_port = cfg->local_udp_port;
> - err = kernel_bind(sock, (struct sockaddr *)&udp6_addr,
> - sizeof(udp6_addr));
> - if (err < 0)
> - goto error;
> -
> - if (cfg->peer_udp_port) {
> - udp6_addr.sin6_family = AF_INET6;
> - memcpy(&udp6_addr.sin6_addr, &cfg->peer_ip6,
> - sizeof(udp6_addr.sin6_addr));
> - udp6_addr.sin6_port = cfg->peer_udp_port;
> - err = kernel_connect(sock,
> - (struct sockaddr *)&udp6_addr,
> - sizeof(udp6_addr), 0);
> - }
> - if (err < 0)
> - goto error;
> -
> - udp_set_no_check6_tx(sock->sk, !cfg->use_udp6_tx_checksums);
> - udp_set_no_check6_rx(sock->sk, !cfg->use_udp6_rx_checksums);
> - } else
> -#endif
> - if (cfg->family == AF_INET) {
> + return udp_sock_create6(net, cfg, sockp);
> + } else if (cfg->family == AF_INET) {
> struct sockaddr_in udp_addr;
>
> err = sock_create_kern(AF_INET, SOCK_DGRAM, 0, &sock);
> @@ -82,7 +49,6 @@ int udp_sock_create(struct net *net, struct udp_port_cfg *cfg,
> return -EPFNOSUPPORT;
> }
>
> -
> *sockp = sock;
>
> return 0;
> @@ -97,4 +63,74 @@ error:
> }
> EXPORT_SYMBOL(udp_sock_create);
>
> +struct udp_tunnel_sock *
> +create_udp_tunnel_sock(struct net *net, size_t size,
> + struct udp_tunnel_sock_cfg *cfg)
> +{
> + struct udp_tunnel_sock *uts;
> + struct sock *sk;
> + struct socket *sock = cfg->sock;
> +
> + uts = kzalloc(size, GFP_KERNEL);
> + if (!uts)
> + return ERR_PTR(-ENOMEM);
> +
Allocating memory for the caller seems like overkill to me and there's
no guarantee that is what caller wants anyway (maybe they are using
array of static structures for instance). Seems like it would be just
as easy to return the sock and let caller allocate whatever else it
needs on its own (only field in udp_tunnel_sock is the sock anyway).
> + sk = sock->sk;
> +
> + /* Disable multicast loopback */
> + inet_sk(sk)->mc_loop = 0;
> +
Probably want to enable checksum unnecessary conversions also.
> + rcu_assign_sk_user_data(sk, uts);
> +
> + udp_sk(sk)->encap_type = cfg->encap_type;
> + udp_sk(sk)->encap_rcv = cfg->encap_rcv;
> + udp_sk(sk)->encap_destroy = cfg->encap_destroy;
> +
> + uts->sock = sock;
> +
> + udp_tunnel_encap_enable(sock);
> +
> + return uts;
> +}
> +EXPORT_SYMBOL_GPL(create_udp_tunnel_sock);
> +
> +int udp_tunnel_xmit_skb(struct udp_tunnel_sock *uts, struct rtable *rt,
> + struct sk_buff *skb, __be32 src, __be32 dst,
> + __u8 tos, __u8 ttl, __be16 df, __be16 src_port,
> + __be16 dst_port, bool xnet)
> +{
> + struct udphdr *uh;
> + struct socket *sock = uts->sock;
> +
> + __skb_push(skb, sizeof(*uh));
> + skb_reset_transport_header(skb);
> + uh = udp_hdr(skb);
> +
> + uh->dest = dst_port;
> + uh->source = src_port;
> + uh->len = htons(skb->len);
> +
> + udp_set_csum(sock->sk->sk_no_check_tx, skb, src, dst, skb->len);
> +
> + return iptunnel_xmit(sock->sk, rt, skb, src, dst, IPPROTO_UDP,
> + tos, ttl, df, xnet);
> +}
> +EXPORT_SYMBOL_GPL(udp_tunnel_xmit_skb);
> +
> +void udp_tunnel_sock_release(struct udp_tunnel_sock *uts)
> +{
> + struct sock *sk = uts->sock->sk;
> +
> + rcu_assign_sk_user_data(uts->sock->sk, NULL);
> + kernel_sock_shutdown(uts->sock, SHUT_RDWR);
> + sk_release_kernel(sk);
> +}
> +EXPORT_SYMBOL_GPL(udp_tunnel_sock_release);
> +
> +void udp_tunnel_sock_free(struct udp_tunnel_sock *uts)
> +{
> + kfree(uts);
> +}
> +EXPORT_SYMBOL_GPL(udp_tunnel_sock_free);
> +
> MODULE_LICENSE("GPL");
> diff --git a/net/ipv6/Makefile b/net/ipv6/Makefile
> index 2fe6836..45f830e 100644
> --- a/net/ipv6/Makefile
> +++ b/net/ipv6/Makefile
Probably good to make IPv4 and v6 stuff in separate patches.
> @@ -35,6 +35,7 @@ obj-$(CONFIG_INET6_XFRM_MODE_ROUTEOPTIMIZATION) += xfrm6_mode_ro.o
> obj-$(CONFIG_INET6_XFRM_MODE_BEET) += xfrm6_mode_beet.o
> obj-$(CONFIG_IPV6_MIP6) += mip6.o
> obj-$(CONFIG_NETFILTER) += netfilter/
> +obj-$(CONFIG_NET_UDP_TUNNEL) += ip6_udp_tunnel.o
>
> obj-$(CONFIG_IPV6_VTI) += ip6_vti.o
> obj-$(CONFIG_IPV6_SIT) += sit.o
> diff --git a/net/ipv6/ip6_udp_tunnel.c b/net/ipv6/ip6_udp_tunnel.c
> new file mode 100644
> index 0000000..5109f46
> --- /dev/null
> +++ b/net/ipv6/ip6_udp_tunnel.c
> @@ -0,0 +1,121 @@
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/socket.h>
> +#include <linux/udp.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/in6.h>
> +#include <net/udp.h>
> +#include <net/udp_tunnel.h>
> +#include <net/net_namespace.h>
> +#include <net/netns/generic.h>
> +#include <net/ip6_tunnel.h>
> +#include <net/ip6_checksum.h>
> +
> +int udp_sock_create6(struct net *net, struct udp_port_cfg *cfg,
> + struct socket **sockp)
> +{
> + struct sockaddr_in6 udp6_addr;
> + int err = -EINVAL;
> + struct socket *sock = NULL;
> +
> + err = sock_create_kern(AF_INET6, SOCK_DGRAM, 0, &sock);
> + if (err < 0)
> + goto error;
> +
> + sk_change_net(sock->sk, net);
> +
> + udp6_addr.sin6_family = AF_INET6;
> + memcpy(&udp6_addr.sin6_addr, &cfg->local_ip6,
> + sizeof(udp6_addr.sin6_addr));
> + udp6_addr.sin6_port = cfg->local_udp_port;
> + err = kernel_bind(sock, (struct sockaddr *)&udp6_addr,
> + sizeof(udp6_addr));
> + if (err < 0)
> + goto error;
> +
> + if (cfg->peer_udp_port) {
> + udp6_addr.sin6_family = AF_INET6;
> + memcpy(&udp6_addr.sin6_addr, &cfg->peer_ip6,
> + sizeof(udp6_addr.sin6_addr));
> + udp6_addr.sin6_port = cfg->peer_udp_port;
> + err = kernel_connect(sock,
> + (struct sockaddr *)&udp6_addr,
> + sizeof(udp6_addr), 0);
> + }
> + if (err < 0)
> + goto error;
> +
> + udp_set_no_check6_tx(sock->sk, !cfg->use_udp6_tx_checksums);
> + udp_set_no_check6_rx(sock->sk, !cfg->use_udp6_rx_checksums);
> +
> + *sockp = sock;
> + return 0;
> +
> +error:
> + if (sock) {
> + kernel_sock_shutdown(sock, SHUT_RDWR);
> + sk_release_kernel(sock->sk);
> + }
> + *sockp = NULL;
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(udp_sock_create6);
> +
> +int udp_tunnel6_xmit_skb(struct udp_tunnel_sock *uts, struct dst_entry *dst,
> + struct sk_buff *skb, struct net_device *dev,
> + struct in6_addr *saddr, struct in6_addr *daddr,
> + __u8 prio, __u8 ttl, __be16 src_port, __be16 dst_port)
> +{
> + struct udphdr *uh;
> + struct ipv6hdr *ip6h;
> +
> + __skb_push(skb, sizeof(*uh));
> + skb_reset_transport_header(skb);
> + uh = udp_hdr(skb);
> +
> + uh->dest = dst_port;
> + uh->source = src_port;
> +
> + uh->len = htons(skb->len);
> + uh->check = 0;
> +
> + memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
> + IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED
> + | IPSKB_REROUTED);
> + skb_dst_set(skb, dst);
> +
> + if (!skb_is_gso(skb) && !(dst->dev->features & NETIF_F_IPV6_CSUM)) {
> + __wsum csum = skb_checksum(skb, 0, skb->len, 0);
> +
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + uh->check = csum_ipv6_magic(saddr, daddr, skb->len,
> + IPPROTO_UDP, csum);
> + if (uh->check == 0)
> + uh->check = CSUM_MANGLED_0;
> + } else {
> + skb->ip_summed = CHECKSUM_PARTIAL;
> + skb->csum_start = skb_transport_header(skb) - skb->head;
> + skb->csum_offset = offsetof(struct udphdr, check);
> + uh->check = ~csum_ipv6_magic(saddr, daddr,
> + skb->len, IPPROTO_UDP, 0);
> + }
Can't we call udp6_set_csum for this?
> +
> + __skb_push(skb, sizeof(*ip6h));
> + skb_reset_network_header(skb);
> + ip6h = ipv6_hdr(skb);
> + ip6h->version = 6;
> + ip6h->priority = prio;
> + ip6h->flow_lbl[0] = 0;
> + ip6h->flow_lbl[1] = 0;
> + ip6h->flow_lbl[2] = 0;
> + ip6h->payload_len = htons(skb->len);
> + ip6h->nexthdr = IPPROTO_UDP;
> + ip6h->hop_limit = ttl;
> + ip6h->daddr = *daddr;
> + ip6h->saddr = *saddr;
> +
> + ip6tunnel_xmit(skb, dev);
So iptunnel_xmit creates the IP header, but ip6tunnel_xmit doesn't. It
should be on the TODO list to make this consistent!
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(udp_tunnel6_xmit_skb);
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Luis R. Rodriguez @ 2014-09-11 21:08 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: James Bottomley, Tejun Heo, Lennart Poettering, Kay Sievers,
Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai, Arjan van de Ven,
linux-kernel@vger.kernel.org, Oleg Nesterov, hare, Andrew Morton,
Tetsuo Handa, Joseph Salisbury, Benjamin Poirier,
Santosh Rastapur, One Thousand Gnomes, Tim Gardner,
Pierre Fersing, Nagalakshmi Nandigama,
Praveen Krishnamoorthy <praveen
In-Reply-To: <20140911205308.GB2598@core.coreip.homeip.net>
On Thu, Sep 11, 2014 at 1:53 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Sep 11, 2014 at 01:42:20PM -0700, Luis R. Rodriguez wrote:
>> On Thu, Sep 11, 2014 at 1:23 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> >
>> >> There are elements in common, but by and
>> >> large the biggest headaches at least in large device number boots have
>> >> already been tackled by the enterprise crowd (they don't like their
>> >> S390's or 1024 core NUMA systems taking half an hour to come up).
>> >
>> > Please do not position this as a mostly solved large systems problem,
>> > For us it is touchpad detection stalling kernel for 0.5-1 sec. Which is
>> > a lot given that we boot in seconds.
>>
>> Dmitry, would working on top of the aysnc series be reasonable? Then
>> we could address these as separate things which we'd build on top of.
>> The one aspect I see us needing to share is the "async probe universe
>> is OK" flag.
>
> Sure. Are you planning on refreshing your series?
Yes.
> I think the code-related discussion kind of stalled...
I was just waiting for any possible brain farts to flush out before a
new respin. I'll tackle this now.
Luis
^ permalink raw reply
* URGENT RESPOND NEEDED
From: Mohammed Aziz @ 2014-09-11 21:15 UTC (permalink / raw)
Dear Friend,
I want to transfer US$20.5 Million to your bank account. The fund
belong to our deceased customer who died with his entire family in Iraq
War 2006, leaving nobody for the claim and as such, I decided to
contact you to enable us claim the fund. Your share is 40% while 60%
for me. This transaction is 100% risky free.
Note: reply me on my private email address: ( mohammed_aziz05@voila.fr
) for
security reasons
Thanks,
Mohammed Aziz.
Banque Régionale de Solidarite( BRS),
ouagadougou,Burkina Faso
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Tom Gundersen @ 2014-09-11 21:43 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: One Thousand Gnomes, Takashi Iwai, Kay Sievers, Sreekanth Reddy,
James Bottomley, Praveen Krishnamoorthy, hare,
Nagalakshmi Nandigama, Wu Zhangjin, Tetsuo Handa,
mpt-fusionlinux.pdl, Tim Gardner, Benjamin Poirier,
Santosh Rastapur, Casey Leedom, Hariprasad S, Pierre Fersing,
Arjan van de Ven, Abhijit Mahajan, systemd Mailing List
In-Reply-To: <CAB=NE6UprJH69hzc1mjqyq6PAn+7LZmX65Z8CDwUdJ73nDusHg@mail.gmail.com>
On Wed, Sep 10, 2014 at 11:10 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
>>> More than two years
>>> have gone by on growing design and assumptions on top of that original
>>> commit. I'm not sure if *systemd folks* yet believe its was a design
>>> regression?
>>
>> I don't think so. udev should not allow its workers to run for an
>> unbounded length of time. Whether the upper bound should be 30, 60,
>> 180 seconds or something else is up for debate (currently it is 60,
>> but if that is too short for some drivers we could certainly revisit
>> that).
>
> That's the thing -- the timeout was put in place under the assumption
> probe was asyncronous and its not, the driver core issues both module
> init *and* probe together, the loader has to wait. That alone makes
> the timeout a design flaw, and then systemd carried on top of that
> design over two years after that. Its not systemd's fault, its just
> that we never spoke about this as a design thing broadly and we should
> have, and I will mention that even when the first issues creeped up,
> the issue was still tossed back a driver problems. It was only until
> recently that we realized that both init and probe run together that
> we've been thinking about this problem differently. Systemd was trying
> to ensure init on driver don't take long but its not init that is
> taking long, its probe, and probe gets then penalized as the driver
> core batches both init and probe synchronously before finishing module
> loading.
Just to clarify: udev/systemd is not trying to get into the business
of what the kernel does on finit_module(), we just need to make sure
that none of our workers stay around forever, which is why we have a
global timeout. If necessary we can bump this higher (as mentioned in
another thread I just bumped it to 180 secs), but we cannot abolish it
entirely.
> Furthermore as clarified by Tejun random userland is known to
> exist that will wait indefinitely for module loading under the simple
> assumption things *are done synchronously*, and its precisely why we
> can't just blindly enable async probe upstream through a new driver
> boolean as it can be unfair to this old userland. What is being
> evaluated is to enable aync probe for *all* drivers through a new
> general system-wide option. We cannot regress old userspace and
> assumptions but we can create a new shiny universe.
How about simply introducing a new flag to finit_module() to indicate
that the caller does not care about asynchronicity. We could then pass
this from udev, but existing scripts calling modprobe/insmod will not
be affected.
>> Moreover, it seems from this discussion that the aim is (still)
>> that insmod should be near-instantaneous (i.e., not wait for probe),
>
> The only reason that is being discussed is that systemd has not
> accepted the timeout as a system design despite me pointing out the
> original design flaw recently and at this point even if was accepted
> as a design flaw it seems its too late. The approach taken to help
> make all drivers async is simply an afterthought to give systemd what
> it *thought* was in place, and it by no measure should be considered
> the proper fix to the regression introduced by systemd, it may perhaps
> be the right step long term for systemd systems given it goes with
> what it assumed was there, but the timeout was flawed. Its not clear
> if systemd can help with old kernels, it seems the ship has sailed and
> there seems no options but for folks to work around that -- unless of
> course some reasonable solution is found which doesn't break current
> systemd design?
If I read the git logs correctly the hard timeout was introduced in
April 2011, so reverting it now seems indeed not to help much with all
the running systems out there.
> As part of this series I addressed hunting for the "misbehaving
> drivers" in-kernel as I saw no progress on the systemd side of things
> to non-fatally detect "misbehaving drivers" despite my original RFCs
> and request for advice. I quote "misbehaving drivers" as its a flawed
> view to consider them misbehaving now in light of clarifications of
> how the driver core works in that it batches both init and probe
> together always and we can't be penalizing long probes due to the fact
> long probes are simply fine. My patch to pick up "misbehaving drivers"
> drivers on the kernel front by picking up systemd's signal was
> reactive but it was also simply braindead given the same exact reasons
> why systemd's original timeout was flawed. We want a general solution
> and we don't want to work around the root cause, in this case it was
> systemd's assumption on how drivers work.
Would your ongoing work to make probing asynchronous solve this
problem in the long-term? In the short-term I guess bumping the udev
timeout should be sufficient.
> Keep in mind that the above just addresses kmod built-in cmd on
> systemd, its where the timeout was introduced but as has been
> clarified here assuming the same timeout on *all* other built-in
> likely is likely pretty flawed as well and this does concern me. Its
> why I mentioned that more than two years have gone by now on growing
> design and assumptions on top of that original commit and its why its
> hard for systemd to consider an alternative.
All built-ins should be near-instantaneous. If they are not, that
needs to be fixed, or they should not be udev built-ins at all. I have
now added a warning to udev if any builtin-in takes more than a third
of the timeout, so hopefully any problems should be spotted early.
>>>>> I'm afraid distributions that want to avoid this
>>>>> sigkill at least on the kernel front will have to work around this
>>>>> issue either on systemd by increasing the default timeout which is now
>>>>> possible thanks to Hannes' changes or by some other means such as the
>>>>> combination of a modified non-chatty version of this patch + a check
>>>>> at the end of load_module() as mentioned earlier on these threads.
>>>>
>>>> Increasing the default timeout in systemd seems like the obvious bug fix
>>>> to me. If the patch exists already, having distros that want it use it
>>>> looks to be correct ... not every bug is a kernel bug, after all.
>>>
>>> Its merged upstream on systemd now, along with a few fixes on top of
>>> it. I also see Kay merged a change to the default timeout to 60 second
>>> on August 30. Its unclear if these discussions had any impact on that
>>> decision or if that was just because udev firmware loading got now
>>> ripped out. I'll note that the new 60 second timeout wouldn't suffice
>>> for cxgb4 even if it didn't do firmware loading, its probe takes over
>>> one full minute.
>>>
>>>> Negotiating a probe vs init split for drivers is fine too, but it's a
>>>> longer term thing rather than a bug fix.
>>>
>>> Indeed. What I proposed with a multiplier for the timeout for the
>>> different types of built in commands was deemed complex but saw no
>>> alternatives proposed despite my interest to work on one and
>>> clarifications noted that this was a design regression. Not quite sure
>>> what else I could have done here. I'm interested in learning what the
>>> better approach is for the future as if we want to marry init + kernel
>>> we need a smooth way for us to discuss design without getting worked
>>> up about it, or taking it personal. I really want this to work as I
>>> personally like systemd so far.
>>
>> How about this: keep the timeout global, but also introduce a
>> (relatively short, say 10 or 15 seconds) timeout after which a warning
>> is printed.
>
> That is something that I originally was looking forward to on systemd,
> but here's the thing once that warning comes up -- what would we do
> with it?
Short term: bump the timeout further. Long-term, hopefully the driver
(core) can be changed to avoid the problem.
> This patch addresses this warning in-kernel and the idea was
> that we'd then peg an async_probe bool as true on the driver as a fix,
> that was decided to be silly given all the above. These drivers are
> actually not misbehaving and it would be even more incorrect to try to
> "fix" them by making them run asynchronously. In fact for some old
> storage drivers it may even be the worst thing to do given the
> possible slew of userland deployment and scripts which assume things
> *are* synchronous.
As mentioned above, it probably makes sense to switch on the
asynchronous behaviour only for a given call to finit_module(), and
not globally to avoid problems with userland assumptions.
>> Even if nothing is actually killed, having workers (be it
>> insmod or something else) take longer than a couple of seconds is
>> likely a sign that something is seriously off somewhere.
>
> Probe can take a long time and that's fine,
But isn't finit_module() taking a long time a serious problem given
that it means no other module can be loaded in parallel? Even if you
have some storage device which legitimately needs to take a couple of
minutes to probe, you probably still want your computer to boot and
get on with its other tasks whilst you wait... Or worse still, some
insignificant driver is broken and simply hangs in probe, but surely
you still want the rest of the system to boot?
> so for kmod the current
> assumption is flawed. If we had an option to async probe all drivers
> then perhaps this kmod timeout *might be reasonable*, and even then I
> do recommend for a clear warning that can be collected on logs on its
> first iteration rather than sigkilling, only after a whlie should
> sigkilling be done really. If systemd can deal with module loading in
> the background for drivers that take a long time and warning on that
> intsead of sigkiling it may be good start prior to enabling a default
> sigkill on drivers. This is perhaps also true for other workers but
> its not clear if this is a reasonable strategy for systemd.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Andy Lutomirski @ 2014-09-11 21:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuziPptHxtw_7fkOdR-paB+8BatNmRPoo3txP8wOp9D6Tw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Sep 11, 2014 at 1:33 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> On Thu, Sep 11, 2014 at 12:47 PM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 09/10/2014 07:32 PM, Alexei Starovoitov wrote:
>>>
>>> On Wed, Sep 10, 2014 at 2:03 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> wrote:
>>>>>
>>>>> struct { /* anonymous struct used by BPF_PROG_LOAD command
>>>>> */
>>>>> enum bpf_prog_type prog_type;
>>>>> __u32 insn_cnt;
>>>>> const struct bpf_insn *insns;
>>>>> const char *license;
>>>>> __u32 log_level; /* verbosity level of
>>>>> eBPF verifier */
>>>>> __u32 log_size; /* size of user buffer
>>>>> */
>>>>> void *log_buf; /* user supplied
>>>>> buffer
>>>>> */
>>>>
>>>>
>>>>
>>>> What is log buffer? Would that mean the verifier will return an error
>>>> string if the program will not pass it, or if not, what other data?
>>>> I think the man page is missing how to examine the returned verifier
>>>> log buffer data.
>>>
>>>
>>> yes. it's an error log (as text string for humans) from verifier.
>>
>> I was confused due to the void pointer. But that also means that the text
>
> ahh. ok. will change it to 'char *' then.
>
>> string becomes part of the ABI; aren't eBPF specific error codes (perhaps
>> a tuple of [line + error code]), though ugly as well, but perhaps the better
>> solution to this [which user space can then map to an actual string]?
>
> the verifier log contains full trace. Last unsafe instruction + error
> in many cases is useless. What we found empirically from using
> it over last 2 years is that developers have different learning curve
> to adjust to 'safe' style of C. Pretty much everyone couldn't
> figure out why program is rejected based on last error. Therefore
> verifier emits full log. From the 1st insn all the way till the last
> 'unsafe' instruction. So the log is multiline output.
> 'Understanding eBPF verifier messages' section of
> Documentation/networking/filter.txt provides few trivial
> examples of these multiline messages.
> Like for the program:
> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> BPF_LD_MAP_FD(BPF_REG_1, 0),
> BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
> BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
> BPF_EXIT_INSN(),
> the verifier log_buf is:
> 0: (7a) *(u64 *)(r10 -8) = 0
> 1: (bf) r2 = r10
> 2: (07) r2 += -8
> 3: (b7) r1 = 0
> 4: (85) call 1
> 5: (15) if r0 == 0x0 goto pc+1
> R0=map_ptr R10=fp
> 6: (7a) *(u64 *)(r0 +4) = 0
> misaligned access off 4 size 8
>
> It will surely change over time as verifier becomes smarter,
> supports new types, optimizations and so on.
> So this log is not an ABI. It's for humans to read.
> The log explains _how_ verifier came to conclusion
> that the program is unsafe.
Given that you've already arranged (I think) for the verifier to be
compilable in the kernel and in userspace, would it make more sense to
have the kernel version just say yes or no and to make it easy for
user code to retry verification in userspace if they want a full
explanation?
--Andy
^ permalink raw reply
* [Patch net-next] ipv6: exit early in addrconf_notify() if IPv6 is disabled
From: Cong Wang @ 2014-09-11 22:07 UTC (permalink / raw)
To: netdev; +Cc: Hideaki YOSHIFUJI, David S. Miller, Cong Wang
If IPv6 is explicitly disabled before the interface comes up,
it makes no sense to continue when it comes up, even just
print a message.
(I am not sure about other cases though, so I prefer not to touch)
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ad4598f..12a2efe 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2844,6 +2844,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
if (dev->flags & IFF_SLAVE)
break;
+ if (idev && idev->cnf.disable_ipv6)
+ break;
+
if (event == NETDEV_UP) {
if (!addrconf_qdisc_ok(dev)) {
/* device is not ready yet. */
^ permalink raw reply related
* Re: [PATCH net-next v3 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: Mahesh Bandewar @ 2014-09-11 22:08 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David Miller,
netdev, Eric Dumazet, Maciej Zenczykowski
In-Reply-To: <CAF2d9jgPRuTGibfro3i6+Lf9t=_OEfTSTZ6pHgqS+1T+Ubg=fQ@mail.gmail.com>
some how my earlier mail bounced back (formatting issues, I suppose!).
So it's a resend.
On Thu, Sep 11, 2014 at 2:27 PM, Mahesh Bandewar <maheshb@google.com> wrote:
>
> On Thu, Sep 11, 2014 at 2:39 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:
> > On 11/09/14 06:16, Mahesh Bandewar wrote:
> >>
> >> Earlier change to use usable slave array for TLB mode had an additional
> >> performance advantage. So extending the same logic to all other modes
> >> that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
> >> Also consolidating this with the earlier TLB change.
> >>
> >> The main idea is to build the usable slaves array in the control path
> >> and use that array for slave selection during xmit operation.
> >>
> >> Measured performance in a setup with a bond of 4x1G NICs with 200
> >> instances of netperf for the modes involved (3ad, xor, tlb)
> >> cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
> >>
> >> Mode TPS-Before TPS-After
> >>
> >> 802.3ad : 468,694 493,101
> >> TLB (lb=0): 392,583 392,965
> >> XOR : 475,696 484,517
> >>
> >> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> >> ---
> >> v1:
> >> (a) If bond_update_slave_arr() fails to allocate memory, it will
> >> overwrite
> >> the slave that need to be removed.
> >> (b) Freeing of array will assign NULL (to handle bond->down to bond->up
> >> transition gracefully.
> >> (c) Change from pr_debug() to pr_err() if bond_update_slave_arr()
> >> returns
> >> failure.
> >> (d) XOR: bond_update_slave_arr() will consider mii-mon, arp-mon cases
> >> and
> >> will populate the array even if these parameters are not used.
> >> (e) 3AD: Should handle the ad_agg_selection_logic correctly.
> >> v2:
> >> (a) Removed rcu_read_{un}lock() calls from array manipulation code.
> >> (b) Slave link-events now refresh array for all these modes.
> >> (c) Moved free-array call from bond_close() to bond_uninit().
> >> v3:
> >> (a) Fixed null pointer dereference.
> >> (b) Removed bond->lock lockdep dependency.
> >>
> > Hello Mahesh,
> > You should've given me time to respond, the reason I wrote this:
> > "First a question, if a bond device in XOR mode is up and we enslave a
> > single
> > slave how would it start transmitting ? Same question, if we are enslaving
> > a
> > second device then the array will be rebuild with only the first upon
> > NETDEV_UP
> > (of course all this is in the case miimon is 0).
> > The NETDEV_UP upon enslave happens before the slave is linked in."
> > was not because I wanted you to remove the slave rebuilding from the
> > NETDEV_UP/DOWN events, but because I didn't see how would a slave start
> > transmitting in XOR mode after enslaving, and I just tested it - it doesn't
> > since the slave array never gets rebuilt. The NETDEV_UP event is carried by
> > the dev_open() done in bond_enslave() earlier so the bond_set_carrier() in
> > the end isn't of much importance in most cases, simply do the following and
> > you'll see:
> > modprobe bonding mode=2
> > ip set bond0 up
> > ifenslave bond0 eth0
> >
> > Try to transmit anything and watch on the other side, you won't be able to
> > see anything as there's no slave array. My second question was given all
> > this, if you enslave any subsequent slaves, will it start transmitting ? But
> > I just tested this scenario and it still doesn't as the array doesn't get
> > built.
>
OK I tried that and I don't see what you have observed -
Here is the log -
~# modprobe bonding mode=2
~# ip link set bond0 up
[ 133.537516] New slave count=0
~# [add ip addr]
~# [add default route]
~# ifenslave bond0 eth0
~# [ 211.044852] Adding slave=eth0
[ 211.047826] New slave count=1
~# ifenslave bond0 eth1
[ 723.795877] Adding slave=eth0
[ 723.798853] Adding slave=eth1
[ 723.801824] New slave count=2
I have added some instrumentation to see what is happening and
following are the couple of changes (basically printfs!) -
@@ -3730,13 +3736,16 @@ int bond_update_slave_arr(struct bonding
*bond, struct slave *skipslave)
continue;
if (skipslave == slave)
continue;
+pr_err("Adding slave=%s\n", slave->dev->name);
new_arr->arr[new_arr->count++] = slave;
}
old_arr = rcu_dereference_protected(bond->slave_arr,
lockdep_rtnl_is_held() ||
lockdep_is_held(&bond->curr_slave_lock));
rcu_assign_pointer(bond->slave_arr, new_arr);
+pr_err("New slave count=%d\n", new_arr->count);
if (old_arr)
kfree_rcu(old_arr, rcu);
> > A few suggestions below, nothing serious though.
> >
> >
> >> drivers/net/bonding/bond_3ad.c | 76 +++-----------------
> >> drivers/net/bonding/bond_alb.c | 51 ++------------
> >> drivers/net/bonding/bond_alb.h | 8 ---
> >> drivers/net/bonding/bond_main.c | 152
> >> ++++++++++++++++++++++++++++++++++++----
> >> drivers/net/bonding/bonding.h | 8 +++
> >> 5 files changed, 164 insertions(+), 131 deletions(-)
> >>
> >> diff --git a/drivers/net/bonding/bond_3ad.c
> >> b/drivers/net/bonding/bond_3ad.c
> >> index 5d27a6207384..516075f0a740 100644
> >> --- a/drivers/net/bonding/bond_3ad.c
> >> +++ b/drivers/net/bonding/bond_3ad.c
> >> @@ -1579,6 +1579,8 @@ static void ad_agg_selection_logic(struct aggregator
> >> *agg)
> >> __disable_port(port);
> >> }
> >> }
> >> + if (bond_update_slave_arr(bond, NULL))
> >> + pr_err("Failed to build slave-array for 3ad
> >> mode.\n");
> >> }
> >>
> >> /* if the selected aggregator is of join individuals
> >> @@ -1717,6 +1719,8 @@ static void ad_enable_collecting_distributing(struct
> >> port *port)
> >> port->actor_port_number,
> >> port->aggregator->aggregator_identifier);
> >> __enable_port(port);
> >> + if (bond_update_slave_arr(port->slave->bond, NULL))
> >> + pr_err("Failed to build slave-array for 3ad
> >> mode.\n");
> >> }
> >> }
> >>
> >> @@ -1733,6 +1737,8 @@ static void
> >> ad_disable_collecting_distributing(struct port *port)
> >> port->actor_port_number,
> >> port->aggregator->aggregator_identifier);
> >> __disable_port(port);
> >> + if (bond_update_slave_arr(port->slave->bond, NULL))
> >> + pr_err("Failed to build slave-array for 3ad
> >> mode.\n");
> >> }
> >> }
> >>
> >> @@ -2311,6 +2317,9 @@ void bond_3ad_handle_link_change(struct slave
> >> *slave, char link)
> >> */
> >> port->sm_vars |= AD_PORT_BEGIN;
> >>
> >> + if (bond_update_slave_arr(slave->bond, NULL))
> >> + pr_err("Failed to build slave-array for 3ad mode.\n");
> >> +
> >> __release_state_machine_lock(port);
> >> }
> >>
> >> @@ -2406,73 +2415,6 @@ int bond_3ad_get_active_agg_info(struct bonding
> >> *bond, struct ad_info *ad_info)
> >> return ret;
> >> }
> >>
> >> -int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
> >> -{
> >> - struct bonding *bond = netdev_priv(dev);
> >> - struct slave *slave, *first_ok_slave;
> >> - struct aggregator *agg;
> >> - struct ad_info ad_info;
> >> - struct list_head *iter;
> >> - int slaves_in_agg;
> >> - int slave_agg_no;
> >> - int agg_id;
> >> -
> >> - if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
> >> - netdev_dbg(dev, "__bond_3ad_get_active_agg_info
> >> failed\n");
> >> - goto err_free;
> >> - }
> >> -
> >> - slaves_in_agg = ad_info.ports;
> >> - agg_id = ad_info.aggregator_id;
> >> -
> >> - if (slaves_in_agg == 0) {
> >> - netdev_dbg(dev, "active aggregator is empty\n");
> >> - goto err_free;
> >> - }
> >> -
> >> - slave_agg_no = bond_xmit_hash(bond, skb) % slaves_in_agg;
> >> - first_ok_slave = NULL;
> >> -
> >> - bond_for_each_slave_rcu(bond, slave, iter) {
> >> - agg = SLAVE_AD_INFO(slave)->port.aggregator;
> >> - if (!agg || agg->aggregator_identifier != agg_id)
> >> - continue;
> >> -
> >> - if (slave_agg_no >= 0) {
> >> - if (!first_ok_slave && bond_slave_can_tx(slave))
> >> - first_ok_slave = slave;
> >> - slave_agg_no--;
> >> - continue;
> >> - }
> >> -
> >> - if (bond_slave_can_tx(slave)) {
> >> - bond_dev_queue_xmit(bond, skb, slave->dev);
> >> - goto out;
> >> - }
> >> - }
> >> -
> >> - if (slave_agg_no >= 0) {
> >> - netdev_err(dev, "Couldn't find a slave to tx on for
> >> aggregator ID %d\n",
> >> - agg_id);
> >> - goto err_free;
> >> - }
> >> -
> >> - /* we couldn't find any suitable slave after the agg_no, so use
> >> the
> >> - * first suitable found, if found.
> >> - */
> >> - if (first_ok_slave)
> >> - bond_dev_queue_xmit(bond, skb, first_ok_slave->dev);
> >> - else
> >> - goto err_free;
> >> -
> >> -out:
> >> - return NETDEV_TX_OK;
> >> -err_free:
> >> - /* no suitable interface, frame not sent */
> >> - dev_kfree_skb_any(skb);
> >> - goto out;
> >> -}
> >> -
> >> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding
> >> *bond,
> >> struct slave *slave)
> >> {
> >> diff --git a/drivers/net/bonding/bond_alb.c
> >> b/drivers/net/bonding/bond_alb.c
> >> index 028496205f39..dbac0ceb17f6 100644
> >> --- a/drivers/net/bonding/bond_alb.c
> >> +++ b/drivers/net/bonding/bond_alb.c
> >> @@ -200,7 +200,6 @@ static int tlb_initialize(struct bonding *bond)
> >> static void tlb_deinitialize(struct bonding *bond)
> >> {
> >> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> >> - struct tlb_up_slave *arr;
> >>
> >> _lock_tx_hashtbl_bh(bond);
> >>
> >> @@ -208,10 +207,6 @@ static void tlb_deinitialize(struct bonding *bond)
> >> bond_info->tx_hashtbl = NULL;
> >>
> >> _unlock_tx_hashtbl_bh(bond);
> >> -
> >> - arr = rtnl_dereference(bond_info->slave_arr);
> >> - if (arr)
> >> - kfree_rcu(arr, rcu);
> >> }
> >>
> >> static long long compute_gap(struct slave *slave)
> >> @@ -1409,39 +1404,9 @@ out:
> >> return NETDEV_TX_OK;
> >> }
> >>
> >> -static int bond_tlb_update_slave_arr(struct bonding *bond,
> >> - struct slave *skipslave)
> >> -{
> >> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> >> - struct slave *tx_slave;
> >> - struct list_head *iter;
> >> - struct tlb_up_slave *new_arr, *old_arr;
> >> -
> >> - new_arr = kzalloc(offsetof(struct tlb_up_slave,
> >> arr[bond->slave_cnt]),
> >> - GFP_ATOMIC);
> >> - if (!new_arr)
> >> - return -ENOMEM;
> >> -
> >> - bond_for_each_slave(bond, tx_slave, iter) {
> >> - if (!bond_slave_can_tx(tx_slave))
> >> - continue;
> >> - if (skipslave == tx_slave)
> >> - continue;
> >> - new_arr->arr[new_arr->count++] = tx_slave;
> >> - }
> >> -
> >> - old_arr = rtnl_dereference(bond_info->slave_arr);
> >> - rcu_assign_pointer(bond_info->slave_arr, new_arr);
> >> - if (old_arr)
> >> - kfree_rcu(old_arr, rcu);
> >> -
> >> - return 0;
> >> -}
> >> -
> >> int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> >> {
> >> struct bonding *bond = netdev_priv(bond_dev);
> >> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> >> struct ethhdr *eth_data;
> >> struct slave *tx_slave = NULL;
> >> u32 hash_index;
> >> @@ -1462,12 +1427,14 @@ int bond_tlb_xmit(struct sk_buff *skb, struct
> >> net_device *bond_dev)
> >> hash_index &
> >> 0xFF,
> >> skb->len);
> >> } else {
> >> - struct tlb_up_slave *slaves;
> >> + struct bond_up_slave *slaves;
> >> + unsigned int count;
> >>
> >> - slaves =
> >> rcu_dereference(bond_info->slave_arr);
> >> - if (slaves && slaves->count)
> >> + slaves = rcu_dereference(bond->slave_arr);
> >> + count = slaves ? slaves->count : 0;
> >> + if (count)
> >
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > In both of these cases (slaves & slaves->count) you could use likely() as in
> > the case we have slaves to transmit, it'd be advantageous and will be hit
> > every time. The cases that we get here and don't have a slave_arr/count are
> > mostly 2:
> > 1. The slaves got released between the start of xmit and this part
> > 2. They were not eligible so the array got emptied
> > In both of these cases we don't care about the fallout path since
> > transmission isn't possible anyway.
> > Also another minor thing is that usually in the cases where we want to fetch
> > a variable only once ACCESS_ONCE() is used as a weak compiler barrier to
> > make sure the compiler doesn't optimize out something. The others can
> > correct me if I'm wrong, but I think in this case it's a good precaution for
> > slaves->count.
> > These are merely suggestions, I might be wrong.
> >
Will do.
> >
> >> tx_slave = slaves->arr[hash_index
> >> %
> >> -
> >> slaves->count];
> >> + count];
> >> }
> >> break;
> >> }
> >> @@ -1733,10 +1700,6 @@ void bond_alb_deinit_slave(struct bonding *bond,
> >> struct slave *slave)
> >> rlb_clear_slave(bond, slave);
> >> }
> >>
> >> - if (bond_is_nondyn_tlb(bond))
> >> - if (bond_tlb_update_slave_arr(bond, slave))
> >> - pr_err("Failed to build slave-array for TLB
> >> mode.\n");
> >> -
> >> }
> >>
> >> /* Caller must hold bond lock for read */
> >> @@ -1762,7 +1725,7 @@ void bond_alb_handle_link_change(struct bonding
> >> *bond, struct slave *slave, char
> >> }
> >>
> >> if (bond_is_nondyn_tlb(bond)) {
> >> - if (bond_tlb_update_slave_arr(bond, NULL))
> >> + if (bond_update_slave_arr(bond, NULL))
> >> pr_err("Failed to build slave-array for TLB
> >> mode.\n");
> >> }
> >> }
> >> diff --git a/drivers/net/bonding/bond_alb.h
> >> b/drivers/net/bonding/bond_alb.h
> >> index aaeac61d03cf..5fc76c01636c 100644
> >> --- a/drivers/net/bonding/bond_alb.h
> >> +++ b/drivers/net/bonding/bond_alb.h
> >> @@ -139,20 +139,12 @@ struct tlb_slave_info {
> >> */
> >> };
> >>
> >> -struct tlb_up_slave {
> >> - unsigned int count;
> >> - struct rcu_head rcu;
> >> - struct slave *arr[0];
> >> -};
> >> -
> >> struct alb_bond_info {
> >> struct tlb_client_info *tx_hashtbl; /* Dynamically allocated */
> >> spinlock_t tx_hashtbl_lock;
> >> u32 unbalanced_load;
> >> int tx_rebalance_counter;
> >> int lp_counter;
> >> - /* -------- non-dynamic tlb mode only ---------*/
> >> - struct tlb_up_slave __rcu *slave_arr; /* Up slaves */
> >> /* -------- rlb parameters -------- */
> >> int rlb_enabled;
> >> struct rlb_client_info *rx_hashtbl; /* Receive hash table */
> >> diff --git a/drivers/net/bonding/bond_main.c
> >> b/drivers/net/bonding/bond_main.c
> >> index b43b2df9e5d1..4412c458939d 100644
> >> --- a/drivers/net/bonding/bond_main.c
> >> +++ b/drivers/net/bonding/bond_main.c
> >> @@ -1700,6 +1700,10 @@ static int __bond_release_one(struct net_device
> >> *bond_dev,
> >> write_unlock_bh(&bond->curr_slave_lock);
> >> }
> >>
> >> + if (bond_mode_uses_xmit_hash(bond) &&
> >> + bond_update_slave_arr(bond, slave))
> >> + pr_err("Failed to build slave-array.\n");
> >> +
> >> netdev_info(bond_dev, "Releasing %s interface %s\n",
> >> bond_is_active_slave(slave) ? "active" : "backup",
> >> slave_dev->name);
> >> @@ -2015,6 +2019,10 @@ static void bond_miimon_commit(struct bonding
> >> *bond)
> >> bond_alb_handle_link_change(bond, slave,
> >> BOND_LINK_UP);
> >>
> >> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
> >> + bond_update_slave_arr(bond, NULL))
> >> + pr_err("Failed to build slave-array for
> >> XOR mode.\n");
> >> +
> >> if (!bond->curr_active_slave || slave == primary)
> >> goto do_failover;
> >>
> >> @@ -2042,6 +2050,10 @@ static void bond_miimon_commit(struct bonding
> >> *bond)
> >> bond_alb_handle_link_change(bond, slave,
> >>
> >> BOND_LINK_DOWN);
> >>
> >> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
> >> + bond_update_slave_arr(bond, NULL))
> >> + pr_err("Failed to build slave-array for
> >> XOR mode.\n");
> >> +
> >> if (slave ==
> >> rcu_access_pointer(bond->curr_active_slave))
> >> goto do_failover;
> >>
> >> @@ -2505,6 +2517,9 @@ static void bond_loadbalance_arp_mon(struct
> >> work_struct *work)
> >>
> >> if (slave_state_changed) {
> >> bond_slave_state_change(bond);
> >> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
> >> + bond_update_slave_arr(bond, NULL))
> >> + pr_err("Failed to build slave-array for
> >> XOR mode.\n");
> >> } else if (do_failover) {
> >> /* the bond_select_active_slave must hold RTNL
> >> * and curr_slave_lock for write.
> >> @@ -2899,11 +2914,23 @@ static int bond_slave_netdev_event(unsigned long
> >> event,
> >> if (old_duplex != slave->duplex)
> >> bond_3ad_adapter_duplex_changed(slave);
> >> }
> >> + /* Refresh slave-array if applicable!
> >> + * If the setuo does not use miimon or arpmon
> >> (mode-specific!),
> >> + * then these event will not cause the slave-array to be
> >> + * refreshed. This will cause xmit to use a slave that is
> >> not
> >> + * usable. Avoid such situation by refeshing the array at
> >> these
> >> + * events. If these (miimon/arpmon) parameters are
> >> configured
> >> + * then array gets refreshed twice and that should be
> >> fine!
> >> + */
> >> + if (bond_mode_uses_xmit_hash(bond) &&
> >> + bond_update_slave_arr(bond, NULL))
> >> + pr_err("Failed to build slave-array for XOR
> >> mode.\n");
> >> break;
> >> case NETDEV_DOWN:
> >> - /*
> >> - * ... Or is it this?
> >> - */
> >> + /* Refresh slave-array if applicable! */
> >> + if (bond_mode_uses_xmit_hash(bond) &&
> >> + bond_update_slave_arr(bond, NULL))
> >> + pr_err("Failed to build slave-array for XOR
> >> mode.\n");
> >> break;
> >> case NETDEV_CHANGEMTU:
> >> /*
> >> @@ -3147,6 +3174,10 @@ static int bond_open(struct net_device *bond_dev)
> >> bond_3ad_initiate_agg_selection(bond, 1);
> >> }
> >>
> >> + if (bond_mode_uses_xmit_hash(bond) &&
> >> + bond_update_slave_arr(bond, NULL))
> >> + pr_err("Failed to build slave-array for XOR mode.\n");
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -3654,15 +3685,106 @@ static int bond_xmit_activebackup(struct sk_buff
> >> *skb, struct net_device *bond_d
> >> return NETDEV_TX_OK;
> >> }
> >>
> >> -/* In bond_xmit_xor() , we determine the output device by using a pre-
> >> - * determined xmit_hash_policy(), If the selected device is not enabled,
> >> - * find the next active slave.
> >> +/* Build the usable slaves array in control path for modes that use
> >> xmit-hash
> >> + * to determine the slave interface -
> >> + * (a) BOND_MODE_8023AD
> >> + * (b) BOND_MODE_XOR
> >> + * (c) BOND_MODE_TLB && tlb_dynamic_lb == 0
> >> */
> >> -static int bond_xmit_xor(struct sk_buff *skb, struct net_device
> >> *bond_dev)
> >> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> >> {
> >> - struct bonding *bond = netdev_priv(bond_dev);
> >> + struct slave *slave;
> >> + struct list_head *iter;
> >> + struct bond_up_slave *new_arr, *old_arr;
> >> + int slaves_in_agg;
> >> + int agg_id = 0;
> >> + int ret = 0;
> >> +
> >> + new_arr = kzalloc(offsetof(struct bond_up_slave,
> >> arr[bond->slave_cnt]),
> >> + GFP_ATOMIC);
> >> + if (!new_arr) {
> >> + ret = -ENOMEM;
> >> + goto out;
> >> + }
> >> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> >> + struct ad_info ad_info;
> >> +
> >> + if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
> >> + pr_debug("bond_3ad_get_active_agg_info failed\n");
> >> + kfree_rcu(new_arr, rcu);
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> + slaves_in_agg = ad_info.ports;
> >> + agg_id = ad_info.aggregator_id;
> >> + }
> >> + bond_for_each_slave(bond, slave, iter) {
> >> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> >> + struct aggregator *agg;
> >>
> >> - bond_xmit_slave_id(bond, skb, bond_xmit_hash(bond, skb) %
> >> bond->slave_cnt);
> >> + agg = SLAVE_AD_INFO(slave)->port.aggregator;
> >> + if (!agg || agg->aggregator_identifier != agg_id)
> >> + continue;
> >> + }
> >> + if (!bond_slave_can_tx(slave))
> >> + continue;
> >> + if (skipslave == slave)
> >> + continue;
> >> + new_arr->arr[new_arr->count++] = slave;
> >> + }
> >> +
> >> + old_arr = rcu_dereference_protected(bond->slave_arr,
> >> + lockdep_rtnl_is_held() ||
> >> +
> >> lockdep_is_held(&bond->curr_slave_lock));
> >> + rcu_assign_pointer(bond->slave_arr, new_arr);
> >> + if (old_arr)
> >> + kfree_rcu(old_arr, rcu);
> >> +
> >> +out:
> >> + if (ret != 0 && skipslave) {
> >> + int idx;
> >> +
> >> + /* Rare situation where caller has asked to skip a
> >> specific
> >> + * slave but allocation failed (most likely!). BTW this is
> >> + * only possible when the call is initiated from
> >> + * __bond_release_one(). In this sitation; overwrite the
> >> + * skipslave entry in the array with the last entry from
> >> the
> >> + * array to avoid a situation where the xmit path may
> >> choose
> >> + * this to-be-skipped slave to send a packet out.
> >> + */
> >> + old_arr = rtnl_dereference(bond->slave_arr);
> >> + for (idx = 0; idx < old_arr->count; idx++) {
> >> + if (skipslave == old_arr->arr[idx]) {
> >> + old_arr->arr[idx] =
> >> + old_arr->arr[old_arr->count-1];
> >> + old_arr->count--;
> >> + break;
> >> + }
> >> + }
> >> + }
> >> + return ret;
> >> +}
> >> +
> >> +/* Use this Xmit function for 3AD as well as XOR modes. The current
> >> + * usable slave array is formed in the control path. The xmit function
> >> + * just calculates hash and sends the packet out.
> >> + */
> >> +int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
> >> +{
> >> + struct bonding *bond = netdev_priv(dev);
> >> + struct slave *slave;
> >> + struct bond_up_slave *slaves;
> >> + unsigned int count;
> >> +
> >> + slaves = rcu_dereference(bond->slave_arr);
> >> + count = slaves ? slaves->count : 0;
> >> + if (count) {
> >
> > ^^^^^^^^^^^^^^
> > The same comment as above applies here, too.
> >
> >
> >> + slave = slaves->arr[bond_xmit_hash(bond, skb) % count];
> >> + bond_dev_queue_xmit(bond, skb, slave->dev);
> >> + } else {
> >> + dev_kfree_skb_any(skb);
> >> + atomic_long_inc(&dev->tx_dropped);
> >> + }
> >>
> >> return NETDEV_TX_OK;
> >> }
> >> @@ -3764,12 +3886,11 @@ static netdev_tx_t __bond_start_xmit(struct
> >> sk_buff *skb, struct net_device *dev
> >> return bond_xmit_roundrobin(skb, dev);
> >> case BOND_MODE_ACTIVEBACKUP:
> >> return bond_xmit_activebackup(skb, dev);
> >> + case BOND_MODE_8023AD:
> >> case BOND_MODE_XOR:
> >> - return bond_xmit_xor(skb, dev);
> >> + return bond_3ad_xor_xmit(skb, dev);
> >> case BOND_MODE_BROADCAST:
> >> return bond_xmit_broadcast(skb, dev);
> >> - case BOND_MODE_8023AD:
> >> - return bond_3ad_xmit_xor(skb, dev);
> >> case BOND_MODE_ALB:
> >> return bond_alb_xmit(skb, dev);
> >> case BOND_MODE_TLB:
> >> @@ -3947,6 +4068,7 @@ static void bond_uninit(struct net_device *bond_dev)
> >> struct bonding *bond = netdev_priv(bond_dev);
> >> struct list_head *iter;
> >> struct slave *slave;
> >> + struct bond_up_slave *arr;
> >>
> >> bond_netpoll_cleanup(bond_dev);
> >>
> >> @@ -3955,6 +4077,12 @@ static void bond_uninit(struct net_device
> >> *bond_dev)
> >> __bond_release_one(bond_dev, slave->dev, true);
> >> netdev_info(bond_dev, "Released all slaves\n");
> >>
> >> + arr = rtnl_dereference(bond->slave_arr);
> >> + if (arr) {
> >> + kfree_rcu(arr, rcu);
> >> + RCU_INIT_POINTER(bond->slave_arr, NULL);
> >> + }
> >> +
> >> list_del(&bond->bond_list);
> >>
> >> bond_debug_unregister(bond);
> >> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> >> index 8375133dd347..78d6d3b7a780 100644
> >> --- a/drivers/net/bonding/bonding.h
> >> +++ b/drivers/net/bonding/bonding.h
> >> @@ -177,6 +177,12 @@ struct slave {
> >> struct kobject kobj;
> >> };
> >>
> >> +struct bond_up_slave {
> >> + unsigned int count;
> >> + struct rcu_head rcu;
> >> + struct slave *arr[0];
> >> +};
> >> +
> >> /*
> >> * Link pseudo-state only used internally by monitors
> >> */
> >> @@ -193,6 +199,7 @@ struct bonding {
> >> struct slave __rcu *curr_active_slave;
> >> struct slave __rcu *current_arp_slave;
> >> struct slave __rcu *primary_slave;
> >> + struct bond_up_slave __rcu *slave_arr; /* Array of usable slaves
> >> */
> >> bool force_primary;
> >> s32 slave_cnt; /* never change this value outside the
> >> attach/detach wrappers */
> >> int (*recv_probe)(const struct sk_buff *, struct bonding *,
> >> @@ -530,6 +537,7 @@ const char *bond_slave_link_status(s8 link);
> >> struct bond_vlan_tag *bond_verify_device_path(struct net_device
> >> *start_dev,
> >> struct net_device *end_dev,
> >> int level);
> >> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
> >>
> >> #ifdef CONFIG_PROC_FS
> >> void bond_create_proc_entry(struct bonding *bond);
> >>
> >
^ permalink raw reply
* [PATCH] drivers: net: b44: Fix typo in returning multicast stats
From: Mark Einon @ 2014-09-11 22:14 UTC (permalink / raw)
To: zambrano; +Cc: netdev, linux-kernel, Mark Einon
nstat->multicast refers to received packets, not transmitted as
is returned here. Change it so that received packet stats are
given.
Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
drivers/net/ethernet/broadcom/b44.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 4a7028d..d588136 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -1697,7 +1697,7 @@ static struct rtnl_link_stats64 *b44_get_stats64(struct net_device *dev,
hwstat->tx_underruns +
hwstat->tx_excessive_cols +
hwstat->tx_late_cols);
- nstat->multicast = hwstat->tx_multicast_pkts;
+ nstat->multicast = hwstat->rx_multicast_pkts;
nstat->collisions = hwstat->tx_total_cols;
nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
--
2.1.0
^ permalink raw reply related
* [PATCH Resend] drivers: net: b44: Fix typo in returning multicast stats
From: Mark Einon @ 2014-09-11 22:19 UTC (permalink / raw)
To: gary.zambrano; +Cc: netdev, linux-kernel, Mark Einon
nstat->multicast refers to received packets, not transmitted as
is returned here. Change it so that received packet stats are
given.
Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
changing to send to alternate address for Gary Zambrano, maintainers entry bounced.
drivers/net/ethernet/broadcom/b44.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 4a7028d..d588136 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -1697,7 +1697,7 @@ static struct rtnl_link_stats64 *b44_get_stats64(struct net_device *dev,
hwstat->tx_underruns +
hwstat->tx_excessive_cols +
hwstat->tx_late_cols);
- nstat->multicast = hwstat->tx_multicast_pkts;
+ nstat->multicast = hwstat->rx_multicast_pkts;
nstat->collisions = hwstat->tx_total_cols;
nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
--
2.1.0
^ permalink raw reply related
* Re: [systemd-devel] [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Luis R. Rodriguez @ 2014-09-11 22:26 UTC (permalink / raw)
To: Tom Gundersen, Tejun Heo
Cc: James Bottomley, One Thousand Gnomes, Takashi Iwai, Kay Sievers,
Oleg Nesterov, Praveen Krishnamoorthy, hare,
Nagalakshmi Nandigama, Wu Zhangjin, Tetsuo Handa,
mpt-fusionlinux.pdl, Tim Gardner, Benjamin Poirier,
Santosh Rastapur, Casey Leedom, Hariprasad S, Pierre Fersing,
Sreekanth Reddy, Arjan van de Ven, Abhijit Mahajan, systemd
In-Reply-To: <CAG-2HqUzCvd55mq=M3=tXbHAjc2QnVuRtbFZMWTz9k_6BmmngQ@mail.gmail.com>
On Thu, Sep 11, 2014 at 2:43 PM, Tom Gundersen <teg@jklm.no> wrote:
> On Wed, Sep 10, 2014 at 11:10 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
>>>> More than two years
>>>> have gone by on growing design and assumptions on top of that original
>>>> commit. I'm not sure if *systemd folks* yet believe its was a design
>>>> regression?
>>>
>>> I don't think so. udev should not allow its workers to run for an
>>> unbounded length of time. Whether the upper bound should be 30, 60,
>>> 180 seconds or something else is up for debate (currently it is 60,
>>> but if that is too short for some drivers we could certainly revisit
>>> that).
>>
>> That's the thing -- the timeout was put in place under the assumption
>> probe was asyncronous and its not, the driver core issues both module
>> init *and* probe together, the loader has to wait. That alone makes
>> the timeout a design flaw, and then systemd carried on top of that
>> design over two years after that. Its not systemd's fault, its just
>> that we never spoke about this as a design thing broadly and we should
>> have, and I will mention that even when the first issues creeped up,
>> the issue was still tossed back a driver problems. It was only until
>> recently that we realized that both init and probe run together that
>> we've been thinking about this problem differently. Systemd was trying
>> to ensure init on driver don't take long but its not init that is
>> taking long, its probe, and probe gets then penalized as the driver
>> core batches both init and probe synchronously before finishing module
>> loading.
>
> Just to clarify: udev/systemd is not trying to get into the business
> of what the kernel does on finit_module(), we just need to make sure
> that none of our workers stay around forever, which is why we have a
> global timeout. If necessary we can bump this higher (as mentioned in
> another thread I just bumped it to 180 secs), but we cannot abolish it
> entirely.
180 seconds is certainly better than 30, but let me be clear here on
the extent to which the timeout at least for kmod built-in command can
be an issue. The driver core not only batches init and probe together
synchronously, it also runs probe for *all* devices that the device
driver can claim and all those series of probes run synchronously
within itself, that is bus_for_each_dev() runs synchronously on each
device. So, if a init takes 1 second and probe for each device takes
120 seconds and the system has 2 devices with the new timeout the
second device would not be successfully probed (and in fact I'm not
sure if this would kill the first).
>> Furthermore as clarified by Tejun random userland is known to
>> exist that will wait indefinitely for module loading under the simple
>> assumption things *are done synchronously*, and its precisely why we
>> can't just blindly enable async probe upstream through a new driver
>> boolean as it can be unfair to this old userland. What is being
>> evaluated is to enable aync probe for *all* drivers through a new
>> general system-wide option. We cannot regress old userspace and
>> assumptions but we can create a new shiny universe.
>
> How about simply introducing a new flag to finit_module() to indicate
> that the caller does not care about asynchronicity. We could then pass
> this from udev, but existing scripts calling modprobe/insmod will not
> be affected.
Do you mean that you *do want asynchronicity*?
>>> Moreover, it seems from this discussion that the aim is (still)
>>> that insmod should be near-instantaneous (i.e., not wait for probe),
>>
>> The only reason that is being discussed is that systemd has not
>> accepted the timeout as a system design despite me pointing out the
>> original design flaw recently and at this point even if was accepted
>> as a design flaw it seems its too late. The approach taken to help
>> make all drivers async is simply an afterthought to give systemd what
>> it *thought* was in place, and it by no measure should be considered
>> the proper fix to the regression introduced by systemd, it may perhaps
>> be the right step long term for systemd systems given it goes with
>> what it assumed was there, but the timeout was flawed. Its not clear
>> if systemd can help with old kernels, it seems the ship has sailed and
>> there seems no options but for folks to work around that -- unless of
>> course some reasonable solution is found which doesn't break current
>> systemd design?
>
> If I read the git logs correctly the hard timeout was introduced in
> April 2011, so reverting it now seems indeed not to help much with all
> the running systems out there.
yeah figured :(
>> As part of this series I addressed hunting for the "misbehaving
>> drivers" in-kernel as I saw no progress on the systemd side of things
>> to non-fatally detect "misbehaving drivers" despite my original RFCs
>> and request for advice. I quote "misbehaving drivers" as its a flawed
>> view to consider them misbehaving now in light of clarifications of
>> how the driver core works in that it batches both init and probe
>> together always and we can't be penalizing long probes due to the fact
>> long probes are simply fine. My patch to pick up "misbehaving drivers"
>> drivers on the kernel front by picking up systemd's signal was
>> reactive but it was also simply braindead given the same exact reasons
>> why systemd's original timeout was flawed. We want a general solution
>> and we don't want to work around the root cause, in this case it was
>> systemd's assumption on how drivers work.
>
> Would your ongoing work to make probing asynchronous solve this
> problem in the long-term? In the short-term I guess bumping the udev
> timeout should be sufficient.
That and the global flag / module param to specify the async desire
which would not regress old userspace. Probe afterall is the main
source of the issue.
>> Keep in mind that the above just addresses kmod built-in cmd on
>> systemd, its where the timeout was introduced but as has been
>> clarified here assuming the same timeout on *all* other built-in
>> likely is likely pretty flawed as well and this does concern me. Its
>> why I mentioned that more than two years have gone by now on growing
>> design and assumptions on top of that original commit and its why its
>> hard for systemd to consider an alternative.
>
> All built-ins should be near-instantaneous. If they are not, that
> needs to be fixed, or they should not be udev built-ins at all. I have
> now added a warning to udev if any builtin-in takes more than a third
> of the timeout, so hopefully any problems should be spotted early.
Great thanks. Collecting these should be valuable and help being proactive.
>>>>>> I'm afraid distributions that want to avoid this
>>>>>> sigkill at least on the kernel front will have to work around this
>>>>>> issue either on systemd by increasing the default timeout which is now
>>>>>> possible thanks to Hannes' changes or by some other means such as the
>>>>>> combination of a modified non-chatty version of this patch + a check
>>>>>> at the end of load_module() as mentioned earlier on these threads.
>>>>>
>>>>> Increasing the default timeout in systemd seems like the obvious bug fix
>>>>> to me. If the patch exists already, having distros that want it use it
>>>>> looks to be correct ... not every bug is a kernel bug, after all.
>>>>
>>>> Its merged upstream on systemd now, along with a few fixes on top of
>>>> it. I also see Kay merged a change to the default timeout to 60 second
>>>> on August 30. Its unclear if these discussions had any impact on that
>>>> decision or if that was just because udev firmware loading got now
>>>> ripped out. I'll note that the new 60 second timeout wouldn't suffice
>>>> for cxgb4 even if it didn't do firmware loading, its probe takes over
>>>> one full minute.
>>>>
>>>>> Negotiating a probe vs init split for drivers is fine too, but it's a
>>>>> longer term thing rather than a bug fix.
>>>>
>>>> Indeed. What I proposed with a multiplier for the timeout for the
>>>> different types of built in commands was deemed complex but saw no
>>>> alternatives proposed despite my interest to work on one and
>>>> clarifications noted that this was a design regression. Not quite sure
>>>> what else I could have done here. I'm interested in learning what the
>>>> better approach is for the future as if we want to marry init + kernel
>>>> we need a smooth way for us to discuss design without getting worked
>>>> up about it, or taking it personal. I really want this to work as I
>>>> personally like systemd so far.
>>>
>>> How about this: keep the timeout global, but also introduce a
>>> (relatively short, say 10 or 15 seconds) timeout after which a warning
>>> is printed.
>>
>> That is something that I originally was looking forward to on systemd,
>> but here's the thing once that warning comes up -- what would we do
>> with it?
>
> Short term: bump the timeout further. Long-term, hopefully the driver
> (core) can be changed to avoid the problem.
Fine by me, although I think some folks still have concerns with the
sigkill completely. But not sure if we escape it now.
>> This patch addresses this warning in-kernel and the idea was
>> that we'd then peg an async_probe bool as true on the driver as a fix,
>> that was decided to be silly given all the above. These drivers are
>> actually not misbehaving and it would be even more incorrect to try to
>> "fix" them by making them run asynchronously. In fact for some old
>> storage drivers it may even be the worst thing to do given the
>> possible slew of userland deployment and scripts which assume things
>> *are* synchronous.
>
> As mentioned above, it probably makes sense to switch on the
> asynchronous behaviour only for a given call to finit_module(), and
> not globally to avoid problems with userland assumptions.
Sure that's one way.
>>> Even if nothing is actually killed, having workers (be it
>>> insmod or something else) take longer than a couple of seconds is
>>> likely a sign that something is seriously off somewhere.
>>
>> Probe can take a long time and that's fine,
>
> But isn't finit_module() taking a long time a serious problem given
> that it means no other module can be loaded in parallel?
Indeed but having a desire to make the init() complete fast is
different than the desire to have the combination of both init and
probe fast synchronously. If userspace wants init to be fast and let
probe be async then userspace has no option but to deal with the fact
that async probe will be async, and it should then use other methods
to match any dependencies if its doing that itself. For example
networking should not kick off after a network driver is loaded but
rather one the device creeps up on udev. We should be good with
networking dealing with this correctly today but not sure about other
subsystems. depmod should be able to load the required modules in
order and if bus drivers work right then probe of the remnant devices
should happen asynchronously. The one case I can think of that is a
bit different is modules-load.d things but those *do not rely on the
timeout*, but are loaded prior to a service requirement. Note though
that if those modules had probe and they then run async'd then systemd
service would probably need to consider that the requirements may not
be there until later. If this is not carefully considered that could
introduce regression to users of modules-load.d when async probe is
fully deployed. The same applies to systemd making assumptions of kmod
loading a module and a dependency being complete as probe would have
run it before.
> Even if you
> have some storage device which legitimately needs to take a couple of
> minutes to probe, you probably still want your computer to boot and
> get on with its other tasks whilst you wait... Or worse still, some
> insignificant driver is broken and simply hangs in probe, but surely
> you still want the rest of the system to boot?
Agreed, I believe one concern here lies in on whether or not userspace
is properly equipped to deal with the requirements on module loading
doing async probing and that possibly failing. Perhaps systemd might
think all userspace is ready for that but are we sure that's the case?
Another obvious issue was if the driver was a storage driver and your
boot depended upon it. If it takes a while we kill it and you can't
boot, no bueno. If systemd can avoid those situations that'd be nice.
That was the source of the first major issue reported by Joseph.
Chattiness on issues before the timeout should help a lot, we should
start collecting these somehow. These should be collected and
addressed. If we really want to be good on this we should put a bit of
effort on monitoring these and not being reactive.
Luis
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-11 22:29 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Borkmann, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CALCETrWCEwscbbfX7wAW-A+VQ5Y92igD36BmTXqFra04Qdwk0Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Sep 11, 2014 at 2:54 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>
>> the verifier log contains full trace. Last unsafe instruction + error
>> in many cases is useless. What we found empirically from using
>> it over last 2 years is that developers have different learning curve
>> to adjust to 'safe' style of C. Pretty much everyone couldn't
>> figure out why program is rejected based on last error. Therefore
>> verifier emits full log. From the 1st insn all the way till the last
>> 'unsafe' instruction. So the log is multiline output.
>> 'Understanding eBPF verifier messages' section of
>> Documentation/networking/filter.txt provides few trivial
>> examples of these multiline messages.
>> Like for the program:
>> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> BPF_LD_MAP_FD(BPF_REG_1, 0),
>> BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
>> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
>> BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
>> BPF_EXIT_INSN(),
>> the verifier log_buf is:
>> 0: (7a) *(u64 *)(r10 -8) = 0
>> 1: (bf) r2 = r10
>> 2: (07) r2 += -8
>> 3: (b7) r1 = 0
>> 4: (85) call 1
>> 5: (15) if r0 == 0x0 goto pc+1
>> R0=map_ptr R10=fp
>> 6: (7a) *(u64 *)(r0 +4) = 0
>> misaligned access off 4 size 8
>>
>> It will surely change over time as verifier becomes smarter,
>> supports new types, optimizations and so on.
>> So this log is not an ABI. It's for humans to read.
>> The log explains _how_ verifier came to conclusion
>> that the program is unsafe.
>
> Given that you've already arranged (I think) for the verifier to be
> compilable in the kernel and in userspace, would it make more sense to
> have the kernel version just say yes or no and to make it easy for
> user code to retry verification in userspace if they want a full
> explanation?
Good memory :) Long ago I had a hack where I compiled
verifier.o for kernel and linked it with userspace wrappers to
have the same verifier for userspace. It was very fragile.
and maps were not separate objects and there were no fds.
It's not feasible anymore, since different subsystems
will configure different bpf_context and helper functions and
verifier output is dynamic based on maps that were created.
For example, if user's samples/bpf/sock_example.c does
bpf_create_map(HASH, sizeof(key) * 2, ...);
instead of
bpf_create_map(HASH, sizeof(key), ...);
the same program will be rejected in first case and will be
accepted in the second, because map sizes and ebpf
program expectations are mismatching.
For the 1st case verifier will complain that program is
trying to pass uninitialized stack into bpf_lookup(key,...)
method or stack may be out of bounds.
Human insight is needed to understand what is going on.
I think more important is that source of truth needs to be
in one place == kernel. If we have two verifiers they will
diverge sooner or later and cause confusion for users.
I think as long as we document that verifier log
messages are not cast in stone and will change, we're fine.
I consider them as continuation of compiler warnings/errors.
They are meant for humans and do change from time to time.
^ 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