* 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 11/17] mlx4_en: Enabling new steering
From: Or Gerlitz @ 2011-03-08 21:10 UTC (permalink / raw)
To: Yevgeny Petrilin; +Cc: davem, netdev, vlad, Roland Dreier, nirmu, Aleksey Senin
In-Reply-To: <4D74B0C9.9020108@mellanox.co.il>
Yevgeny Petrilin <yevgenyp@mellanox.co.il> wrote:
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -623,6 +623,14 @@ static int mlx4_ib_mcg_attach(struct ib_qp *ibqp, union
[...]
> + 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
Or.
^ permalink raw reply
* Re: [net-next-2.6 00/14][pull request] Intel Wired LAN Driver Updates
From: Jeff Kirsher @ 2011-03-08 21:10 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, gospo@redhat.com, bphilips@novell.com
In-Reply-To: <20110308.111137.112604487.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 855 bytes --]
On Tue, 2011-03-08 at 11:11 -0800, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Mon, 7 Mar 2011 18:24:54 -0800
>
> > The following series contains fixes and cleanups for e1000e and ixgbe, as
> > well as support for FCoE DDP in target mode for ixgbe. In igb, added
> > additional stats for OS2BMC feature.
> >
> > The following are changes since commit 541ac7c9b30ee2ff84ad87f27e0bc069e143afb5:
> > bonding: COW before overwriting the destination MAC address
> >
> > and are available in the git repository at:
> > master.kernel.org:/pub/scm/linux/kernel/git/jkirsher/net-next-2.6 master
>
> Pulled.
>
> Please address the feedback given on the lack of docbook descriptions for
> all of those fcoe device ops.
Thanks Dave.
I will work with Yi to get FCoE documentation patches together.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH 1/8] Phonet: fix NULL-deref in a8059512b120362b15424f152b2548fe8b11bd0c
From: David Miller @ 2011-03-08 21:09 UTC (permalink / raw)
To: remi; +Cc: netdev
In-Reply-To: <201103082259.41844.remi@remlab.net>
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.
^ permalink raw reply
* Re: [PATCH 1/8] Phonet: fix NULL-deref in a8059512b120362b15424f152b2548fe8b11bd0c
From: Rémi Denis-Courmont @ 2011-03-08 20:59 UTC (permalink / raw)
To: netdev
In-Reply-To: <20110308.112923.39177383.davem@davemloft.net>
Hello,
On March 8th 2011 21:29:23 David Miller, you wrote :
> You don't even read what I tell you, which makes reviewing your work
> insanely frustrating.
Fair enough.
You were the one complaining about the Nokia mail server being insanely
broken (in private email by the way). So I have to read stuff at home, and
implement it the next day at work. Informations do get lost at night. I don't
need to mention I don't like this is frustrating for us Nokians as well.
(...)
> It also irks me that you persisted to provide the most terse possible
> one-line commit message. Have a conversation in your commit message,
> this isn't for you it's for other people trying to understand your
> work.
I'm trying to fix the terrible mess that ST-Ericsson left of the Phonet stack
here. This wouldn't be an issue if the crap hadn't been merged, or if I had
been given enough time to voice my concerns.
Just one WTF? out of many:
static u8 data[4] = {
0x03, 0x04,
};
data[2] = pn->tx_fc;
data[3] = pn->rx_fc;
(Those coffee-deprivated, note the 'static' keyword.) I wonder if the guy ever
wrote C (not C++) before he came to kernel.org and left as quickly.
I am totally fine with strictness on your part. I do genuinely believe it
improves code quality and such, and do try to apply similar patterns when I am
the reviewer. So I present my apologizes for my series of unintended
violations of The Process.
I just wish you'd been as strict back then. Nobody would be having this
pointless non-technical discussion... Well... I'll fix it tomorrow because I
don't want that kind of crap in the Linux kernel anyway.
Thanks every one you for your cooperation.
--
Rémi
^ permalink raw reply
* Re: [stable] [stable-2.6.32.y] ixgbe: fix for 82599 erratum on Header Splitting
From: Greg KH @ 2011-03-08 19:48 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: gregkh, Don Skidmore, stable, netdev
In-Reply-To: <1299366890-10333-1-git-send-email-jeffrey.t.kirsher@intel.com>
On Sat, Mar 05, 2011 at 03:14:50PM -0800, Jeff Kirsher wrote:
> From: Don Skidmore <donald.c.skidmore@intel.com>
>
> We have found a hardware erratum on 82599 hardware that can lead to
> unpredictable behavior when Header Splitting mode is enabled. So
> we are no longer enabling this feature on affected hardware.
>
> This is an updated version of the following commit which applies to the stable
> linux-2.6.32.y tree:
Applied, thanks.
greg k-h
^ permalink raw reply
* [PATCH] can: wrong index used in inner loop
From: roel @ 2011-03-08 19:52 UTC (permalink / raw)
To: Wolfgang Grandegger, socketcan-core, netdev, Andrew Morton, LKML
Index i was already used in the outer loop.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
drivers/net/can/usb/esd_usb2.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
index 05a5275..dc53c83 100644
--- a/drivers/net/can/usb/esd_usb2.c
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -659,7 +659,7 @@ failed:
static void unlink_all_urbs(struct esd_usb2 *dev)
{
struct esd_usb2_net_priv *priv;
- int i;
+ int i, j;
usb_kill_anchored_urbs(&dev->rx_submitted);
for (i = 0; i < dev->net_count; i++) {
@@ -668,8 +668,8 @@ static void unlink_all_urbs(struct esd_usb2 *dev)
usb_kill_anchored_urbs(&priv->tx_submitted);
atomic_set(&priv->active_tx_jobs, 0);
- for (i = 0; i < MAX_TX_URBS; i++)
- priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+ for (j = 0; j < MAX_TX_URBS; j++)
+ priv->tx_contexts[j].echo_index = MAX_TX_URBS;
}
}
}
^ permalink raw reply related
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards
From: Michael Chan @ 2011-03-08 19:43 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: <20110308193601.GA19669@hmsreliant.think-freely.org>
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.
>
> 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
> >
>
^ permalink raw reply
* Re: [PATCH] Make CUBIC Hystart more robust to RTT variations
From: David Miller @ 2011-03-08 19:43 UTC (permalink / raw)
To: rhee; +Cc: lucas.nussbaum, xiyou.wangcong, netdev
In-Reply-To: <4D764AAC.30302@ncsu.edu>
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.
^ permalink raw reply
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards
From: Ben Hutchings @ 2011-03-08 19:40 UTC (permalink / raw)
To: Neil Horman
Cc: Michael Chan, netdev@vger.kernel.org, David S. Miller,
Dmitry Kravkov, Eddie Wai, Eilon Greenstein
In-Reply-To: <20110308193601.GA19669@hmsreliant.think-freely.org>
On Tue, 2011-03-08 at 14:36 -0500, 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.
[...]
If you don't even know what strcmp() does, what are you doing hacking on
the kernel?
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] tcp_cubic: enable TCP timestamps
From: David Miller @ 2011-03-08 19:38 UTC (permalink / raw)
To: lucas.nussbaum; +Cc: sangtae.ha, shemminger, netdev
In-Reply-To: <20110308193602.GA14526@xanadu.blop.info>
From: Lucas Nussbaum <lucas.nussbaum@loria.fr>
Date: Tue, 8 Mar 2011 20:36:02 +0100
> My other patch mitigates the performance problem by making it harder for
> Hystart to abort slow start. But after a few days working on this issue,
> I'm wondering whether Hystart shouldn't be completely rewritten to work
> on the usec values, or disabled by default.
I am of the opinion that Hystart, like TCP VEGAS, fundamentally cannot
work.
It's disabled in several major distributions because of the performance
problems you noticed, and this was on my own personal recommendation.
^ permalink raw reply
* Re: [rds-devel] [PATCH] rds: prevent BUG_ON triggering on congestion map updates
From: Neil Horman @ 2011-03-08 19:36 UTC (permalink / raw)
To: David Miller; +Cc: chris.mason, netdev, rds-devel, venkat.x.venkatsubra
In-Reply-To: <20110308.112258.183048712.davem@davemloft.net>
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: [PATCH] cnic: fix double initalization of bnx2 based cards
From: Neil Horman @ 2011-03-08 19:36 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: <1299611277.12601.15.camel@HP1>
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.
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
>
^ permalink raw reply
* Re: [PATCH] tcp_cubic: enable TCP timestamps
From: Lucas Nussbaum @ 2011-03-08 19:36 UTC (permalink / raw)
To: Sangtae Ha; +Cc: David Miller, shemminger, netdev
In-Reply-To: <AANLkTinT0eHbbELWDLxYyj0axpYt=GHnxLP8fMH-pWOs@mail.gmail.com>
On 08/03/11 at 14:15 -0500, Sangtae Ha wrote:
> Yes. I remember that CONFIG_HZ was 1000 at that time and the value of
> CONFIG_HZ could affect the algorithm.
> I don't think HyStart needs this extra RTT_STAMP since we only need a
> rough delay estimate.
> Let me check HyStart with the latest git and with different CONFIG_HZ values.
One of the problems I discovered was that ca->delay_min was completely
off if RTT_STAMP is disabled. For example, on a link with a 11ms RTT, I
could get delay_min = 4ms. But even with RTT_STAMP, there's the problem
that the code rounds the computed RTT value to a jiffie (in
bictcp_acked()).
My other patch mitigates the performance problem by making it harder for
Hystart to abort slow start. But after a few days working on this issue,
I'm wondering whether Hystart shouldn't be completely rewritten to work
on the usec values, or disabled by default.
--
| Lucas Nussbaum MCF Université Nancy 2 |
| lucas.nussbaum@loria.fr LORIA / AlGorille |
| http://www.loria.fr/~lnussbau/ +33 3 54 95 86 19 |
^ permalink raw reply
* Re: pull request: sfc-next-2.6 2011-03-07
From: David Miller @ 2011-03-08 19:33 UTC (permalink / raw)
To: bhutchings; +Cc: linux-net-drivers, netdev
In-Reply-To: <1299540194.2522.46.camel@bwh-desktop>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 07 Mar 2011 23:23:14 +0000
> The following changes since commit 07df5294a753dfac2cc9f75e6159fc25fdc22149:
>
> inet: Replace left-over references to inet->cork (2011-03-01 23:00:58 -0800)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next-2.6.git for-davem
>
> Just one more performance optimisation.
Pulled, thanks Ben.
^ permalink raw reply
* Re: [PATCH 1/8] Phonet: fix NULL-deref in a8059512b120362b15424f152b2548fe8b11bd0c
From: David Miller @ 2011-03-08 19:29 UTC (permalink / raw)
To: remi.denis-courmont; +Cc: netdev
In-Reply-To: <1299592626-18510-1-git-send-email-remi.denis-courmont@nokia.com>
From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Date: Tue, 8 Mar 2011 15:56:59 +0200
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
You don't even read what I tell you, which makes reviewing your work
insanely frustrating.
I said:
Reference other changes like a man, by mentioned the SHA1 ID and
giving the commit message header line inside of ("") right
afterwards. :-)
I didn't say to put the SHA1 reference in the subject line, it's
so messy there. Put it in the commit message. Do you see other
people putting commit ID references in the commit header line?
Also, I said explicitly to reference the commit like this:
$(SHA1_ID) ("commit message header line")
so in this case it would be:
a8059512b120362b15424f152b2548fe8b11bd0c ("Phonet: implement
per-socket destination/peer address")
because when commits get backported that SHA1_ID might be different
in the tree it gets backported to, so you have to provide that
textual reference to the commit so that in such a case it can still
be matched up.
It also irks me that you persisted to provide the most terse possible
one-line commit message. Have a conversation in your commit message,
this isn't for you it's for other people trying to understand your
work.
The commit message header line just gives a top-level description.
Details like the reasoning, reference to relevant commits, etc.
belong in the commit message contents.
^ permalink raw reply
* Re: [rds-devel] [PATCH] rds: prevent BUG_ON triggering on congestion map updates
From: David Miller @ 2011-03-08 19:22 UTC (permalink / raw)
To: chris.mason; +Cc: nhorman, netdev, rds-devel, venkat.x.venkatsubra
In-Reply-To: <1299610642-sup-5101@think>
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.
^ permalink raw reply
* Re: [PATCH] tcp_cubic: enable TCP timestamps
From: Stephen Hemminger @ 2011-03-08 19:17 UTC (permalink / raw)
To: David Miller; +Cc: lucas.nussbaum, netdev, sha2
In-Reply-To: <20110308.105503.104060436.davem@davemloft.net>
On Tue, 08 Mar 2011 10:55:03 -0800 (PST)
David Miller <davem@davemloft.net> wrote:
Complete the sentence...
> on old desktops.
But without it enabling RTT_STAMP, packets that
get acked in less than a jiffie (1 - 10 ms) will
be processed with no rtt value (-1); this causes hystart
never to be invoked on local connections.
Maybe the following would be better, it passes 0 as RTT
when not using RTT_STAMP for quick packets.
It turns out the Cubic ends up converting rtt_us back
to jiffies anyway, so RTT_STAMP is not really needed.
----
Subject: tcp: fix RTT for quick packets in congestion control
In the congestion control interface, the callback for each ACK
includes an estimated round trip time in microseconds.
Some algorithms need high resolution (Vegas style) but most only
need jiffie resolution. If RTT is not accurate (like a retransmission)
-1 is used as a flag value.
When doing coarse resolution if RTT is less than a a jiffie
then 0 should be returned rather than no estimate. Otherwise algorithms
that expect good ack's to trigger slow start (like CUBIC Hystart)
will be confused.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/net/ipv4/tcp_input.c 2011-03-08 11:11:26.093183654 -0800
+++ b/net/ipv4/tcp_input.c 2011-03-08 11:11:46.641404939 -0800
@@ -3350,7 +3350,7 @@ static int tcp_clean_rtx_queue(struct so
net_invalid_timestamp()))
rtt_us = ktime_us_delta(ktime_get_real(),
last_ackt);
- else if (ca_seq_rtt > 0)
+ else if (ca_seq_rtt >= 0)
rtt_us = jiffies_to_usecs(ca_seq_rtt);
}
^ permalink raw reply
* Re: [PATCH] tcp_cubic: enable TCP timestamps
From: Sangtae Ha @ 2011-03-08 19:15 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, lucas.nussbaum, netdev
In-Reply-To: <20110308.105503.104060436.davem@davemloft.net>
Yes. I remember that CONFIG_HZ was 1000 at that time and the value of
CONFIG_HZ could affect the algorithm.
I don't think HyStart needs this extra RTT_STAMP since we only need a
rough delay estimate.
Let me check HyStart with the latest git and with different CONFIG_HZ values.
Sangtae
On Tue, Mar 8, 2011 at 1:55 PM, David Miller <davem@davemloft.net> wrote:
>
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Tue, 8 Mar 2011 10:42:11 -0800
>
> > On Tue, 8 Mar 2011 09:09:26 +0100
> > Lucas Nussbaum <lucas.nussbaum@loria.fr> wrote:
> >
> >> The Hystart slow start algorithm requires precise RTT delay measurements
> >> to decide when to leave slow start. However, currently, CUBIC doesn't
> >> enable TCP timestamps. This can cause Hystart to mis-estimate the RTT,
> >> and to leave slow start too early, generating bad performance since
> >> convergence to the optimal cwnd is slower.
> >>
> >> Timestamps are already used by TCP Illinois, LP, Vegas, Veno and Yeah.
> >>
> >> Signed-off-by: Lucas Nussbaum <lucas.nussbaum@loria.fr>
> >
> > Just to explain what RTT_STAMP does. It causes the tcp receive code
> > to compute the rtt using high resolution clocks rather than just
> > jiffies. It requires access to ktime_get_real which means accessing
> > clock source. This is cheap for TSC, a little expensive for HPET but
> > expensive for PIT. I worry that enabling it may hurt regular users
> > on old desktops. But without it enabling RTT_STAMP, packets that
> > get acked in less than a jiffie (1 - 10 ms) will
> >
> > Also I should have used ktime_get rather than ktime_get_real
> > because real time is altered by NTP and other actions.
>
> This also means that whatever testing was originally done on Hystart
> is dependent upon whatever value CONFIG_HZ had in the test kernel.
^ permalink raw reply
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards
From: Michael Chan @ 2011-03-08 19:07 UTC (permalink / raw)
To: Ben Hutchings
Cc: Neil Horman, netdev@vger.kernel.org, David S. Miller,
Dmitry Kravkov, Eddie Wai, Eilon Greenstein
In-Reply-To: <1299610768.4957.1.camel@bwh-desktop>
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?
>
> And why is a function named 'is_cnic_dev' doing initialisation, anyway?
Just data structure init if a supported device is detected.
>
> 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);
>
^ permalink raw reply
* Re: [net-next-2.6 00/14][pull request] Intel Wired LAN Driver Updates
From: David Miller @ 2011-03-08 19:11 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev, gospo, bphilips
In-Reply-To: <1299551108-23663-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Mon, 7 Mar 2011 18:24:54 -0800
> The following series contains fixes and cleanups for e1000e and ixgbe, as
> well as support for FCoE DDP in target mode for ixgbe. In igb, added
> additional stats for OS2BMC feature.
>
> The following are changes since commit 541ac7c9b30ee2ff84ad87f27e0bc069e143afb5:
> bonding: COW before overwriting the destination MAC address
>
> and are available in the git repository at:
> master.kernel.org:/pub/scm/linux/kernel/git/jkirsher/net-next-2.6 master
Pulled.
Please address the feedback given on the lack of docbook descriptions for
all of those fcoe device ops.
^ permalink raw reply
* Re: [PATCH] ipv4: Cache source address in nexthop entries.
From: David Miller @ 2011-03-08 19:03 UTC (permalink / raw)
To: ja; +Cc: netdev
In-Reply-To: <20110308.103801.193702247.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Tue, 08 Mar 2011 10:38:01 -0800 (PST)
> From: Julian Anastasov <ja@ssi.bg>
> Date: Tue, 8 Mar 2011 11:57:38 +0200 (EET)
>
>> It means, even if there are addresses on the
>> concerned device it does not mean the routes on this device
>> are required to use prefsrc from this device. We must
>> restrict the scope according to the provided for the
>> route: cfg->fc_scope.
>
> Simple to fix, I'll remember the fc_scope value in the nexthop
> and use that in the address selection call.
I'll push the following into net-next-2.6, thanks Julian:
--------------------
ipv4: Fix scope value used in route src-address caching.
We have to use cfg->fc_scope not the final nh_scope value.
Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/net/ip_fib.h | 1 +
net/ipv4/fib_semantics.c | 5 +++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index 0e14083..3f6c943 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -51,6 +51,7 @@ struct fib_nh {
struct fib_info *nh_parent;
unsigned nh_flags;
unsigned char nh_scope;
+ unsigned char nh_cfg_scope;
#ifdef CONFIG_IP_ROUTE_MULTIPATH
int nh_weight;
int nh_power;
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 952c737..d73d758 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -854,9 +854,10 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
}
change_nexthops(fi) {
+ nexthop_nh->nh_cfg_scope = cfg->fc_scope;
nexthop_nh->nh_saddr = inet_select_addr(nexthop_nh->nh_dev,
nexthop_nh->nh_gw,
- nexthop_nh->nh_scope);
+ nexthop_nh->nh_cfg_scope);
} endfor_nexthops(fi)
link_it:
@@ -1141,7 +1142,7 @@ void fib_update_nh_saddrs(struct net_device *dev)
continue;
nh->nh_saddr = inet_select_addr(nh->nh_dev,
nh->nh_gw,
- nh->nh_scope);
+ nh->nh_cfg_scope);
}
}
--
1.7.4.1
^ permalink raw reply related
* Re: [rds-devel] [PATCH] rds: prevent BUG_ON triggering on congestion map updates
From: Chris Mason @ 2011-03-08 18:58 UTC (permalink / raw)
To: David Miller; +Cc: nhorman, netdev, rds-devel, venkat.x.venkatsubra
In-Reply-To: <20110308.105223.71117584.davem@davemloft.net>
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>
-chris
^ permalink raw reply
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards
From: Ben Hutchings @ 2011-03-08 18:59 UTC (permalink / raw)
To: Neil Horman
Cc: netdev, David S. Miller, Michael Chan, Dmitry Kravkov, Eddie Wai,
Eilon Greenstein
In-Reply-To: <1299610588-22356-1-git-send-email-nhorman@tuxdriver.com>
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?
And why is a function named 'is_cnic_dev' doing initialisation, anyway?
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);
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH] cnic: fix double initalization of bnx2 based cards
From: Neil Horman @ 2011-03-08 18:56 UTC (permalink / raw)
To: netdev
Cc: Neil Horman, David S. Miller, Michael Chan, Dmitry Kravkov,
Eddie Wai, Eilon Greenstein
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). 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);
--
1.7.4
^ permalink raw reply related
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