* Re: [patch net-next-2.6 4/8] bonding: wrap slave state work
From: Nicolas de Pesloüan @ 2011-03-08 21:23 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, shemminger, kaber, fubar, eric.dumazet, andy
In-Reply-To: <20110308071805.GB2826@psychotron.redhat.com>
Le 08/03/2011 08:18, Jiri Pirko a écrit :
> Mon, Mar 07, 2011 at 08:55:12PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 07/03/2011 10:58, Jiri Pirko a écrit :
>>>>>
>>>>> +static inline void bond_set_active_slave(struct slave *slave)
>>>>> +{
>>>>> + slave->backup = 0;
>>>>
>>>> In the comment above, you said that the possible value for backup
>>>> corresponds with BOND_STATE_ACTIVE and BOND_STATE_BACKUP.
>>>>
>>>> So, should be:
>>>>
>>>> slave->backup = BOND_STATE_ACTIVE;
>>>>
>>>>> +}
>>>>> +
>>>>> +static inline void bond_set_backup_slave(struct slave *slave)
>>>>> +{
>>>>> + slave->backup = 1;
>>>>
>>>> slave->backup = BOND_STATE_BACKUP;
>>>>
>>>
>>> Well, I think it's weird and misleading to assign some define to :1
>>> bitfield. Should be 0 or 1, nothing else.
>>
>> Agreed, but the comment appears missleading... May be you should fix the comment, not the code.
>
> Hmm. I thought that the comment is accurate. BOND_STATE_ACTIVE
> corresponds with 0, BOND_STATE_BACKUP corresponds with 1. Anyway, let me
> know how would you like to formulate this and I can repost (or do a
> little comment-changing followup)
>
> Thanks Nicolas
Hi Jiri,
I'm probably too perfectionist. Keep it the way it is.
Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
^ permalink raw reply
* Re: [Patch V2] bonding: fix netpoll in active-backup mode
From: Andy Gospodarek @ 2011-03-08 21:24 UTC (permalink / raw)
To: Amerigo Wang; +Cc: linux-kernel, Neil Horman, Jay Vosburgh, netdev
In-Reply-To: <1299578336-5888-1-git-send-email-amwang@redhat.com>
On Tue, Mar 08, 2011 at 05:58:56PM +0800, Amerigo Wang wrote:
> V2: avoid calling slave_diable_netpoll() with write_lock_bh() held.
>
> netconsole doesn't work in active-backup mode, because we don't do anything
> for nic failover in active-backup mode. We should disable netpoll on the
> failing slave when it is detected down and enable netpoll when it becomes
> the active slave.
>
> Tested by ifdown the current active slave and ifup it again for several times,
> netconsole works well.
>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
>
It seems like you are going to a lot of trouble to fix a bug where
netpoll will not be setup on any interface that is down when enslaved.
That seems to be the only path that would not have slave->np setup
properly at enslavement.
Did you ever try just this?
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 0592e6d..8d93044 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1352,8 +1352,6 @@ static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni)
read_lock(&bond->lock);
bond_for_each_slave(bond, slave, i) {
- if (!IS_UP(slave->dev))
- continue;
err = slave_enable_netpoll(slave);
if (err) {
__bond_netpoll_cleanup(bond);
^ permalink raw reply related
* Re: [PATCH 1/8] Phonet: fix NULL-deref in a8059512b120362b15424f152b2548fe8b11bd0c
From: Rémi Denis-Courmont @ 2011-03-08 21:30 UTC (permalink / raw)
To: netdev
In-Reply-To: <20110308.130926.246531377.davem@davemloft.net>
Le mardi 8 mars 2011 23:09:26 David Miller, vous avez écrit :
> From: "Rémi Denis-Courmont" <remi@remlab.net>
> Date: Tue, 8 Mar 2011 22:59:41 +0200
>
> > I just wish you'd been as strict back then.
>
> The only thing worse that putting crappy code into the tree is having
> patches rot in patchwork because the maintainer isn't responsive at
> reviewing things.
Patch submitted:
Date: Mon, 27 Sep 2010 10:37:59 +0530
Patch merged:
Date: Mon, 27 Sep 2010 21:50:59 -0700
So the patches had been "rotting" for 0 days 23:13:00. Come on.
--
Rémi Denis-Courmont
http://www.remlab.info/
http://fi.linkedin.com/in/remidenis
^ permalink raw reply
* Re: [PATCH 11/17] mlx4_en: Enabling new steering
From: Roland Dreier @ 2011-03-08 21:30 UTC (permalink / raw)
To: Or Gerlitz; +Cc: Yevgeny Petrilin, davem, netdev, vlad, nirmu, Aleksey Senin
In-Reply-To: <AANLkTikftARWMbmy8zHbRShBbbnQhXhiequzdR_aPBst@mail.gmail.com>
On Tue, Mar 8, 2011 at 1:10 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> + if (mdev->dev->caps.vep_mc_steering && ibqp->qp_type == IB_QPT_RAW_ETHERTYPE) {
>
> Guys,
>
> Didn't you want to refer here to the "raw ethernet QP" which is typed
> to IB_QPT_RAW_PACKET by the series posted on December last year, see
> http://marc.info/?l=linux-rdma&m=129162076818154 --
> IB_QPT_RAW_ETHERTYPE is a rename of IB's IB_QPT_RAW_ETY done by
> commit a2ebf07ae53e65bd073f96877e4818f2e89271ae
Good catch. I think the drivers/infiniband part of this patch should
just be dropped -- we don't have any raw QP support of any kind, right?
- R.
^ permalink raw reply
* Re: [PATCH 1/8] Phonet: fix NULL-deref in a8059512b120362b15424f152b2548fe8b11bd0c
From: David Miller @ 2011-03-08 21:34 UTC (permalink / raw)
To: remi; +Cc: netdev
In-Reply-To: <201103082330.31652.remi@remlab.net>
From: "Rémi Denis-Courmont" <remi@remlab.net>
Date: Tue, 8 Mar 2011 23:30:31 +0200
> Le mardi 8 mars 2011 23:09:26 David Miller, vous avez écrit :
>> From: "Rémi Denis-Courmont" <remi@remlab.net>
>> Date: Tue, 8 Mar 2011 22:59:41 +0200
>>
>> > I just wish you'd been as strict back then.
>>
>> The only thing worse that putting crappy code into the tree is having
>> patches rot in patchwork because the maintainer isn't responsive at
>> reviewing things.
>
> Patch submitted:
> Date: Mon, 27 Sep 2010 10:37:59 +0530
>
> Patch merged:
> Date: Mon, 27 Sep 2010 21:50:59 -0700
>
> So the patches had been "rotting" for 0 days 23:13:00. Come on.
I gave you nearly a full day.
If I do my daily email scan twice, and see the same patch on both
occaisions without a review I am very likely to merge the thing
or poke the maintainer.
^ permalink raw reply
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Nicolas de Pesloüan @ 2011-03-08 21:34 UTC (permalink / raw)
To: Andy Gospodarek
Cc: Jiri Pirko, netdev, davem, shemminger, kaber, fubar, eric.dumazet
In-Reply-To: <20110308024356.GV11864@gospo.rdu.redhat.com>
Le 08/03/2011 03:43, Andy Gospodarek a écrit :
> On Tue, Mar 08, 2011 at 12:09:48AM +0100, Nicolas de Pesloüan wrote:
>>
>> After Jiri's last patch series, bond_arp_rcv() is not registered anymore
>> as a protocol handler on bond0, but directly called from inside
>> bond_handle_frame(), through bond->recv_probe.
>>
>> Because bond_handler_frame() is a rx_handler for the slave interfaces,
>> bond_arp_rcv() is now called at the slave level and not a the master
>> level anymore.
>>
>> Hence this patch and the reason I thought it should work.
>>
>> Did you tested this patch with Jiri's previous patches applied before?
>>
>
> No, I have not tested them yet. I looked at this problem first as
> Jiri's patch did not fix a regression in code that is actually in
> net-next right now.
For as far as I understand, the regression was introduced by Jiri's patches that are not yet
accepted in net-next-2.6, so this path definitely need the full patch series to be applied before.
And there is no special urgency to fix this regression, because it doesn't "exist" yet anywhere
except in some sandboxes.
This patch change the reinjection to inject at the slave level instead of the master level, because
in Jiri's patch series, ARP handling moved from a normal protocol handler registered at the master
level to the rx_handler at the slave level.
Jiri, do I miss something ?
> I don't have a problem with the work you and Jiri are doing on this
> latest patch series, but the frequency of patch revisions and now
> regressions concerns me.
I agree with your concerns, and this is the reason why Jiri and I are working hard to try and fight
every possible problems in the whole package of patch. I think that, at the end, Jiri and I will
have to properly resubmit the whole thing, for the convenient of those who will have to decide to
accept it or not.
Nicolas.
^ permalink raw reply
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards
From: Neil Horman @ 2011-03-08 21:40 UTC (permalink / raw)
To: Michael Chan
Cc: Ben Hutchings, netdev@vger.kernel.org, David S. Miller,
Dmitry Kravkov, Eddie Wai, Eilon Greenstein
In-Reply-To: <1299613396.12601.21.camel@HP1>
On Tue, Mar 08, 2011 at 11:43:16AM -0800, Michael Chan wrote:
>
> On Tue, 2011-03-08 at 11:36 -0800, Neil Horman wrote:
> > On Tue, Mar 08, 2011 at 11:07:57AM -0800, Michael Chan wrote:
> > >
> > > On Tue, 2011-03-08 at 10:59 -0800, Ben Hutchings wrote:
> > > > On Tue, 2011-03-08 at 13:56 -0500, Neil Horman wrote:
> > > > > bnx2 cards can work with the cnic driver, but when the cnic driver detects a
> > > > > bnx2 card, is_cnic_dev erroneously calls the initalization routines for both
> > > > > bnx2 and bnx2x (the former being a regex subset of the later).
> > > >
> > > > Since when does strcmp() do a regex match?
> > >
> > > Yeah, strcmp() does NULL-terminated string compare, right?
> > >
> > Sorry, poor choice of words. It doesn't do a regex match, I only ment to
> > illustrate that bnx2 is a substring of bnx2x.
> >
> > strcmp does this:
> >
> > strcmp(const char *str1, const char *str2) {
> > ...
> > while (*s1 || *s2) {
> > ...
> > }
> > ...
> > }
> >
> > Since bnx2 is a substring of bnx2 both if clauses are a match, since you'll hit
> > the null terminator of the shorter string in both, which means we call both
> > initalization functions.
>
> No, that should not happen. When we compare NULL-terminated "bnx2" with
> NULL-terminated "bnx2x", it won't match because the '\0' won't match the
> 'x', right?
>
> I think the patch is good. We can avoid the unnecessary strcmp() when
> we have a match with "bnx2" already. But we should not get both matches
> even without the patch.
>
Crap, sorry, I rescind this (although you're right, it probably wouldn't hurt to
roll this change in, just to save a potential second strcmp). Anywho, I'm
getting this error on the ifup of my bnx2 interface:
bnx2i: iSCSI not supported, dev=eth1
That message appears to only get set (from my read) on cards driven by the bnx2x
driver, and when I saw the 2 ifs rather than the if/else, I jumped to a
conclusion that we must be using the bnx2x init path on a bnx2 driver.
Apologies.
/me wanders off to see where completion_status gets set to
ISCSI_KCQE_COMPLETION_STATUS_ISCSI_NOT_SUPPORTED for bnx2.
Neil
> >
> > I'll leave commentary on initalization in is_cnic_dev to the authors :)
> >
> > Neil
> >
> >
> > >
> > > >
> > > > Ben.
> > > >
> > > > > This causes
> > > > > initalization of bnx2 to unilaterally fail in the cnic driver, which, while not
> > > > > catastrophic, is definately not expected. Fix this by choosing either the bnx2
> > > > > or bnx2x initalization path, not both
> > > > >
> > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > > CC: David S. Miller <davem@davemloft.net>
> > > > > CC: Michael Chan <mchan@broadcom.com>
> > > > > CC: Dmitry Kravkov <dmitry@broadcom.com>
> > > > > CC: Eddie Wai <waie@broadcom.com>
> > > > > CC: Eilon Greenstein <eilong@broadcom.com>
> > > > > ---
> > > > > drivers/net/cnic.c | 2 +-
> > > > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
> > > > > index 271a1f0..18b59ad 100644
> > > > > --- a/drivers/net/cnic.c
> > > > > +++ b/drivers/net/cnic.c
> > > > > @@ -5292,7 +5292,7 @@ static struct cnic_dev *is_cnic_dev(struct net_device *dev)
> > > > >
> > > > > if (!strcmp(drvinfo.driver, "bnx2"))
> > > > > cdev = init_bnx2_cnic(dev);
> > > > > - if (!strcmp(drvinfo.driver, "bnx2x"))
> > > > > + else if (!strcmp(drvinfo.driver, "bnx2x"))
> > > > > cdev = init_bnx2x_cnic(dev);
> > > > > if (cdev) {
> > > > > write_lock(&cnic_dev_lock);
> > > >
> > >
> > >
> > > --
> > > 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
> > >
> >
>
>
> --
> 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: [Patch V2] bonding: fix netpoll in active-backup mode
From: Neil Horman @ 2011-03-08 21:41 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: Amerigo Wang, linux-kernel, Jay Vosburgh, netdev
In-Reply-To: <20110308212405.GX11864@gospo.rdu.redhat.com>
On Tue, Mar 08, 2011 at 04:24:05PM -0500, Andy Gospodarek wrote:
> On Tue, Mar 08, 2011 at 05:58:56PM +0800, Amerigo Wang wrote:
> > V2: avoid calling slave_diable_netpoll() with write_lock_bh() held.
> >
> > netconsole doesn't work in active-backup mode, because we don't do anything
> > for nic failover in active-backup mode. We should disable netpoll on the
> > failing slave when it is detected down and enable netpoll when it becomes
> > the active slave.
> >
> > Tested by ifdown the current active slave and ifup it again for several times,
> > netconsole works well.
> >
> > Signed-off-by: WANG Cong <amwang@redhat.com>
> > Cc: Neil Horman <nhorman@tuxdriver.com>
> >
>
> It seems like you are going to a lot of trouble to fix a bug where
> netpoll will not be setup on any interface that is down when enslaved.
> That seems to be the only path that would not have slave->np setup
> properly at enslavement.
>
> Did you ever try just this?
>
+1, this is what I was suggesting. Just setup netpoll on all the slaves
regardless of status, and let their netpoll state follow that of the bond.
Neil
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 0592e6d..8d93044 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1352,8 +1352,6 @@ static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni)
>
> read_lock(&bond->lock);
> bond_for_each_slave(bond, slave, i) {
> - if (!IS_UP(slave->dev))
> - continue;
> err = slave_enable_netpoll(slave);
> if (err) {
> __bond_netpoll_cleanup(bond);
> --
> 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: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Nicolas de Pesloüan @ 2011-03-08 21:44 UTC (permalink / raw)
To: Andy Gospodarek
Cc: Jiri Pirko, netdev, davem, shemminger, kaber, fubar, eric.dumazet
In-Reply-To: <20110308134247.GW11864@gospo.rdu.redhat.com>
Le 08/03/2011 14:42, Andy Gospodarek a écrit :
> I'm pretty sure this patch will have the same catastrophic problem your
> last one did. By cloning and setting skb2->dev = orig_dev you just
> inserted a frame identical to the one we received right back into the
> stack. It only took a few minutes for my box to melt as one frame on
> the wire will cause an infinite number of frames to be received by the
> stack.
I agree with Andy. We still keep one reinject (netif_rx), which is probably better that two
(__netif_receive_skb), but not enough.
I really think we need a general framework for late delivery of final packets to packet handler
registered somewhere in the rx_handler path.
Jiri, is this patch the one you announced as "I have some kind nice solution in mind and I'm going
to submit that as a patch later (too many patches are in the wind atm)" ?
^ permalink raw reply
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards
From: Michael Chan @ 2011-03-08 21:41 UTC (permalink / raw)
To: Neil Horman
Cc: Ben Hutchings, netdev@vger.kernel.org, David S. Miller,
Dmitry Kravkov, Eddie Wai, Eilon Greenstein
In-Reply-To: <20110308214028.GD19669@hmsreliant.think-freely.org>
On Tue, 2011-03-08 at 13:40 -0800, Neil Horman wrote:
> > No, that should not happen. When we compare NULL-terminated "bnx2" with
> > NULL-terminated "bnx2x", it won't match because the '\0' won't match the
> > 'x', right?
> >
> > I think the patch is good. We can avoid the unnecessary strcmp() when
> > we have a match with "bnx2" already. But we should not get both matches
> > even without the patch.
> >
> Crap, sorry, I rescind this (although you're right, it probably wouldn't hurt to
> roll this change in, just to save a potential second strcmp). Anywho, I'm
> getting this error on the ifup of my bnx2 interface:
> bnx2i: iSCSI not supported, dev=eth1
>
> That message appears to only get set (from my read) on cards driven by the bnx2x
> driver, and when I saw the 2 ifs rather than the if/else, I jumped to a
> conclusion that we must be using the bnx2x init path on a bnx2 driver.
> Apologies.
>
> /me wanders off to see where completion_status gets set to
> ISCSI_KCQE_COMPLETION_STATUS_ISCSI_NOT_SUPPORTED for bnx2.
On bnx2, this error code is set by the firmware if iSCSI is not
configured in NVRAM.
Thanks.
^ permalink raw reply
* Re: Network performance with small packets
From: Shirley Ma @ 2011-03-08 21:57 UTC (permalink / raw)
To: Rusty Russell
Cc: Michael S. Tsirkin, Krishna Kumar2, David Miller, kvm, netdev,
steved
In-Reply-To: <201102091107.20270.rusty@rustcorp.com.au>
On Wed, 2011-02-09 at 11:07 +1030, Rusty Russell wrote:
> I've finally read this thread... I think we need to get more serious
> with our stats gathering to diagnose these kind of performance issues.
>
> This is a start; it should tell us what is actually happening to the
> virtio ring(s) without significant performance impact...
Should we also add similar stat on vhost vq as well for monitoring
vhost_signal & vhost_notify?
Shirley
^ permalink raw reply
* kbench_mod
From: David Miller @ 2011-03-08 22:10 UTC (permalink / raw)
To: netdev
I just added a special kernel module to the net_test_tools tree,
it allows more accurate measurement of routing lookups.
It does cycle counts across one routing lookup.
The flow key is controlled by module parameters, iif != 0 means
input route lookup, otherwise we do an output lookup.
What's cool is you can jack-up the warmup_count value and then
run insmod under "perf record" and it just works.
^ permalink raw reply
* Re: [PATCH 1/2 net-2.6] r6040: fix multicast operations
From: David Miller @ 2011-03-08 22:22 UTC (permalink / raw)
To: florian; +Cc: netdev
In-Reply-To: <201103071109.40853.florian@openwrt.org>
From: Florian Fainelli <florian@openwrt.org>
Date: Mon, 7 Mar 2011 11:09:40 +0100
> From: Shawn Lin <shawn@dmp.com.tw>
>
> The original code does not work well when the number of mulitcast
> address to handle is greater than MCAST_MAX. It only enable promiscous
> mode instead of multicast hash table mode, so the hash table function
> will not be activated and all multicast frames will be recieved in this
> condition.
>
> This patch fixes the following issues with the r6040 NIC operating in
> multicast:
>
> 1) When the IFF_ALLMULTI flag is set, we should write 0xffff to the NIC
> hash table registers to make it process multicast traffic.
>
> 2) When the number of multicast address to handle is smaller than
> MCAST_MAX, we should use the NIC multicast registers MID1_{L,M,H}.
>
> 3) The hashing of the address was not correct, due to an invalid
> substraction (15 - (crc & 0x0f)) instead of (crc & 0x0f) and an
> incorrect crc algorithm (ether_crc_le) instead of (ether_crc).
>
> 4) If necessary, we should set HASH_EN flag in MCR0 to enable multicast
> hash table function.
>
> Reported-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
> Tested-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
> Signed-off-by: Shawn Lin <shawn@dmp.com.tw>
> Signed-off-by: Albert Chen <albert.chen@rdc.com.tw>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> Signed-off-by: David S. Miller <davem@davemloft.net>
Applied.
^ permalink raw reply
* Re: [PATCH 2/2 net-2.6] r6040: bump to version 0.27 and date 23Feb2011
From: David Miller @ 2011-03-08 22:22 UTC (permalink / raw)
To: florian; +Cc: netdev
In-Reply-To: <201103071109.42377.florian@openwrt.org>
From: Florian Fainelli <florian@openwrt.org>
Date: Mon, 7 Mar 2011 11:09:42 +0100
> From: Florian Fainelli <florian@openwrt.org>
>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> Signed-off-by: David S. Miller <davem@davemloft.net>
Applied.
^ permalink raw reply
* [PATCH net-next-2.6] ixgbe: fix compile failure in ixgbe_init_mbx_params_pf
From: Andy Gospodarek @ 2011-03-08 22:24 UTC (permalink / raw)
To: netdev, davem, Emil Tantilov, Jeff Kirsher
This commit:
commit d7c8a29fc8bd20ba45ec2f52577ed04a988a9500
Author: Emil Tantilov <emil.s.tantilov@intel.com>
Date: Thu Mar 3 09:25:02 2011 +0000
ixgbe: improve logic in ixgbe_init_mbx_params_pf
incorrectly added a line that accessed mbx->udelay. I'm sure the intent
was mbx->usec_delay. This patch fixes the compilation error.
Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
---
drivers/net/ixgbe/ixgbe_mbx.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_mbx.c b/drivers/net/ixgbe/ixgbe_mbx.c
index 3cf8aec..c7ed82e 100644
--- a/drivers/net/ixgbe/ixgbe_mbx.c
+++ b/drivers/net/ixgbe/ixgbe_mbx.c
@@ -453,7 +453,7 @@ void ixgbe_init_mbx_params_pf(struct ixgbe_hw *hw)
return;
mbx->timeout = 0;
- mbx->udelay = 0;
+ mbx->usec_delay = 0;
mbx->stats.msgs_tx = 0;
mbx->stats.msgs_rx = 0;
^ permalink raw reply related
* Re: [PATCH] dsa/mv88e6060: support nonzero mii base address
From: David Miller @ 2011-03-08 22:24 UTC (permalink / raw)
To: buytenh; +Cc: jacmet, netdev
In-Reply-To: <20110307182413.GZ16649@mail.wantstofly.org>
From: Lennert Buytenhek <buytenh@wantstofly.org>
Date: Mon, 7 Mar 2011 19:24:13 +0100
> On Mon, Mar 07, 2011 at 04:49:47PM +0100, Peter Korsgaard wrote:
>
>> The mv88e6060 uses either the lower 16 or upper 16 mii addresses,
>> depending on the value of the EE_CLK/ADDR4 pin. Support both
>> configurations by using the sw_addr setting as base address.
>>
>> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
>
> Looks good!
>
> Acked-by: Lennert Buytenhek <buytenh@wantstofly.org>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ixgbe: fix compile failure in ixgbe_init_mbx_params_pf
From: David Miller @ 2011-03-08 22:26 UTC (permalink / raw)
To: andy; +Cc: netdev, davem, emil.s.tantilov, jeffrey.t.kirsher
In-Reply-To: <1299623047-25656-1-git-send-email-andy@greyhouse.net>
From: Andy Gospodarek <andy@greyhouse.net>
Date: Tue, 8 Mar 2011 17:24:07 -0500
> This commit:
>
> commit d7c8a29fc8bd20ba45ec2f52577ed04a988a9500
> Author: Emil Tantilov <emil.s.tantilov@intel.com>
> Date: Thu Mar 3 09:25:02 2011 +0000
>
> ixgbe: improve logic in ixgbe_init_mbx_params_pf
>
> incorrectly added a line that accessed mbx->udelay. I'm sure the intent
> was mbx->usec_delay. This patch fixes the compilation error.
>
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
Applied, thanks Andy.
^ permalink raw reply
* Re: [PATCH 11/17] mlx4_en: Enabling new steering
From: David Miller @ 2011-03-08 22:26 UTC (permalink / raw)
To: roland; +Cc: or.gerlitz, yevgenyp, netdev, vlad, nirmu, alekseys
In-Reply-To: <AANLkTim8XDT5YKJc4wroyEDJc1X+xZOFL9y_090F5eLr@mail.gmail.com>
From: Roland Dreier <roland@kernel.org>
Date: Tue, 8 Mar 2011 13:30:25 -0800
> On Tue, Mar 8, 2011 at 1:10 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>>> + if (mdev->dev->caps.vep_mc_steering && ibqp->qp_type == IB_QPT_RAW_ETHERTYPE) {
>>
>> Guys,
>>
>> Didn't you want to refer here to the "raw ethernet QP" which is typed
>> to IB_QPT_RAW_PACKET by the series posted on December last year, see
>> http://marc.info/?l=linux-rdma&m=129162076818154 --
>> IB_QPT_RAW_ETHERTYPE is a rename of IB's IB_QPT_RAW_ETY done by
>> commit a2ebf07ae53e65bd073f96877e4818f2e89271ae
>
> Good catch. I think the drivers/infiniband part of this patch should
> just be dropped -- we don't have any raw QP support of any kind, right?
Once this is all sorted out, please someone respin this entire patch
set.
Thanks.
^ permalink raw reply
* Re: [PATCH] vxge: update MAINTAINERS
From: David Miller @ 2011-03-08 22:28 UTC (permalink / raw)
To: jdmason; +Cc: netdev
In-Reply-To: <1299517321-4567-1-git-send-email-jdmason@kudzu.us>
From: Jon Mason <jdmason@kudzu.us>
Date: Mon, 7 Mar 2011 11:02:01 -0600
> Exar has exited the 10G Ethernet NIC market, orphaning both the s2io and
> vxge drivers. With the promise of free hardware, I'll take over
> maintainership.
>
> Signed-off-by: Jon Mason <jdmason@kudzu.us>
Applied, thanks.
^ permalink raw reply
* [PATCH] inetpeer: Don't disable BH for initial fast RCU lookup.
From: David Miller @ 2011-03-08 22:59 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet
If modifications on other cpus are ok, then modifications to
the tree during lookup done by the local cpu are ok too.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/ipv4/inetpeer.c | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index f604ffd..6442c35 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -206,16 +206,16 @@ static int addr_compare(const struct inetpeer_addr *a,
})
/*
- * Called with rcu_read_lock_bh()
+ * Called with rcu_read_lock()
* Because we hold no lock against a writer, its quite possible we fall
* in an endless loop.
* But every pointer we follow is guaranteed to be valid thanks to RCU.
* We exit from this function if number of links exceeds PEER_MAXDEPTH
*/
-static struct inet_peer *lookup_rcu_bh(const struct inetpeer_addr *daddr,
- struct inet_peer_base *base)
+static struct inet_peer *lookup_rcu(const struct inetpeer_addr *daddr,
+ struct inet_peer_base *base)
{
- struct inet_peer *u = rcu_dereference_bh(base->root);
+ struct inet_peer *u = rcu_dereference(base->root);
int count = 0;
while (u != peer_avl_empty) {
@@ -231,9 +231,9 @@ static struct inet_peer *lookup_rcu_bh(const struct inetpeer_addr *daddr,
return u;
}
if (cmp == -1)
- u = rcu_dereference_bh(u->avl_left);
+ u = rcu_dereference(u->avl_left);
else
- u = rcu_dereference_bh(u->avl_right);
+ u = rcu_dereference(u->avl_right);
if (unlikely(++count == PEER_MAXDEPTH))
break;
}
@@ -470,11 +470,11 @@ struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create)
/* Look up for the address quickly, lockless.
* Because of a concurrent writer, we might not find an existing entry.
*/
- rcu_read_lock_bh();
+ rcu_read_lock();
sequence = read_seqbegin(&base->lock);
- p = lookup_rcu_bh(daddr, base);
+ p = lookup_rcu(daddr, base);
invalidated = read_seqretry(&base->lock, sequence);
- rcu_read_unlock_bh();
+ rcu_read_unlock();
if (p) {
/* The existing node has been found.
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] Make CUBIC Hystart more robust to RTT variations
From: Stephen Hemminger @ 2011-03-08 23:21 UTC (permalink / raw)
To: David Miller; +Cc: rhee, lucas.nussbaum, xiyou.wangcong, netdev
In-Reply-To: <20110308.114346.48506864.davem@davemloft.net>
On Tue, 08 Mar 2011 11:43:46 -0800 (PST)
David Miller <davem@davemloft.net> wrote:
> From: Injong Rhee <rhee@ncsu.edu>
> Date: Tue, 08 Mar 2011 10:26:36 -0500
>
> > Thanks for updating CUBIC hystart. You might want to test the
> > cases with more background traffic and verify whether this
> > threshold is too conservative.
>
> So let's get down to basics.
>
> What does Hystart do specially that allows it to avoid all of the
> problems that TCP VEGAS runs into.
>
> Specifically, that if you use RTTs to make congestion control
> decisions it is impossible to notice new bandwidth becomming available
> fast enough.
>
> Again, it's impossible to react fast enough. No matter what you tweak
> all of your various settings to, this problem will still exist.
>
> This is a core issue, you cannot get around it.
>
> This is why I feel that Hystart is fundamentally flawed and we should
> turn it off by default if not flat-out remove it.
>
> Distributions are turning it off by default already, therefore it's
> stupid for the upstream kernel to behave differently if that's what
> %99 of the world is going to end up experiencing.
The assumption in Hystart that spacing between ACK's is solely due to
congestion is a bad. If you read the paper, this is why FreeBSD's
estimation logic is dismissed. The Hystart problem is different
than the Vegas issue.
Algorithms that look at min RTT are ok, since the lower bound is
fixed; additional queuing and variation in network only increases RTT
it never reduces it. With a min RTT it is possible to compute the
upper bound on available bandwidth. i.e If all packets were as good as
this estimate minRTT then the available bandwidth is X. But then using
an individual RTT sample to estimate unused bandwidth is flawed. To
quote paper.
"Thus, by checking whether ∆(N ) is larger than Dmin , we
can detect whether cwnd has reached the available capacity
of the path"
So what goes wrong:
1. Dmin can be too large because this connection always sees delays
due to other traffic or hardware. i.e buffer bloat. This would cause
the bandwidth estimate to be too low and therefore TCP would leave
slow start too early (and not get up to full bandwidth).
2. Dmin can be smaller than the clock resolution. This would cause
either sample to be ignored, or Dmin to be zero. If Dmin is zero,
the bandwidth estimate would in theory be infinite, which would
lead to TCP not leaving slow start because of Hystart. Instead
TCP would leave slow start at first loss.
Other possible problems:
3. ACK's could be nudged together by variations in delay.
This would cause HyStart to exit slow start prematurely. To false
think it is an ACK train.
Noise in network is not catastrophic, it just
causes TCP to exit slow-start early and have to go into normal
window growth phase. The problem is that the original non-Hystart
behavior of Cubic is unfair; the first flow dominates the link
and other flows are unable to get in. If you run tests with two
flows one will get a larger share of the bandwidth.
I think Hystart is okay in concept but there may be issues
on low RTT links as well as other corner cases that need bug
fixing.
1. Needs to use better resolution than HZ. Since HZ can be 100.
2. Hardcoding 2ms as spacing between ACK's as train is wrong
for local networks.
^ permalink raw reply
* Re: [rds-devel] [PATCH] rds: prevent BUG_ON triggering on congestion map updates
From: Venkat Venkatsubra @ 2011-03-08 23:36 UTC (permalink / raw)
To: nhorman; +Cc: netdev, rds-devel, chris.mason, davem
I was trying to recreate on our test systems. It's not a ppc.
So far not successful in crashing even though rds_ib_xmit hits this path:
/* Do not send cong updates to IB loopback */
if (conn->c_loopback
&& rm->m_inc.i_hdr.h_flags & RDS_FLAG_CONG_BITMAP) {
rds_cong_map_updated(conn->c_fcong, ~(u64) 0);
return sizeof(struct rds_header) + RDS_CONG_MAP_BYTES;
}
For the BUG_ON condition in rds_send_xmit:
ret=4096 conn->c_xmit_sg=1 rm->data.op_nents=2
BUG_ON(ret != 0 && conn->c_xmit_sg == rm->data.op.nents);
Venkat
----- Original Message -----
From: nhorman@tuxdriver.com
To: davem@davemloft.net
Cc: chris.mason@oracle.com, netdev@vger.kernel.org, rds-devel@oss.oracle.com, venkat.x.venkatsubra@oracle.com
Sent: Tuesday, March 8, 2011 1:36:50 PM GMT -06:00 US/Canada Central
Subject: Re: [rds-devel] [PATCH] rds: prevent BUG_ON triggering on congestion map updates
On Tue, Mar 08, 2011 at 11:22:58AM -0800, David Miller wrote:
> From: Chris Mason <chris.mason@oracle.com>
> Date: Tue, 08 Mar 2011 13:58:57 -0500
>
> > Excerpts from David Miller's message of 2011-03-08 13:52:23 -0500:
> >> From: Chris Mason <chris.mason@oracle.com>
> >> Date: Mon, 07 Mar 2011 15:41:04 -0500
> >>
> >> > Excerpts from David Miller's message of 2011-03-07 15:27:53 -0500:
> >> >> From: Neil Horman <nhorman@tuxdriver.com>
> >> >> Date: Wed, 2 Mar 2011 11:28:22 -0500
> >> >>
> >> >> > Recently had this bug halt reported to me:
> >> >>
> >> >> Well, does anyone on the RDS team care about this bug fix at all?
> >> >>
> >> >> Stuff like this should not sit for nearly a week without any reply
> >> >> whatsoever.
> >> >>
> >> >
> >> > The patch looks good to me, but I'm surprised we haven't seen it here.
> >> > Venkat please take a look (link to the patch below) Has it only been seen on ppc?
> >> >
> >> > http://permalink.gmane.org/gmane.linux.network/187933
> >>
> >> Can I get an ACK or something, _please_?
> >
> > Sorry, wasn't clear:
> >
> > Acked-by: Chris Mason <chris.mason@oracle.com>
>
> Applied, thanks everyone.
Thanks, Chris, Dave.
Neil
> --
> 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: linux-next: manual merge of the net tree with the net-current tree
From: Stephen Rothwell @ 2011-03-09 0:11 UTC (permalink / raw)
To: Dmitry Kravkov
Cc: David Miller, netdev@vger.kernel.org, linux-next@vger.kernel.org,
linux-kernel@vger.kernel.org, Eilon Greenstein,
Vladislav Zolotarov
In-Reply-To: <1299577473.17233.41.camel@lb-tlvb-dmitry>
[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]
Hi Dmitry,
On Tue, 8 Mar 2011 11:44:33 +0200 "Dmitry Kravkov" <dmitry@broadcom.com> wrote:
>
> You are correct it's just an addition and it does not really matter
> where to do this. But the merge pushed new code in the middle
> of MAC configuration between MAC and multicast list.
> It's clearer to put it at the end of MAC/ML/UL block:
> ---
> drivers/net/bnx2x/bnx2x_cmn.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> index b01b622..9e37675 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> @@ -1498,6 +1498,11 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)
> /* Clear UC lists configuration */
> bnx2x_invalidate_uc_list(bp);
>
> + if (bp->pending_max) {
> + bnx2x_update_max_mf_config(bp, bp->pending_max);
> + bp->pending_max = 0;
> + }
> +
> if (bp->port.pmf)
> bnx2x_initial_phy_init(bp, load_mode);
OK, thanks. I have changed the conflict resolution to do that.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH] ipv4: Cache source address in nexthop entries.
From: Julian Anastasov @ 2011-03-09 0:49 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110308.103801.193702247.davem@davemloft.net>
Hello,
On Tue, 8 Mar 2011, David Miller wrote:
>> As the addresses have nothing to do with the link
>> state, I don't think it is correct to call fib_update_nh_saddrs
>> for DEV events.
>
> fib_update_nh_saddrs() gets called for "DEV" events since those
> are what are emitted when addresses are addded and removed.
But we still need to propagate the address events
to nexthops on all devices. Even if it is slow, I see it in
this way (not tested):
static inline void update_nh_saddr(const struct in_ifaddr *ifa,
struct net_device *dev,
struct fib_nh *nexthop_nh,
unsigned long event)
{
if (event == NETDEV_UP) {
if (ifa->ifa_local == nexthop_nh->nh_saddr)
return;
} else {
if (ifa->ifa_local != nexthop_nh->nh_saddr)
return;
}
if (ifa->ifa_scope > nexthop_nh->nh_cfg_scope)
return;
if (dev != nexthop_nh->nh_dev && ifa->ifa_scope >= RT_SCOPE_LINK)
return;
if (nexthop_nh->nh_flags & RTNH_F_DEAD)
return;
nexthop_nh->nh_saddr = inet_select_addr(nexthop_nh->nh_dev,
nexthop_nh->nh_gw,
nexthop_nh->nh_cfg_scope);
}
void fib_update_nh_saddrs(const struct in_ifaddr *ifa, unsigned long event)
{
struct net_device *dev = ifa->ifa_dev->dev;
int i;
for (i = 0; i < fib_hash_size; i++) {
struct hlist_head *head = &fib_info_hash[i];
struct hlist_node *node;
struct fib_info *fi;
hlist_for_each_entry(fi, node, head, fib_hash) {
if (fi->fib_prefsrc)
continue;
change_nexthops(fi) {
if (!nexthop_nh->nh_dev)
continue;
update_nh_saddr(ifa, dev, nexthop_nh, event);
} endfor_nexthops(fi);
}
}
}
Then may be fib_sync_up() should also refresh nh_saddr,
so that all alive nexthops get actual source address which
was not possible while device was down.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH] Make CUBIC Hystart more robust to RTT variations
From: Injong Rhee @ 2011-03-09 1:30 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David Miller, lucas.nussbaum, xiyou.wangcong, netdev, sangtae.ha
In-Reply-To: <20110308152103.714f5f05@nehalam>
HyStart is a slow start algorithm, but not a congestion control
algorithm. So the difference between vegas and hystart is obvious. Yes.
Both hystart and vegas use delays for indication of congestion. But
hystart exits slow starts at the detection of congestion and enters
normal congestion avoidance; in some sense, it is much safer than vegas
as it does not change the regular behaviors of congestion control.
I think the main problem arising right now is not because it is using
noisy delays as congestion indication, but because of rather some
implementation issues like use of Hz, hardcoding 2ms, etc.
Then, you might ask why hystart can use delays while vegas can't. The
main motivation for use delays during slow start is that slow start
creates an environment where delay samples can be more trusted. That is
because it sends so many packets as a a burst because of doubling
windows, which can be used as packet train to estimate the available
capacity more reliably.
(tool 1) When many packets are sent in burst, the spacing in returning
ACKs can be a good indicator. Hystart also uses delays as an estimation.
(tool 2) If estimated avg delays increase beyond a certain threshold, it
sees that as a possible congestion.
Now, both tools can be wrong. But that is not catastrophic since
congestion avoidance can kick in to save the day. In a pipe where no
other flows are competing, then exiting slow start too early can slow
things down as the window can be still too small. But that is in fact
when delays are most reliable. So those tests that say bad performance
with hystart are in fact, where hystart is supposed to perform well.
Then why do we have a bad performance? I think the answer is again the
implementation flaws -- use different hardware, some hardwired codes,
etc, and also could be related to a few corner cases like very low RTT
links.
Let us examine Stephen's analysis in more detail.
1. Use of minRTT is ok. I agree.
2. Dmin can be too large at the beginning. But it is just like minRTT.
This cannot be too large. If you trust minRTT, then delay estimation
should say that there is a congestion. This is exactly the opposite case
to the cases we are seeing. If Dmin is too large, then hystart would not
exit the slow start as it does not detect the congestion. That is not
what we are seeing right now.
3. Dmin can be smaller than clock resolution. That is why we are using a
bunch of ACKs to get better accuracy. With a bunch of ACKs, we get
higher value of spacing so that we can take average.
4. If ACKs are nudged together, then hystart does not quit slow start.
Instead, it sees that there is no congestion. It is when it sees big
spacing between ACKs -- that is when it detects congestion.
On 3/8/11 6:21 PM, Stephen Hemminger wrote:
> On Tue, 08 Mar 2011 11:43:46 -0800 (PST)
> David Miller<davem@davemloft.net> wrote:
>
>> From: Injong Rhee<rhee@ncsu.edu>
>> Date: Tue, 08 Mar 2011 10:26:36 -0500
>>
>>> Thanks for updating CUBIC hystart. You might want to test the
>>> cases with more background traffic and verify whether this
>>> threshold is too conservative.
>> So let's get down to basics.
>>
>> What does Hystart do specially that allows it to avoid all of the
>> problems that TCP VEGAS runs into.
>>
>> Specifically, that if you use RTTs to make congestion control
>> decisions it is impossible to notice new bandwidth becomming available
>> fast enough.
>>
>> Again, it's impossible to react fast enough. No matter what you tweak
>> all of your various settings to, this problem will still exist.
>>
>> This is a core issue, you cannot get around it.
>>
>> This is why I feel that Hystart is fundamentally flawed and we should
>> turn it off by default if not flat-out remove it.
>>
>> Distributions are turning it off by default already, therefore it's
>> stupid for the upstream kernel to behave differently if that's what
>> %99 of the world is going to end up experiencing.
> The assumption in Hystart that spacing between ACK's is solely due to
> congestion is a bad. If you read the paper, this is why FreeBSD's
> estimation logic is dismissed. The Hystart problem is different
> than the Vegas issue.
>
> Algorithms that look at min RTT are ok, since the lower bound is
> fixed; additional queuing and variation in network only increases RTT
> it never reduces it. With a min RTT it is possible to compute the
> upper bound on available bandwidth. i.e If all packets were as good as
> this estimate minRTT then the available bandwidth is X. But then using
> an individual RTT sample to estimate unused bandwidth is flawed. To
> quote paper.
>
> "Thus, by checking whether ∆(N ) is larger than Dmin , we
> can detect whether cwnd has reached the available capacity
> of the path"
>
> So what goes wrong:
> 1. Dmin can be too large because this connection always sees delays
> due to other traffic or hardware. i.e buffer bloat. This would cause
> the bandwidth estimate to be too low and therefore TCP would leave
> slow start too early (and not get up to full bandwidth).
>
> 2. Dmin can be smaller than the clock resolution. This would cause
> either sample to be ignored, or Dmin to be zero. If Dmin is zero,
> the bandwidth estimate would in theory be infinite, which would
> lead to TCP not leaving slow start because of Hystart. Instead
> TCP would leave slow start at first loss.
>
> Other possible problems:
> 3. ACK's could be nudged together by variations in delay.
> This would cause HyStart to exit slow start prematurely. To false
> think it is an ACK train.
>
> Noise in network is not catastrophic, it just
> causes TCP to exit slow-start early and have to go into normal
> window growth phase. The problem is that the original non-Hystart
> behavior of Cubic is unfair; the first flow dominates the link
> and other flows are unable to get in. If you run tests with two
> flows one will get a larger share of the bandwidth.
>
> I think Hystart is okay in concept but there may be issues
> on low RTT links as well as other corner cases that need bug
> fixing.
>
> 1. Needs to use better resolution than HZ. Since HZ can be 100.
> 2. Hardcoding 2ms as spacing between ACK's as train is wrong
> for local networks.
>
>
>
>
>
^ 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