* [PATCH net-next 2/2] bonding: add bond_time_in_interval() and use it for time comparison
From: Veaceslav Falico @ 2013-08-03 1:50 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1375494636-7947-1-git-send-email-vfalico@redhat.com>
Currently we use a lot of time comparison math for arp_interval
comparisons, which are sometimes quite hard to read and understand.
All the time comparisons have one pattern:
(time - arp_interval_jiffies) <= jiffies <= (time + mod *
arp_interval_jiffies + arp_interval_jiffies/2)
Introduce a new helper - bond_time_in_interval(), which will do the math in
one place and, thus, will clean up the logical code. This helper introduces
a bit of overhead (by always calculating the jiffies from arp_interval),
however it's really not visible, considering that functions using it
usually run once in arp_interval milliseconds.
There are several lines slightly over 80 chars, however breaking them would
result in more hard-to-read code than several character after the 80 mark.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_main.c | 85 +++++++++++++++------------------------
1 files changed, 32 insertions(+), 53 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index cc13bfe..d58237b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2635,6 +2635,20 @@ out_unlock:
return RX_HANDLER_ANOTHER;
}
+/* function to verify if we're in the arp_interval timeslice, returns true if
+ * (last_act - arp_interval) <= jiffies <= (last_act + mod * arp_interval +
+ * arp_interval/2) . the arp_interval/2 is needed for really fast networks.
+ */
+static bool bond_time_in_interval(struct bonding *bond, unsigned long last_act,
+ int mod)
+{
+ int delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
+
+ return time_in_range(jiffies,
+ last_act - delta_in_ticks,
+ last_act + mod * delta_in_ticks + delta_in_ticks/2);
+}
+
/*
* this function is called regularly to monitor each slave's link
* ensuring that traffic is being sent and received when arp monitoring
@@ -2648,13 +2662,9 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
arp_work.work);
struct slave *slave, *oldcurrent;
int do_failover = 0;
- int delta_in_ticks, extra_ticks;
read_lock(&bond->lock);
- delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
- extra_ticks = delta_in_ticks / 2;
-
if (list_empty(&bond->slave_list))
goto re_arm;
@@ -2671,12 +2681,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
unsigned long trans_start = dev_trans_start(slave->dev);
if (slave->link != BOND_LINK_UP) {
- if (time_in_range(jiffies,
- trans_start - delta_in_ticks,
- trans_start + delta_in_ticks + extra_ticks) &&
- time_in_range(jiffies,
- slave->dev->last_rx - delta_in_ticks,
- slave->dev->last_rx + delta_in_ticks + extra_ticks)) {
+ if (bond_time_in_interval(bond, trans_start, 1) &&
+ bond_time_in_interval(bond, slave->dev->last_rx, 1)) {
slave->link = BOND_LINK_UP;
bond_set_active_slave(slave);
@@ -2704,12 +2710,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
* when the source ip is 0, so don't take the link down
* if we don't know our ip yet
*/
- if (!time_in_range(jiffies,
- trans_start - delta_in_ticks,
- trans_start + 2 * delta_in_ticks + extra_ticks) ||
- !time_in_range(jiffies,
- slave->dev->last_rx - delta_in_ticks,
- slave->dev->last_rx + 2 * delta_in_ticks + extra_ticks)) {
+ if (!bond_time_in_interval(bond, trans_start, 2) ||
+ !bond_time_in_interval(bond, slave->dev->last_rx, 2)) {
slave->link = BOND_LINK_DOWN;
bond_set_backup_slave(slave);
@@ -2749,7 +2751,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
re_arm:
if (bond->params.arp_interval)
- queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
+ queue_delayed_work(bond->wq, &bond->arp_work,
+ msecs_to_jiffies(bond->params.arp_interval));
read_unlock(&bond->lock);
}
@@ -2762,33 +2765,21 @@ re_arm:
*
* Called with bond->lock held for read.
*/
-static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
+static int bond_ab_arp_inspect(struct bonding *bond)
{
unsigned long trans_start, last_rx;
struct slave *slave;
- int extra_ticks;
int commit = 0;
- /* All the time comparisons below need some extra time. Otherwise, on
- * fast networks the ARP probe/reply may arrive within the same jiffy
- * as it was sent. Then, the next time the ARP monitor is run, one
- * arp_interval will already have passed in the comparisons.
- */
- extra_ticks = delta_in_ticks / 2;
-
bond_for_each_slave(bond, slave) {
slave->new_link = BOND_LINK_NOCHANGE;
last_rx = slave_last_rx(bond, slave);
if (slave->link != BOND_LINK_UP) {
- if (time_in_range(jiffies,
- last_rx - delta_in_ticks,
- last_rx + delta_in_ticks + extra_ticks)) {
-
+ if (bond_time_in_interval(bond, last_rx, 1)) {
slave->new_link = BOND_LINK_UP;
commit++;
}
-
continue;
}
@@ -2797,9 +2788,7 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
* active. This avoids bouncing, as the last receive
* times need a full ARP monitor cycle to be updated.
*/
- if (time_in_range(jiffies,
- slave->jiffies - delta_in_ticks,
- slave->jiffies + 2 * delta_in_ticks + extra_ticks))
+ if (bond_time_in_interval(bond, slave->jiffies, 2))
continue;
/*
@@ -2817,10 +2806,7 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
*/
if (!bond_is_active_slave(slave) &&
!bond->current_arp_slave &&
- !time_in_range(jiffies,
- last_rx - delta_in_ticks,
- last_rx + 3 * delta_in_ticks + extra_ticks)) {
-
+ !bond_time_in_interval(bond, last_rx, 3)) {
slave->new_link = BOND_LINK_DOWN;
commit++;
}
@@ -2833,13 +2819,8 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
*/
trans_start = dev_trans_start(slave->dev);
if (bond_is_active_slave(slave) &&
- (!time_in_range(jiffies,
- trans_start - delta_in_ticks,
- trans_start + 2 * delta_in_ticks + extra_ticks) ||
- !time_in_range(jiffies,
- last_rx - delta_in_ticks,
- last_rx + 2 * delta_in_ticks + extra_ticks))) {
-
+ (!bond_time_in_interval(bond, trans_start, 2) ||
+ !bond_time_in_interval(bond, last_rx, 2))) {
slave->new_link = BOND_LINK_DOWN;
commit++;
}
@@ -2854,7 +2835,7 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
*
* Called with RTNL and bond->lock for read.
*/
-static void bond_ab_arp_commit(struct bonding *bond, int delta_in_ticks)
+static void bond_ab_arp_commit(struct bonding *bond)
{
unsigned long trans_start;
struct slave *slave;
@@ -2866,11 +2847,9 @@ static void bond_ab_arp_commit(struct bonding *bond, int delta_in_ticks)
case BOND_LINK_UP:
trans_start = dev_trans_start(slave->dev);
- if ((!bond->curr_active_slave &&
- time_in_range(jiffies,
- trans_start - delta_in_ticks,
- trans_start + delta_in_ticks + delta_in_ticks / 2)) ||
- bond->curr_active_slave != slave) {
+ if (bond->curr_active_slave != slave ||
+ (!bond->curr_active_slave &&
+ bond_time_in_interval(bond, trans_start, 1))) {
slave->link = BOND_LINK_UP;
if (bond->current_arp_slave) {
bond_set_slave_inactive_flags(
@@ -3011,7 +2990,7 @@ void bond_activebackup_arp_mon(struct work_struct *work)
should_notify_peers = bond_should_notify_peers(bond);
- if (bond_ab_arp_inspect(bond, delta_in_ticks)) {
+ if (bond_ab_arp_inspect(bond)) {
read_unlock(&bond->lock);
/* Race avoidance with bond_close flush of workqueue */
@@ -3024,7 +3003,7 @@ void bond_activebackup_arp_mon(struct work_struct *work)
read_lock(&bond->lock);
- bond_ab_arp_commit(bond, delta_in_ticks);
+ bond_ab_arp_commit(bond);
read_unlock(&bond->lock);
rtnl_unlock();
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] vlan: cleanup the usage of vlan_dev_priv(dev)
From: David Miller @ 2013-08-03 0:33 UTC (permalink / raw)
To: shhuiw; +Cc: kaber, netdev
In-Reply-To: <51FC4D52.7070607@gmail.com>
From: Wang Sheng-Hui <shhuiw@gmail.com>
Date: Sat, 03 Aug 2013 08:22:42 +0800
> This patch cleanup 2 points for the usage of vlan_dev_priv(dev):
> * In vlan_dev.c/vlan_dev_hard_header, we should use the var *vlan
> * directly
> after grabing the pointer at the beginning with
> *vlan = vlan_dev_priv(dev);
> when we need to access the fields of *vlan.
> * In vlan.c/register_vlan_device, add the var *vlan pointer
> struct vlan_dev_priv *vlan;
> to cleanup the code to access the fields of vlan_dev_priv(new_dev).
>
> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
This patch doesn't even come close to applying to the net-next tree:
patching file net/8021q/vlan.c
Hunk #1 FAILED at 210.
Hunk #2 succeeded at 260 with fuzz 2.
1 out of 2 hunks FAILED -- saving rejects to file net/8021q/vlan.c.rej
patching file net/8021q/vlan_dev.c
Hunk #1 FAILED at 107.
Hunk #2 FAILED at 133.
2 out of 2 hunks FAILED -- saving rejects to file net/8021q/vlan_dev.c.rej
Please respin your changes and resubmit, thanks.
^ permalink raw reply
* [PATCH] vlan: cleanup the usage of vlan_dev_priv(dev)
From: Wang Sheng-Hui @ 2013-08-03 0:22 UTC (permalink / raw)
To: Patrick McHardy, David S. Miller, netdev
This patch cleanup 2 points for the usage of vlan_dev_priv(dev):
* In vlan_dev.c/vlan_dev_hard_header, we should use the var *vlan directly
after grabing the pointer at the beginning with
*vlan = vlan_dev_priv(dev);
when we need to access the fields of *vlan.
* In vlan.c/register_vlan_device, add the var *vlan pointer
struct vlan_dev_priv *vlan;
to cleanup the code to access the fields of vlan_dev_priv(new_dev).
Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
---
net/8021q/vlan.c | 12 +++++++-----
net/8021q/vlan_dev.c | 6 +++---
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 2fb2d88..b9d3d7e 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -210,6 +210,7 @@ out_vid_del:
static int register_vlan_device(struct net_device *real_dev, u16 vlan_id)
{
struct net_device *new_dev;
+ struct vlan_dev_priv *vlan;
struct net *net = dev_net(real_dev);
struct vlan_net *vn = net_generic(net, vlan_net_id);
char name[IFNAMSIZ];
@@ -260,11 +261,12 @@ static int register_vlan_device(struct net_device *real_dev, u16 vlan_id)
new_dev->mtu = real_dev->mtu;
new_dev->priv_flags |= (real_dev->priv_flags & IFF_UNICAST_FLT);
- vlan_dev_priv(new_dev)->vlan_proto = htons(ETH_P_8021Q);
- vlan_dev_priv(new_dev)->vlan_id = vlan_id;
- vlan_dev_priv(new_dev)->real_dev = real_dev;
- vlan_dev_priv(new_dev)->dent = NULL;
- vlan_dev_priv(new_dev)->flags = VLAN_FLAG_REORDER_HDR;
+ vlan = vlan_dev_priv(new_dev);
+ vlan->vlan_proto = htons(ETH_P_8021Q);
+ vlan->vlan_id = vlan_id;
+ vlan->real_dev = real_dev;
+ vlan->dent = NULL;
+ vlan->flags = VLAN_FLAG_REORDER_HDR;
new_dev->rtnl_link_ops = &vlan_link_ops;
err = register_vlan_dev(new_dev);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 1cd3d2a..9ab8a7e 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -107,10 +107,10 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
u16 vlan_tci = 0;
int rc;
- if (!(vlan_dev_priv(dev)->flags & VLAN_FLAG_REORDER_HDR)) {
+ if (!(vlan->flags & VLAN_FLAG_REORDER_HDR)) {
vhdr = (struct vlan_hdr *) skb_push(skb, VLAN_HLEN);
- vlan_tci = vlan_dev_priv(dev)->vlan_id;
+ vlan_tci = vlan->vlan_id;
vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb);
vhdr->h_vlan_TCI = htons(vlan_tci);
@@ -133,7 +133,7 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
saddr = dev->dev_addr;
/* Now make the underlying real hard header */
- dev = vlan_dev_priv(dev)->real_dev;
+ dev = vlan->real_dev;
rc = dev_hard_header(skb, dev, type, daddr, saddr, len + vhdrlen);
if (rc > 0)
rc += vhdrlen;
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH net] bonding: fix arp monitoring with vlan slaves
From: Nikolay Aleksandrov @ 2013-08-03 0:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, andy, fubar
In-Reply-To: <1375488223.4457.16.camel@edumazet-glaptop>
On 08/03/2013 02:03 AM, Eric Dumazet wrote:
> On Sat, 2013-08-03 at 01:29 +0200, Nikolay Aleksandrov wrote:
>
>> I knew it was because of the LLTX, but I was wondering about the possible
>> reasons for the xmit_lock_owner check.
>> So basically the arp monitoring (or any dev_trans_start code) won't work
>> with LLTX devices because they don't get their trans_start updated (not the
>> txq trans_start nor the dev->trans_start), is this correct ?
>
> Nope : LLTX devices are supposed to update their own dev->trans_start
> I added for these case a special comment as in :
>
> drivers/net/ethernet/atheros/atl1e/atl1e_main.c:1883: netdev->trans_start = jiffies; /* NETIF_F_LLTX driver :( */
>
Ah, didn't know about that, so there could be an LLTX device with proper
trans_start if it updates it itself. Fair enough.
> trans_start is a txq property, and vlan devices have a single txq.
>
> Updating the vlandev->trans_start or the vlantxq->trans_start would be a
> performance killer on MQ ethernet.
>
Yeah, that part I got, that's why I said it explains a lot for me earlier :-)
> And frankly, as this trans_start is really seldom queried, it makes no
> sense to set it on fast path on the vlan device, if its properly done on
> the real device anyway.
>
>> But what if the txqs get bound to a particular CPU, then the txq
>> trans_start is okay to be updated I suppose.
>
> Not sure what you are saying. vlan xmit can be called from any cpus.
>
Never mind, I didn't notice that the 8021q dev has a single txq as you said.
Thanks for taking the time to explain all this.
^ permalink raw reply
* Re: [PATCH net] bonding: fix arp monitoring with vlan slaves
From: Eric Dumazet @ 2013-08-03 0:03 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: David Miller, netdev, andy, fubar
In-Reply-To: <51FC40E5.6050809@redhat.com>
On Sat, 2013-08-03 at 01:29 +0200, Nikolay Aleksandrov wrote:
> I knew it was because of the LLTX, but I was wondering about the possible
> reasons for the xmit_lock_owner check.
> So basically the arp monitoring (or any dev_trans_start code) won't work
> with LLTX devices because they don't get their trans_start updated (not the
> txq trans_start nor the dev->trans_start), is this correct ?
Nope : LLTX devices are supposed to update their own dev->trans_start
I added for these case a special comment as in :
drivers/net/ethernet/atheros/atl1e/atl1e_main.c:1883: netdev->trans_start = jiffies; /* NETIF_F_LLTX driver :( */
trans_start is a txq property, and vlan devices have a single txq.
Updating the vlandev->trans_start or the vlantxq->trans_start would be a
performance killer on MQ ethernet.
And frankly, as this trans_start is really seldom queried, it makes no
sense to set it on fast path on the vlan device, if its properly done on
the real device anyway.
> But what if the txqs get bound to a particular CPU, then the txq
> trans_start is okay to be updated I suppose.
Not sure what you are saying. vlan xmit can be called from any cpus.
^ permalink raw reply
* Re: [PATCH net] bonding: fix arp monitoring with vlan slaves
From: Jay Vosburgh @ 2013-08-02 23:59 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: Eric Dumazet, David Miller, netdev, andy
In-Reply-To: <51FC40E5.6050809@redhat.com>
Nikolay Aleksandrov <nikolay@redhat.com> wrote:
>On 08/03/2013 01:17 AM, Eric Dumazet wrote:
>> On Fri, 2013-08-02 at 15:32 -0700, David Miller wrote:
>>
>>> Handling this specially in bonding isn't really ideal.
>>>
>>> Please either hide this detail in dev_trans_start(), or (preferrably)
>>> have vlan_dev_hard_start_xmit() set the trans_start timestamp
>>> properly thus making this just work for everything.
>>
>> vlan is LLTX, so setting the timestamp would incur false sharing on
>> multiqueue.
>>
>Yeah, this statement actually explains a lot for me, thanks :-)
>I knew it was because of the LLTX, but I was wondering about the possible
>reasons for the xmit_lock_owner check.
>So basically the arp monitoring (or any dev_trans_start code) won't work
>with LLTX devices because they don't get their trans_start updated (not the
>txq trans_start nor the dev->trans_start), is this correct ?
I think what Eric means is that it'll be a performance problem,
because the cache line with the trans_start field will be thrashed
between CPUs as updates compete with each other or inspection from
dev_trans_start. I suspect it will work from a functional point of view
(unless I'm missing something).
As a practical matter, it looks like bonding is at present the
only caller of dev_trans_start that passes either (a) a VLAN device, or
(b) a device other than itself (most drivers seem to use it for transmit
timeout detection on their own device).
Having software devices set trans_start seems unnecessary
(bonding doesn't set it, either), since they'll generally have a
hardware device underneath with a real trans_start value. It might be
hard to hunt down for tun, though.
>But what if the txqs get bound to a particular CPU, then the txq
>trans_start is okay to be updated I suppose.
>
>Nik
>> But it's true we need a helper, because many callers do
>>
>> if (dev->priv_flags & IFF_802_1Q_VLAN)
>> dev = vlan_dev_real_dev(dev);
>>
>> or the slighly better
>>
>> if (is_vlan_dev(dev))
>> dev = vlan_dev_real_dev(dev);
For just the bonding dev_trans_start() for a VLAN case, one of
the above in dev_trans_start() ought to be sufficient.
-J
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* Re: [PATCH] netlabel: use domain based selectors when address based selectors are not available
From: David Miller @ 2013-08-02 23:57 UTC (permalink / raw)
To: pmoore; +Cc: netdev, linux-security-module, selinux
In-Reply-To: <20130802184508.22429.94026.stgit@localhost>
From: Paul Moore <pmoore@redhat.com>
Date: Fri, 02 Aug 2013 14:45:08 -0400
> NetLabel has the ability to selectively assign network security labels
> to outbound traffic based on either the LSM's "domain" (different for
> each LSM), the network destination, or a combination of both. Depending
> on the type of traffic, local or forwarded, and the type of traffic
> selector, domain or address based, different hooks are used to label the
> traffic; the goal being minimal overhead.
>
> Unfortunately, there is a bug such that a system using NetLabel domain
> based traffic selectors does not correctly label outbound local traffic
> that is not assigned to a socket. The issue is that in these cases
> the associated NetLabel hook only looks at the address based selectors
> and not the domain based selectors. This patch corrects this by
> checking both the domain and address based selectors so that the correct
> labeling is applied, regardless of the configuration type.
>
> In order to acomplish this fix, this patch also simplifies some of the
> NetLabel domainhash structures to use a more common outbound traffic
> mapping type: struct netlbl_dommap_def. This simplifies some of the code
> in this patch and paves the way for further simplifications in the
> future.
>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
Applied, thanks Paul.
^ permalink raw reply
* Re: [PATCH net] bonding: fix arp monitoring with vlan slaves
From: Nikolay Aleksandrov @ 2013-08-02 23:29 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, andy, fubar
In-Reply-To: <1375485466.4457.4.camel@edumazet-glaptop>
On 08/03/2013 01:17 AM, Eric Dumazet wrote:
> On Fri, 2013-08-02 at 15:32 -0700, David Miller wrote:
>
>> Handling this specially in bonding isn't really ideal.
>>
>> Please either hide this detail in dev_trans_start(), or (preferrably)
>> have vlan_dev_hard_start_xmit() set the trans_start timestamp
>> properly thus making this just work for everything.
>
> vlan is LLTX, so setting the timestamp would incur false sharing on
> multiqueue.
>
Yeah, this statement actually explains a lot for me, thanks :-)
I knew it was because of the LLTX, but I was wondering about the possible
reasons for the xmit_lock_owner check.
So basically the arp monitoring (or any dev_trans_start code) won't work
with LLTX devices because they don't get their trans_start updated (not the
txq trans_start nor the dev->trans_start), is this correct ?
But what if the txqs get bound to a particular CPU, then the txq
trans_start is okay to be updated I suppose.
Nik
> But it's true we need a helper, because many callers do
>
> if (dev->priv_flags & IFF_802_1Q_VLAN)
> dev = vlan_dev_real_dev(dev);
>
> or the slighly better
>
> if (is_vlan_dev(dev))
> dev = vlan_dev_real_dev(dev);
>
>
>
^ permalink raw reply
* Re: [PATCH net] bonding: fix arp monitoring with vlan slaves
From: Eric Dumazet @ 2013-08-02 23:17 UTC (permalink / raw)
To: David Miller; +Cc: nikolay, netdev, andy, fubar
In-Reply-To: <20130802.153212.1340051334413929810.davem@davemloft.net>
On Fri, 2013-08-02 at 15:32 -0700, David Miller wrote:
> Handling this specially in bonding isn't really ideal.
>
> Please either hide this detail in dev_trans_start(), or (preferrably)
> have vlan_dev_hard_start_xmit() set the trans_start timestamp
> properly thus making this just work for everything.
vlan is LLTX, so setting the timestamp would incur false sharing on
multiqueue.
But it's true we need a helper, because many callers do
if (dev->priv_flags & IFF_802_1Q_VLAN)
dev = vlan_dev_real_dev(dev);
or the slighly better
if (is_vlan_dev(dev))
dev = vlan_dev_real_dev(dev);
^ permalink raw reply
* Re: [PATCH net-next] cnic, bnx2i: Fix bug on some bnx2x devices that don't support iSCSI
From: David Miller @ 2013-08-02 22:56 UTC (permalink / raw)
To: mchan; +Cc: netdev, eddie.wai
In-Reply-To: <1375468103-12341-1-git-send-email-mchan@broadcom.com>
From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 2 Aug 2013 11:28:23 -0700
> On some bnx2x devices, iSCSI is determined to be unsupported only after
> firmware is downloaded. We need to check max_iscsi_conn again after
> NETDEV_UP and block iSCSI init operations. Without this fix, iscsiadm
> can hang as the firmware will not respond to the iSCSI init message.
>
> Signed-off-by: Eddie Wai <eddie.wai@broadcom.com>
> Signed-off-by: Michael Chan <mchan@broadcom.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next 0/2] fix bonding neighbour setup handling
From: David Miller @ 2013-08-02 22:45 UTC (permalink / raw)
To: vfalico; +Cc: netdev, fubar, andy, ebiederm, joe
In-Reply-To: <1375463259-12033-1-git-send-email-vfalico@redhat.com>
From: Veaceslav Falico <vfalico@redhat.com>
Date: Fri, 2 Aug 2013 19:07:37 +0200
> Recent patches revealed an old bug, which was there for quite awhile. It's
> related to vlan on top of bonding and ndo_neigh_setup(). When vlan device
> is initiated, it calls its real_dev->ndo_neigh_setup(), and in case of
> bonding - it will modify neigh_parms->neigh_setup to point to
> bond_neigh_init, while neigh_parms are of vlan's dev.
>
> This way, when neigh_parms->neigh_setup() of vlan's dev is called, the
> bonding function will be called, which expects the dev to be struct
> bonding, but will receive a vlan dev.
>
> It was hidden before because of bond->first_slave usage. Now, with
> Nikolay's conversion to list/RCU, first_slave is gone and we hit a null
> pointer dereference when working with lists/slave.
>
> First patch moves ndo_neigh_setup() in neigh_parms_alloc() to the bottom,
> so that the ->dev will be available to the caller. It doesn't really change
> anything, however is needed for the second patch.
>
> Second patch makes bond_neigh_setup() (bond->ndo_neigh_setup()) check if
> the neigh_parms are really from a bonding dev, and only modify the
> neigh_setup in this case.
Thanks for the detailed explanation of the situation, it made your patches
trivial to review.
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net] net: mlx5: fix sizeof usage in health_care's reg_handler
From: Daniel Borkmann @ 2013-08-02 22:43 UTC (permalink / raw)
To: ogerlitz; +Cc: netdev, David Miller
In-Reply-To: <20130802.151132.539136654627638979.davem@davemloft.net>
On 08/03/2013 12:11 AM, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Fri, 2 Aug 2013 12:16:17 +0200
>
>> Therefore, I strongly assume sizeof(*health->health) is being meant
>> to be passed as an argument. Interestingly, there are actually no
>> in-tree users of mlx5_[un]register_health_report_handler(), but some
>> debugging modules might want to know the correct size instead.
>
> I want these hooks and infrastructure removed immediately.
>
> If there are no in-tree users there is no reason for them to
> exist at all.
Ok, I let Or handle that. Maybe he wants to add a user of it, instead.
I also noticed that coverty scanner found a couple of other issues,
e.g. outlen_write() in mlx5/core/cmd.c does a kzalloc() without
doing sanity checks on the user-passed allocation size, e.g. it
could even be a negative value passed to it.
^ permalink raw reply
* Re: [PATCH net] bonding: fix arp monitoring with vlan slaves
From: Nikolay Aleksandrov @ 2013-08-02 22:40 UTC (permalink / raw)
To: David Miller; +Cc: netdev, andy, fubar
In-Reply-To: <20130802.153212.1340051334413929810.davem@davemloft.net>
On 08/03/2013 12:32 AM, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@redhat.com>
> Date: Fri, 2 Aug 2013 18:41:05 +0200
>
>> From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
>>
>> When arp monitoring is enabled the bonding relies on slaves'
>> dev_trans_start() value to check if the slave link is up or not, but for
>> 8021q devices that value is either stale or 0, and can't be used. So use
>> the 8021q's underlying device value.
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>
> Handling this specially in bonding isn't really ideal.
>
Indeed, it's not really a bonding problem.
> Please either hide this detail in dev_trans_start(), or (preferrably)
> have vlan_dev_hard_start_xmit() set the trans_start timestamp
> properly thus making this just work for everything.
>
> Thanks.
>
Yep, I prefer option 2 as well. I will submit a patch tomorrow, thank you
for the suggestions.
Nik
^ permalink raw reply
* Re: [PATCH net] bonding: fix arp monitoring with vlan slaves
From: David Miller @ 2013-08-02 22:32 UTC (permalink / raw)
To: nikolay; +Cc: netdev, andy, fubar
In-Reply-To: <1375461665-4186-1-git-send-email-nikolay@redhat.com>
From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Fri, 2 Aug 2013 18:41:05 +0200
> From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
>
> When arp monitoring is enabled the bonding relies on slaves'
> dev_trans_start() value to check if the slave link is up or not, but for
> 8021q devices that value is either stale or 0, and can't be used. So use
> the 8021q's underlying device value.
>
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
Handling this specially in bonding isn't really ideal.
Please either hide this detail in dev_trans_start(), or (preferrably)
have vlan_dev_hard_start_xmit() set the trans_start timestamp
properly thus making this just work for everything.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: netlink: minor: remove unused pointer in alloc_pg_vec
From: David Miller @ 2013-08-02 22:26 UTC (permalink / raw)
To: dborkman; +Cc: netdev
In-Reply-To: <1375457559-7565-1-git-send-email-dborkman@redhat.com>
From: Daniel Borkmann <dborkman@redhat.com>
Date: Fri, 2 Aug 2013 17:32:39 +0200
> Variable ptr is being assigned, but never used, so just remove it.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Applied, thanks Daniel.
^ permalink raw reply
* Re: [PATCH v2] fib_rules: add route suppression based on ifgroup
From: David Miller @ 2013-08-02 22:24 UTC (permalink / raw)
To: stefan.tomanek; +Cc: netdev
In-Reply-To: <20130802151956.GK21970@zirkel.wertarbyte.de>
From: Stefan Tomanek <stefan.tomanek@wertarbyte.de>
Date: Fri, 2 Aug 2013 17:19:56 +0200
> This change adds the ability to suppress a routing decision based upon the
> interface group the selected interface belongs to. This allows it to
> exclude specific devices from a routing decision.
>
> Signed-off-by: Stefan Tomanek <stefan.tomanek@wertarbyte.de>
Looks fine, applied, thanks.
^ permalink raw reply
* Re: [PATCH] net: check net.core.somaxconn sysctl values
From: David Miller @ 2013-08-02 22:19 UTC (permalink / raw)
To: klamm; +Cc: eric.dumazet, raise.sail, ebiederm, netdev, linux-kernel
In-Reply-To: <1375454200-21303-1-git-send-email-klamm@yandex-team.ru>
From: Roman Gushchin <klamm@yandex-team.ru>
Date: Fri, 2 Aug 2013 18:36:40 +0400
> It's possible to assign an invalid value to the net.core.somaxconn
> sysctl variable, because there is no checks at all.
>
> The sk_max_ack_backlog field of the sock structure is defined as
> unsigned short. Therefore, the backlog argument in inet_listen()
> shouldn't exceed USHRT_MAX. The backlog argument in the listen() syscall
> is truncated to the somaxconn value. So, the somaxconn value shouldn't
> exceed 65535 (USHRT_MAX).
> Also, negative values of somaxconn are meaningless.
>
> before:
> $ sysctl -w net.core.somaxconn=256
> net.core.somaxconn = 256
> $ sysctl -w net.core.somaxconn=65536
> net.core.somaxconn = 65536
> $ sysctl -w net.core.somaxconn=-100
> net.core.somaxconn = -100
>
> after:
> $ sysctl -w net.core.somaxconn=256
> net.core.somaxconn = 256
> $ sysctl -w net.core.somaxconn=65536
> error: "Invalid argument" setting key "net.core.somaxconn"
> $ sysctl -w net.core.somaxconn=-100
> error: "Invalid argument" setting key "net.core.somaxconn"
>
> Based on a prior patch from Changli Gao.
>
> Signed-off-by: Roman Gushchin <klamm@yandex-team.ru>
> Reported-by: Changli Gao <xiaosuo@gmail.com>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next 2/2] icmpv6_filter: allow ICMPv6 messages with bodies < 4 bytes
From: David Miller @ 2013-08-02 22:16 UTC (permalink / raw)
To: werner; +Cc: netdev
In-Reply-To: <9ad5a4d95d70d8f75127207d796a544aed1947cc.1375417279.git.werner@almesberger.net>
From: Werner Almesberger <werner@almesberger.net>
Date: Fri, 2 Aug 2013 10:51:34 -0300
> By using sizeof(_hdr), net/ipv6/raw.c:icmpv6_filter implicitly assumes
> that any valid ICMPv6 message is at least eight bytes long, i.e., that
> the message body is at least four bytes.
>
> The DIS message of RPL (RFC 6550 section 6.2, from the 6LoWPAN world),
> has a minimum length of only six bytes, and is thus blocked by
> icmpv6_filter.
>
> RFC 4443 seems to allow even a zero-sized body, making the minimum
> allowable message size four bytes.
>
> Signed-off-by: Werner Almesberger <werner@almesberger.net>
Applied, thanks Werner.
^ permalink raw reply
* Re: [PATCH net-next 1/2] icmpv6_filter: fix "_hdr" incorrectly being a pointer
From: David Miller @ 2013-08-02 22:16 UTC (permalink / raw)
To: werner; +Cc: netdev
In-Reply-To: <45f3417354a033cf71cb5e70b5cdf8c9e542e234.1375417279.git.werner@almesberger.net>
From: Werner Almesberger <werner@almesberger.net>
Date: Fri, 2 Aug 2013 10:51:19 -0300
> "_hdr" should hold the ICMPv6 header while "hdr" is the pointer to it.
> This worked by accident.
>
> Signed-off-by: Werner Almesberger <werner@almesberger.net>
Applied.
^ permalink raw reply
* Re: [PATCH net] net: mlx5: fix sizeof usage in health_care's reg_handler
From: David Miller @ 2013-08-02 22:11 UTC (permalink / raw)
To: dborkman; +Cc: netdev, ogerlitz
In-Reply-To: <1375438577-30933-1-git-send-email-dborkman@redhat.com>
From: Daniel Borkmann <dborkman@redhat.com>
Date: Fri, 2 Aug 2013 12:16:17 +0200
> Therefore, I strongly assume sizeof(*health->health) is being meant
> to be passed as an argument. Interestingly, there are actually no
> in-tree users of mlx5_[un]register_health_report_handler(), but some
> debugging modules might want to know the correct size instead.
I want these hooks and infrastructure removed immediately.
If there are no in-tree users there is no reason for them to
exist at all.
Thanks.
^ permalink raw reply
* Re: [PATCH v3] sis900: Fix the tx queue timeout issue
From: David Miller @ 2013-08-02 22:04 UTC (permalink / raw)
To: bhutchings; +Cc: kda, venza, B38611, netdev
In-Reply-To: <1375471135.32254.15.camel@deadeye.wl.decadent.org.uk>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 2 Aug 2013 21:18:55 +0200
> This looks reasonable. It looks like link changes now work like this:
>
> 1. When sis900_timer() detects link-down, it calls netif_carrier_off()
> but does not clear autong_complete.
> 2. When sis900_timer() detects link-up, it calls sis900_check_mode()
> which restarts autonegotiation and clears autong_complete.
> 3. sis900_timer() will now call sis900_read_mode(). When that detects
> link-up, it sets autong_complete and calls netif_carrier_on().
>
> This patch has moved the call to netif_carrier_on() from step 2 to step
> 3. However, I don't understand why autonegotiation is restarted in step
> 2. When autonegotiation is enabled, the PHY should not indicate link-up
> until it has completed. Perhaps this is a necessary workaround for a
> hardware bug. Otherwise it's a waste of time.
Agreed, if the PHY was configured to use autonegotiation, getting a link
up event afterwards implies that autonegotiation is complete.
In any event, I'm going to apply this patch anyways because it is far and
above a significant move forward.
^ permalink raw reply
* Re: [PATCH v2 3/3] af_packet: simplify VLAN frame check in packet_snd
From: David Miller @ 2013-08-02 21:59 UTC (permalink / raw)
To: phil; +Cc: netdev
In-Reply-To: <1375436261-6746-3-git-send-email-phil@nwl.cc>
From: Phil Sutter <phil@nwl.cc>
Date: Fri, 2 Aug 2013 11:37:41 +0200
> For ethernet frames, eth_type_trans() already parses the header, so one
> can skip this when checking the frame size.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
Applied.
^ permalink raw reply
* Re: [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap
From: David Miller @ 2013-08-02 21:58 UTC (permalink / raw)
To: phil; +Cc: netdev
In-Reply-To: <1375436261-6746-2-git-send-email-phil@nwl.cc>
From: Phil Sutter <phil@nwl.cc>
Date: Fri, 2 Aug 2013 11:37:40 +0200
> Since tpacket_fill_skb() parses the protocol field in ethernet frames'
> headers, it's easy to see if any passed frame is a VLAN one and account
> for the extended size.
>
> But as the real protocol does not turn up before tpacket_fill_skb()
> runs which in turn also checks the frame length, move the max frame
> length calculation into the function.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
Applied.
^ permalink raw reply
* Re: [PATCH v2 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol
From: David Miller @ 2013-08-02 21:58 UTC (permalink / raw)
To: phil; +Cc: netdev
In-Reply-To: <1375436261-6746-1-git-send-email-phil@nwl.cc>
From: Phil Sutter <phil@nwl.cc>
Date: Fri, 2 Aug 2013 11:37:39 +0200
> This may be necessary when the SKB is passed to other layers on the go,
> which check the protocol field on their own. An example is a VLAN packet
> sent out using AF_PACKET on a bridge interface. The bridging code checks
> the SKB size, accounting for any VLAN header only if the protocol field
> is set accordingly.
>
> Note that eth_type_trans() sets skb->dev to the passed argument, so this
> can be skipped in packet_snd() for ethernet frames, as well.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
Applied.
^ permalink raw reply
* Re: [PATCH net] net: rtm_to_ifaddr: free ifa if ifa_cacheinfo processing fails
From: David Miller @ 2013-08-02 21:56 UTC (permalink / raw)
To: jiri; +Cc: dborkman, netdev
In-Reply-To: <20130802102229.GB1586@minipsycho.orion>
From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 2 Aug 2013 12:22:29 +0200
> Fri, Aug 02, 2013 at 11:32:43AM CEST, dborkman@redhat.com wrote:
>>Commit 5c766d642 ("ipv4: introduce address lifetime") leaves the ifa
>>resource that was allocated via inet_alloc_ifa() unfreed when returning
>>the function with -EINVAL. Thus, free it first via inet_free_ifa().
>>
>>Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Reviewed-by: Jiri Pirko <jiri@resnulli.us>
Applied and queued up for -stable, thanks.
^ 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