* RE: [net-next 1/9] igb: Add support functions to access thermal data.
From: Wyborny, Carolyn @ 2012-06-13 14:53 UTC (permalink / raw)
To: Ben Hutchings, Kirsher, Jeffrey T
Cc: davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com,
sassmann@redhat.com
In-Reply-To: <1339289333.21665.156.camel@deadeye.wl.decadent.org.uk>
>-----Original Message-----
>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Saturday, June 09, 2012 5:49 PM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Wyborny, Carolyn; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com
>Subject: Re: [net-next 1/9] igb: Add support functions to access thermal
>data.
>
>On Sat, 2012-06-09 at 01:20 -0700, Jeff Kirsher wrote:
>> From: Carolyn Wyborny <carolyn.wyborny@intel.com>
>>
>> Some i350 devices contain thermal data that we can get to via an i2c
>> interface. These functions provide support to get at that data. A
>> following patch will export this data.
>[...]
>
>We already have an I2C subsystem - so don't replicate i2c-algo-bit etc.
>
>It's possible we also have an hwmon driver for whatever temperature
>sensor is on these boards, which you can then probe by calling
>i2c_new_device().
I am not sure that our sensors will be compatible, but I will research it to be sure. I will also take a look at what we can do with the i2c interface that already exists and make sure we are not duplicating any of it.
Thanks,
Carolyn
Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation
^ permalink raw reply
* Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function
From: Ben Hutchings @ 2012-06-13 15:14 UTC (permalink / raw)
To: Takuya Yoshikawa
Cc: Akinobu Mita, Takuya Yoshikawa, akpm, grundler, arnd, benh, avi,
mtosatti, linux-net-drivers, netdev, linux-kernel, linux-arch,
kvm
In-Reply-To: <20120613230013.3cc59bf908616e94bb4ccef2@gmail.com>
On Wed, 2012-06-13 at 23:00 +0900, Takuya Yoshikawa wrote:
> On Wed, 13 Jun 2012 22:31:13 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> > >> Should this hash_table be converted from u16 hash_table[32] to
> > >> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
> > >> on long-word boundary?
> > >
> > > I think hash_table is already long-word aligned because it is placed
> > > right after a pointer.
> >
> > I recommend converting to proper bitmap. Because such an implicit
> > assumption is easily broken by someone touching this function.
>
> Do you mean something like:
> DECLARE_BITMAP(__hash_table, 16 * 32);
> u16 *hash_table = (u16 *)__hash_table;
> ?
[...]
Could this driver perhaps use:
union hash_table {
DECLARE_BITMAP(bits, 16 * 32);
__le16 words[32];
};
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
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 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function
From: Grant Grundler @ 2012-06-13 15:21 UTC (permalink / raw)
To: Takuya Yoshikawa
Cc: Akinobu Mita, Takuya Yoshikawa, akpm, bhutchings, grundler, arnd,
benh, avi, mtosatti, linux-net-drivers, netdev, linux-kernel,
linux-arch, kvm
In-Reply-To: <20120613230013.3cc59bf908616e94bb4ccef2@gmail.com>
On Wed, Jun 13, 2012 at 7:00 AM, Takuya Yoshikawa
<takuya.yoshikawa@gmail.com> wrote:
> On Wed, 13 Jun 2012 22:31:13 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
>> >> Should this hash_table be converted from u16 hash_table[32] to
>> >> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
>> >> on long-word boundary?
>> >
>> > I think hash_table is already long-word aligned because it is placed
>> > right after a pointer.
>>
>> I recommend converting to proper bitmap. Because such an implicit
>> assumption is easily broken by someone touching this function.
>
> Do you mean something like:
> DECLARE_BITMAP(__hash_table, 16 * 32);
> u16 *hash_table = (u16 *)__hash_table;
> ?
>
> Grant, what do you think about this?
Hi Takuya,
two thoughts:
1) while I agree with Akinobu and thank him for pointing out a
_potential_ alignment problem, this is a separate issue and your
existing patch should go in anyway. There are probably other drivers
with _potential_ alignment issues. Akinobu could get credit for
finding them by submitting patches after reviewing calls to set_bit
and set_bit_le() - similar to what you are doing now.
2) I generally do not like declaring one type and then using an alias
of a different type to reference the same memory address. We have a
simple alternative since hash_table[] is indexed directly only in one
hunk of code:
for (i = 0; i < 32; i++) {
*setup_frm++ = ((u16 *)hash_table)[i];
*setup_frm++ = ((u16 *)hash_table)[i];
}
thanks,
grant
>
> Takuya
>
>
> ===
> drivers/net/ethernet/dec/tulip/tulip_core.c:
>
> static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
> {
> struct tulip_private *tp = netdev_priv(dev);
> u16 hash_table[32];
> ...
> }
>
> drivers/net/ethernet/dec/tulip/de2104x.c:
>
> static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
> {
> struct de_private *de = netdev_priv(dev);
> u16 hash_table[32];
> ...
> }
^ permalink raw reply
* [PATCH net-next] bonding: drop_monitor aware
From: Eric Dumazet @ 2012-06-13 15:30 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Jay Vosburgh
From: Eric Dumazet <edumazet@google.com>
When packets are dropped in TX path, its better to use kfree_skb()
instead of dev_kfree_skb() to give proper drop_monitor events.
Also move the kfree_skb() call after read_unlock() in bond_alb_xmit()
and bond_xmit_activebackup()
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
---
drivers/net/bonding/bond_3ad.c | 2 +-
drivers/net/bonding/bond_alb.c | 6 +++---
drivers/net/bonding/bond_main.c | 18 +++++++++---------
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 3031e04..a030e63 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2454,7 +2454,7 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
out:
if (res) {
/* no suitable interface, frame not sent */
- dev_kfree_skb(skb);
+ kfree_skb(skb);
}
return NETDEV_TX_OK;
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index ef3791a..e15cc11 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1346,12 +1346,12 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
}
}
+ read_unlock(&bond->curr_slave_lock);
+
if (res) {
/* no suitable interface, frame not sent */
- dev_kfree_skb(skb);
+ kfree_skb(skb);
}
- read_unlock(&bond->curr_slave_lock);
-
return NETDEV_TX_OK;
}
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index af50632..f5a40b9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3990,7 +3990,7 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
out:
if (res) {
/* no suitable interface, frame not sent */
- dev_kfree_skb(skb);
+ kfree_skb(skb);
}
return NETDEV_TX_OK;
@@ -4012,11 +4012,11 @@ static int bond_xmit_activebackup(struct sk_buff *skb, struct net_device *bond_d
res = bond_dev_queue_xmit(bond, skb,
bond->curr_active_slave->dev);
+ read_unlock(&bond->curr_slave_lock);
+
if (res)
/* no suitable interface, frame not sent */
- dev_kfree_skb(skb);
-
- read_unlock(&bond->curr_slave_lock);
+ kfree_skb(skb);
return NETDEV_TX_OK;
}
@@ -4055,7 +4055,7 @@ static int bond_xmit_xor(struct sk_buff *skb, struct net_device *bond_dev)
if (res) {
/* no suitable interface, frame not sent */
- dev_kfree_skb(skb);
+ kfree_skb(skb);
}
return NETDEV_TX_OK;
@@ -4093,7 +4093,7 @@ static int bond_xmit_broadcast(struct sk_buff *skb, struct net_device *bond_dev)
res = bond_dev_queue_xmit(bond, skb2, tx_dev);
if (res) {
- dev_kfree_skb(skb2);
+ kfree_skb(skb2);
continue;
}
}
@@ -4107,7 +4107,7 @@ static int bond_xmit_broadcast(struct sk_buff *skb, struct net_device *bond_dev)
out:
if (res)
/* no suitable interface, frame not sent */
- dev_kfree_skb(skb);
+ kfree_skb(skb);
/* frame sent to all suitable interfaces */
return NETDEV_TX_OK;
@@ -4213,7 +4213,7 @@ static netdev_tx_t __bond_start_xmit(struct sk_buff *skb, struct net_device *dev
pr_err("%s: Error: Unknown bonding mode %d\n",
dev->name, bond->params.mode);
WARN_ON_ONCE(1);
- dev_kfree_skb(skb);
+ kfree_skb(skb);
return NETDEV_TX_OK;
}
}
@@ -4235,7 +4235,7 @@ static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)
if (bond->slave_cnt)
ret = __bond_start_xmit(skb, dev);
else
- dev_kfree_skb(skb);
+ kfree_skb(skb);
read_unlock(&bond->lock);
^ permalink raw reply related
* Re: PPPoE performance regression
From: Benjamin LaHaise @ 2012-06-13 15:55 UTC (permalink / raw)
To: David Woodhouse
Cc: Nathan Williams, Karl Hiramoto, David S. Miller, netdev,
Paul Mackerras, John Crispin
In-Reply-To: <1339595401.11011.48.camel@shinybook.infradead.org>
On Wed, Jun 13, 2012 at 02:50:01PM +0100, David Woodhouse wrote:
> On Wed, 2012-06-13 at 10:57 +0100, David Woodhouse wrote:
> > This doesn't look *so* evil... if the basic concept of using
> > skb_orphan() and then setting our own destructor is OK, then I'll work
> > out the rest of the details and do it for l2tp too.
>
> Stupid dwmw2. With patch this time...
Does this actually work? Could the skb not end up sitting on the receive
queue of a user socket indefinitely, deferring all further transmits? From
an ISP point of view, PPPoE and L2TP are most typically used on links where
the congestion point is not the local interface the packets are being pumped
into (think of the vast majority of ethernet based DSL modems), and this
kind of transmit overhead is a pure waste of CPU cycles. The only solution
that generically works in most PPPoE/L2TP situations is to shape outgoing
traffic to the speed of limiting link.
Maybe there's a PPP extension that does flow control...
-ben
^ permalink raw reply
* Re: PPPoE performance regression
From: David Woodhouse @ 2012-06-13 16:11 UTC (permalink / raw)
To: Benjamin LaHaise
Cc: Nathan Williams, Karl Hiramoto, David S. Miller, netdev,
Paul Mackerras, John Crispin
In-Reply-To: <20120613155541.GD2361@kvack.org>
[-- Attachment #1: Type: text/plain, Size: 931 bytes --]
On Wed, 2012-06-13 at 11:55 -0400, Benjamin LaHaise wrote:
> Does this actually work? Could the skb not end up sitting on the
> receive queue of a user socket indefinitely, deferring all further
> transmits? From an ISP point of view,
I haven't tried it; only compiled it. Certainly, the similar approach in
PPPoATM in commit 9d02daf7 *does* work for limiting the bufferbloat and
keeping the queues under control. And it'll let me do BQL for PPPoA.
I'm looking at this from the client side, not the ISP side. And in that
case the local interface *is* the bottleneck. When it's a PPPoE over
br2684 interface and it's full, we should stop the PPP netdev from
spewing packets at it, rather than just dropping them.
On the ISP side if the skb ends up sitting on a receive queue of a user
socket, and nothing is servicing that socket, surely the transmits on
this channel weren't happening anyway?
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]
^ permalink raw reply
* Re: PPPoE performance regression
From: Benjamin LaHaise @ 2012-06-13 16:31 UTC (permalink / raw)
To: David Woodhouse
Cc: Nathan Williams, Karl Hiramoto, David S. Miller, netdev,
Paul Mackerras, John Crispin
In-Reply-To: <1339603894.14785.5.camel@shinybook.infradead.org>
On Wed, Jun 13, 2012 at 05:11:34PM +0100, David Woodhouse wrote:
> On Wed, 2012-06-13 at 11:55 -0400, Benjamin LaHaise wrote:
> > Does this actually work? Could the skb not end up sitting on the
> > receive queue of a user socket indefinitely, deferring all further
> > transmits? From an ISP point of view,
>
> I haven't tried it; only compiled it. Certainly, the similar approach in
> PPPoATM in commit 9d02daf7 *does* work for limiting the bufferbloat and
> keeping the queues under control. And it'll let me do BQL for PPPoA.
>
> I'm looking at this from the client side, not the ISP side. And in that
> case the local interface *is* the bottleneck. When it's a PPPoE over
> br2684 interface and it's full, we should stop the PPP netdev from
> spewing packets at it, rather than just dropping them.
I would contend that PPPoE over br2684 is not the common case. The vast
majority of users in client mode are going to be using PPPoE over an
ethernet link to a DSL modem (or cable or wireless radios even). Just look
at what DSL modems are available for users in computer stores / what ISPs
actually ship to their users. Real ATM exposing devices are rare.
> On the ISP side if the skb ends up sitting on a receive queue of a user
> socket, and nothing is servicing that socket, surely the transmits on
> this channel weren't happening anyway?
True, but it's a design issue we've had to contend with elsewhere in the
various tunnelling protocols.
Don't get me wrong: I am very much in favour of intelligent queue
management, but this approach simply does not work for the vast majority
of PPPoE users, while it adds overhead that will negatively impact access
concentrators. If you can somehow restrict the overhead to only impacting
your use-case, that would be an improvement.
-ben
--
"Thought is the essence of where you are now."
^ permalink raw reply
* RE: PPPoE performance regression
From: David Laight @ 2012-06-13 16:32 UTC (permalink / raw)
To: Benjamin LaHaise, David Woodhouse
Cc: Nathan Williams, Karl Hiramoto, David S. Miller, netdev,
Paul Mackerras, John Crispin
In-Reply-To: <20120613163108.GE2361@kvack.org>
> I would contend that PPPoE over br2684 is not the common case. The
vast
> majority of users in client mode are going to be using PPPoE over an
> ethernet link to a DSL modem (or cable or wireless radios even). Just
look
> at what DSL modems are available for users in computer stores / what
ISPs
> actually ship to their users. Real ATM exposing devices are rare.
PPPoA is common in the UK.
David
^ permalink raw reply
* Re: PPPoE performance regression
From: David Woodhouse @ 2012-06-13 16:53 UTC (permalink / raw)
To: Benjamin LaHaise
Cc: Nathan Williams, Karl Hiramoto, David S. Miller, netdev,
Paul Mackerras, John Crispin
In-Reply-To: <20120613163108.GE2361@kvack.org>
[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]
On Wed, 2012-06-13 at 12:31 -0400, Benjamin LaHaise wrote:
> I would contend that PPPoE over br2684 is not the common case. The vast
> majority of users in client mode are going to be using PPPoE over an
> ethernet link to a DSL modem (or cable or wireless radios even). Just look
> at what DSL modems are available for users in computer stores / what ISPs
> actually ship to their users. Real ATM exposing devices are rare.
I'm looking at the class of device on which OpenWRT runs. Linux is *on*
the router with the ADSL port, not connected to it via Ethernet.
I can buy lots of these in the shop. Anything that's an ADSL *router*
rather than *modem* is likely to be running, or at least capable of
running, Linux.
Admittedly, if you have access to the native ADSL interface then you'd
do a lot better to run PPPoA — but I already fixed this issue for PPPoA.
There are people in some parts of the world who are using PPPoEoA and
putting up with the resulting MTU issues because the *ISP* doesn't
support proper PPPoA.
And even if it *were* rare... this is the case that *should* work best,
where we have complete control of the hardware. There's no excuse for
the behaviour that we currently see with PPPoE on BR2684.
> > On the ISP side if the skb ends up sitting on a receive queue of a user
> > socket, and nothing is servicing that socket, surely the transmits on
> > this channel weren't happening anyway?
>
> True, but it's a design issue we've had to contend with elsewhere in the
> various tunnelling protocols.
>
> Don't get me wrong: I am very much in favour of intelligent queue
> management, but this approach simply does not work for the vast majority
> of PPPoE users, while it adds overhead that will negatively impact access
> concentrators.
I think that's largely true of BQL in general, isn't it? That's OK; it's
a config option. I suspect if I make this accounting of PPPoE / L2TP
packets conditional on BQL (or perhaps on a separate config option
PPP_BQL) that ought to address your concern about the cases where you
don't need it?
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]
^ permalink raw reply
* Re: PPPoE performance regression
From: David Woodhouse @ 2012-06-13 16:59 UTC (permalink / raw)
To: David Laight
Cc: Benjamin LaHaise, Nathan Williams, Karl Hiramoto, David S. Miller,
netdev, Paul Mackerras, John Crispin
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B6F47@saturn3.aculab.com>
[-- Attachment #1: Type: text/plain, Size: 932 bytes --]
On Wed, 2012-06-13 at 17:32 +0100, David Laight wrote:
> > I would contend that PPPoE over br2684 is not the common case. The vast
> > majority of users in client mode are going to be using PPPoE over an
> > ethernet link to a DSL modem (or cable or wireless radios even). Just look
> > at what DSL modems are available for users in computer stores / what ISPs
> > actually ship to their users. Real ATM exposing devices are rare.
>
> PPPoA is common in the UK.
In the UK you tend to have the option of using PPPoA *or* PPPoE over
BR2684. The ISP's kit will handle both.
Ben's comment was about the *hardware*, though. If your "ADSL modem" is
a separate box which just bridges Ethernet to BR2684 on the ADSL link,
you're limited to using the PPPoE protocol over that.
Obviously, if you have a proper ADSL *router* and it's not just a PPP
bridge, then you can — and should — use PPPoA.
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]
^ permalink raw reply
* RE: [net-next 1/9] igb: Add support functions to access thermal data.
From: Ben Hutchings @ 2012-06-13 17:06 UTC (permalink / raw)
To: Wyborny, Carolyn
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <9BBC4E0CF881AA4299206E2E1412B626274A6F4B@ORSMSX102.amr.corp.intel.com>
On Wed, 2012-06-13 at 14:53 +0000, Wyborny, Carolyn wrote:
> >-----Original Message-----
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Saturday, June 09, 2012 5:49 PM
> >To: Kirsher, Jeffrey T
> >Cc: davem@davemloft.net; Wyborny, Carolyn; netdev@vger.kernel.org;
> >gospo@redhat.com; sassmann@redhat.com
> >Subject: Re: [net-next 1/9] igb: Add support functions to access thermal
> >data.
> >
> >On Sat, 2012-06-09 at 01:20 -0700, Jeff Kirsher wrote:
> >> From: Carolyn Wyborny <carolyn.wyborny@intel.com>
> >>
> >> Some i350 devices contain thermal data that we can get to via an i2c
> >> interface. These functions provide support to get at that data. A
> >> following patch will export this data.
> >[...]
> >
> >We already have an I2C subsystem - so don't replicate i2c-algo-bit etc.
> >
> >It's possible we also have an hwmon driver for whatever temperature
> >sensor is on these boards, which you can then probe by calling
> >i2c_new_device().
>
> I am not sure that our sensors will be compatible, but I will research
> it to be sure. I will also take a look at what we can do with the i2c
> interface that already exists and make sure we are not duplicating any
> of it.
The patch you posted duplicates i2c-algo-bit. The sfc driver initially
did this too, but I was rightly required to fix that. So if you want an
example, you could do worse than looking for references to 'i2c' in
drivers/net/ethernet/sfc/falcon.c and
drivers/net/ethernet/sfc/falcon_boards.c.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
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: [net-next 1/9] igb: Add support functions to access thermal data.
From: Wyborny, Carolyn @ 2012-06-13 17:15 UTC (permalink / raw)
To: Ben Hutchings
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <1339607189.2612.8.camel@bwh-desktop.uk.solarflarecom.com>
>-----Original Message-----
>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Wednesday, June 13, 2012 10:06 AM
>To: Wyborny, Carolyn
>Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com
>Subject: RE: [net-next 1/9] igb: Add support functions to access thermal
>data.
>
>On Wed, 2012-06-13 at 14:53 +0000, Wyborny, Carolyn wrote:
>> >-----Original Message-----
>> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>> >Sent: Saturday, June 09, 2012 5:49 PM
>> >To: Kirsher, Jeffrey T
>> >Cc: davem@davemloft.net; Wyborny, Carolyn; netdev@vger.kernel.org;
>> >gospo@redhat.com; sassmann@redhat.com
>> >Subject: Re: [net-next 1/9] igb: Add support functions to access
>thermal
>> >data.
>> >
>> >On Sat, 2012-06-09 at 01:20 -0700, Jeff Kirsher wrote:
>> >> From: Carolyn Wyborny <carolyn.wyborny@intel.com>
>> >>
>> >> Some i350 devices contain thermal data that we can get to via an
>i2c
>> >> interface. These functions provide support to get at that data. A
>> >> following patch will export this data.
>> >[...]
>> >
>> >We already have an I2C subsystem - so don't replicate i2c-algo-bit
>etc.
>> >
>> >It's possible we also have an hwmon driver for whatever temperature
>> >sensor is on these boards, which you can then probe by calling
>> >i2c_new_device().
>>
>> I am not sure that our sensors will be compatible, but I will research
>> it to be sure. I will also take a look at what we can do with the i2c
>> interface that already exists and make sure we are not duplicating any
>> of it.
>
>The patch you posted duplicates i2c-algo-bit. The sfc driver initially
>did this too, but I was rightly required to fix that. So if you want an
>example, you could do worse than looking for references to 'i2c' in
>drivers/net/ethernet/sfc/falcon.c and
>drivers/net/ethernet/sfc/falcon_boards.c.
Thanks Ben, appreciate the pointer. I'll start there.
Carolyn
Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation
^ permalink raw reply
* Re: PPPoE performance regression
From: Benjamin LaHaise @ 2012-06-13 17:21 UTC (permalink / raw)
To: David Woodhouse
Cc: Nathan Williams, Karl Hiramoto, David S. Miller, netdev,
Paul Mackerras, John Crispin
In-Reply-To: <1339606383.14785.14.camel@shinybook.infradead.org>
On Wed, Jun 13, 2012 at 05:53:03PM +0100, David Woodhouse wrote:
> I'm looking at the class of device on which OpenWRT runs. Linux is *on*
> the router with the ADSL port, not connected to it via Ethernet.
Ah, yes, that's a worthwhile pursuit.
> And even if it *were* rare... this is the case that *should* work best,
> where we have complete control of the hardware. There's no excuse for
> the behaviour that we currently see with PPPoE on BR2684.
*nod*
> I think that's largely true of BQL in general, isn't it? That's OK; it's
> a config option. I suspect if I make this accounting of PPPoE / L2TP
> packets conditional on BQL (or perhaps on a separate config option
> PPP_BQL) that ought to address your concern about the cases where you
> don't need it?
That would help.
On the whole question of PPPoE over intermediate ethernet links to ADSL
modems, I think it would be possible to limit latency by implementing a
sliding window clocked using LCP ECHO requests. Does this sound worthwhile
implementing? What sort of queue depths are you looking at for the ATM
devices you're working on?
-ben
--
"Thought is the essence of where you are now."
^ permalink raw reply
* Re: PPPoE performance regression
From: David Woodhouse @ 2012-06-13 17:43 UTC (permalink / raw)
To: Benjamin LaHaise
Cc: Nathan Williams, Karl Hiramoto, David S. Miller, netdev,
Paul Mackerras, John Crispin
In-Reply-To: <20120613172122.GF2361@kvack.org>
[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]
On Wed, 2012-06-13 at 13:21 -0400, Benjamin LaHaise wrote:
> On the whole question of PPPoE over intermediate ethernet links to ADSL
> modems, I think it would be possible to limit latency by implementing a
> sliding window clocked using LCP ECHO requests. Does this sound worthwhile
> implementing?
Maybe, for someone who actually cares about those who are using separate
ADSL modems, and doesn't think they should just better hardware if they
care about the performance and queue management :)
> What sort of queue depths are you looking at for the ATM devices
> you're working on?
We're managing the queue to keep it short. On the PPPoATM side, we (now)
strictly limit the number of packets between the ppp_generic code and
the ATM device. It's basically one in-flight and one waiting to be
handed to the device in the TX done interrupt. PPP is designed to feed
new packets with low latency, after all.
The Solos ADSL device *used* to eat as many packets as it had internal
RAM to store them in, acknowledging the "transmit" as soon as it had
buffered them. I got Nathan to fix that some time ago, and the internal
queue is fairly short now.
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]
^ permalink raw reply
* Re: [net-next patch 1/12] bnx2x: Add support for external LB
From: Ben Hutchings @ 2012-06-13 18:17 UTC (permalink / raw)
To: Merav Sicron; +Cc: eilong, davem, netdev
In-Reply-To: <1339591464-10554-2-git-send-email-meravs@broadcom.com>
On Wed, 2012-06-13 at 15:44 +0300, Merav Sicron wrote:
> This change enables to do self-test with external loopback via ethtool.
>
> Signed-off-by: Merav Sicron <meravs@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
Acked-by: Ben Hutchings <bhutchings@solarflare.com>
The ethtool bits look right.
[...]
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -2124,7 +2124,12 @@ u8 bnx2x_initial_phy_init(struct bnx2x *bp, int load_mode)
> }
> }
>
> - rc = bnx2x_phy_init(&bp->link_params, &bp->link_vars);
> + if (load_mode == LOAD_LOOPBACK_EXT) {
> + struct link_params *lp = &bp->link_params;
> + lp->loopback_mode = LOOPBACK_EXT;
> + }
> +
> + rc = bnx2x_phy_init(&bp->link_params, &bp->link_vars);
>
> bnx2x_release_phy_lock(bp);
Indentation is wrong here, though.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
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: [net-next patch 3/12] bnx2x: Add support for 4-tupple UDP RSS
From: Ben Hutchings @ 2012-06-13 18:20 UTC (permalink / raw)
To: Merav Sicron; +Cc: eilong, davem, netdev
In-Reply-To: <1339591464-10554-4-git-send-email-meravs@broadcom.com>
On Wed, 2012-06-13 at 15:44 +0300, Merav Sicron wrote:
> This change enables to control via ethtool whether to do UDP RSS on 2-tupple
> (IP source / destination only) or on 4-tupple (include UDP source / destination
> port). It also enables to read back the RSS configuration.
[...]
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
> @@ -2600,6 +2600,52 @@ static int bnx2x_set_phys_id(struct net_device *dev,
> return 0;
> }
>
> +static int bnx2x_get_rss_flags(struct bnx2x *bp, struct ethtool_rxnfc *info)
> +{
> +
> + switch (info->flow_type) {
> + case TCP_V4_FLOW:
> + case TCP_V6_FLOW:
> + info->data = RXH_IP_SRC | RXH_IP_DST |
> + RXH_L4_B_0_1 | RXH_L4_B_2_3;
> + break;
> + case UDP_V4_FLOW:
> + if (bp->rss_conf_obj.udp_rss_v4)
> + info->data = RXH_IP_SRC | RXH_IP_DST |
> + RXH_L4_B_0_1 | RXH_L4_B_2_3;
> + else
> + info->data = RXH_IP_SRC | RXH_IP_DST;
> + break;
> + case UDP_V6_FLOW:
> + if (bp->rss_conf_obj.udp_rss_v6)
> + info->data = RXH_IP_SRC | RXH_IP_DST |
> + RXH_L4_B_0_1 | RXH_L4_B_2_3;
> + else
> + info->data = RXH_IP_SRC | RXH_IP_DST;
> + break;
> + case IPV4_FLOW:
> + case IPV6_FLOW:
> + info->data = RXH_IP_SRC | RXH_IP_DST;
> + break;
> + case SCTP_V4_FLOW:
> + case AH_ESP_V4_FLOW:
> + case AH_V4_FLOW:
> + case ESP_V4_FLOW:
> + case SCTP_V6_FLOW:
> + case AH_ESP_V6_FLOW:
> + case AH_V6_FLOW:
> + case ESP_V6_FLOW:
> + case IP_USER_FLOW:
> + case ETHER_FLOW:
> + info->data = 0;
> + break;
> + default:
> + return -EINVAL;
> + }
[...]
Don't try to enumerate all flow types; we may add others in future. You
should set info->data = 0 for any type for which RSS is not supported.
Otherwise I think the ethtool bits are fine.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
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] bnx2x: fix panic when TX ring is full
From: Eric Dumazet @ 2012-06-13 19:45 UTC (permalink / raw)
To: David Miller
Cc: netdev, Tom Herbert, Robert Evans, Eilon Greenstein, Merav Sicron,
Yaniv Rosner, Willem de Bruijn, Tomas Hruby
From: Eric Dumazet <edumazet@google.com>
There is a off by one error in the minimal number of BD in
bnx2x_start_xmit() and bnx2x_tx_int() before stopping/resuming tx queue.
A full size GSO packet, with data included in skb->head really needs
(MAX_SKB_FRAGS + 4) BDs, because of bnx2x_tx_split()
This error triggers if BQL is disabled and heavy TCP transmit traffic
occurs.
bnx2x_tx_split() definitely can be called, remove a wrong comment.
Reported-by: Tomas Hruby <thruby@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Eilon Greenstein <eilong@broadcom.com>
Cc: Yaniv Rosner <yanivr@broadcom.com>
Cc: Merav Sicron <meravs@broadcom.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Robert Evans <evansr@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index ad0743b..302765a 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -190,7 +190,7 @@ int bnx2x_tx_int(struct bnx2x *bp, struct bnx2x_fp_txdata *txdata)
if ((netif_tx_queue_stopped(txq)) &&
(bp->state == BNX2X_STATE_OPEN) &&
- (bnx2x_tx_avail(bp, txdata) >= MAX_SKB_FRAGS + 3))
+ (bnx2x_tx_avail(bp, txdata) >= MAX_SKB_FRAGS + 4))
netif_tx_wake_queue(txq);
__netif_tx_unlock(txq);
@@ -2501,8 +2501,6 @@ int bnx2x_poll(struct napi_struct *napi, int budget)
/* we split the first BD into headers and data BDs
* to ease the pain of our fellow microcode engineers
* we use one mapping for both BDs
- * So far this has only been observed to happen
- * in Other Operating Systems(TM)
*/
static noinline u16 bnx2x_tx_split(struct bnx2x *bp,
struct bnx2x_fp_txdata *txdata,
@@ -3156,7 +3154,7 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
txdata->tx_bd_prod += nbd;
- if (unlikely(bnx2x_tx_avail(bp, txdata) < MAX_SKB_FRAGS + 3)) {
+ if (unlikely(bnx2x_tx_avail(bp, txdata) < MAX_SKB_FRAGS + 4)) {
netif_tx_stop_queue(txq);
/* paired memory barrier is in bnx2x_tx_int(), we have to keep
@@ -3165,7 +3163,7 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
smp_mb();
fp->eth_q_stats.driver_xoff++;
- if (bnx2x_tx_avail(bp, txdata) >= MAX_SKB_FRAGS + 3)
+ if (bnx2x_tx_avail(bp, txdata) >= MAX_SKB_FRAGS + 4)
netif_tx_wake_queue(txq);
}
txdata->tx_pkt++;
^ permalink raw reply related
* Re: PPPoE performance regression
From: Karl Hiramoto @ 2012-06-13 20:17 UTC (permalink / raw)
To: David Woodhouse; +Cc: Nathan Williams, David S. Miller, netdev
In-Reply-To: <1339317136.2851.54.camel@shinybook.infradead.org>
On 06/10/12 10:32, David Woodhouse wrote:
> On Sun, 2012-06-10 at 10:50 +1000, Nathan Williams wrote:
>>> When using iperf with UDP, we can get 20Mbps downstream, but only about
>>> 15Mbps throughput when using TCP on a short ADSL line (line sync at
>>> 25Mbps). Using iperf to send UDP traffic upstream at the same time
>>> doesn't affect the downstream rate.
>> ...
>>
>> I found the change responsible for the performance problem and rebuilt
>> OpenWrt with the patch reversed on kernel 3.3.8 to confirm everything
>> still works. So the TX buffer is getting full, which causes the netif
>> queue to be stopped and restarted after some skbs have been freed?
> The *Ethernet* netif queue, yes. But not the PPP netif queue, I believe.
> I think the PPP code keeps just blindly calling dev_queue_xmit() and
> throwing away packets when they're not accepted.
>
>> commit 137742cf9738f1b4784058ff79aec7ca85e769d4
>> Author: Karl Hiramoto <karl@hiramoto.org>
>> Date: Wed Sep 2 23:26:39 2009 -0700
>>
>> atm/br2684: netif_stop_queue() when atm device busy and
>> netif_wake_queue() when we can send packets again.
> Nice work; well done finding that. I've added Karl and DaveM, and the
> netdev@ list to Cc.
>
> (Btw, I assume the performance problem also goes away if you use PPPoA?
> I've made changes in the PPPoA code recently to *eliminate* excessive
> calls to netif_wake_queue(), and also to stop it from filling the ATM
> device queue. That was commit 9d02daf7 in 3.5-rc1, which is already in
> OpenWRT.)
>
> I was already looking vaguely at how we could limit the PPP queue depth
> for PPPoE and implement byte queue limits. Currently the PPP code just
> throws the packets at the Ethernet device and considers them 'gone',
> which is why it's hitting the ATM limits all the time. The patch you
> highlight is changing the behaviour in a case that should never *happen*
> with PPP. It's suffering massive queue bloat if it's filling the ATM
> queue, and we should fix *that*.
Agreed, the issue is the PPP layer. I've seen this issue with PPPoE
before, haven't had the itch, time or interest to fix it though. A
workaround to help mitigate the issue may be to increase the TX queue
length of the br2684 interface, and the atm device if possible.
You'll pay the price of buffer bloat and latency.
--
Karl
^ permalink raw reply
* [PATCH] leds: Rename led_brightness_set() to led_set_brightness()
From: Shuah Khan @ 2012-06-13 20:34 UTC (permalink / raw)
To: bryan.wu, rpurdie, johannes, linville, davem
Cc: shuahkhan, LKML, linux-wireless, netdev
Rename leds external interface led_brightness_set() to led_set_brightness().
This is the second phase of the change to reduce confusion between the
leds internal and external interfaces that set brightness. With this change,
now the external interface is led_set_brightness(). The first phase renamed
the internal interface led_set_brightness() to __led_set_brightness().
There are no changes to the interface implementations.
Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
---
drivers/leds/led-class.c | 2 +-
drivers/leds/led-core.c | 4 ++--
drivers/leds/led-triggers.c | 2 +-
drivers/leds/ledtrig-oneshot.c | 2 +-
drivers/leds/ledtrig-timer.c | 2 +-
include/linux/leds.h | 4 ++--
net/mac80211/led.c | 2 +-
7 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index cb0a6eb..c599095 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -222,7 +222,7 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
#endif
/* Stop blinking */
- led_brightness_set(led_cdev, LED_OFF);
+ led_set_brightness(led_cdev, LED_OFF);
device_unregister(led_cdev->dev);
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 176961b..8a09c5f 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -103,7 +103,7 @@ void led_blink_set_oneshot(struct led_classdev *led_cdev,
}
EXPORT_SYMBOL(led_blink_set_oneshot);
-void led_brightness_set(struct led_classdev *led_cdev,
+void led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
/* stop and clear soft-blink timer */
@@ -113,4 +113,4 @@ void led_brightness_set(struct led_classdev *led_cdev,
__led_set_brightness(led_cdev, brightness);
}
-EXPORT_SYMBOL(led_brightness_set);
+EXPORT_SYMBOL(led_set_brightness);
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index f8b14dd..57721f2 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -112,7 +112,7 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
if (led_cdev->trigger->deactivate)
led_cdev->trigger->deactivate(led_cdev);
led_cdev->trigger = NULL;
- led_brightness_set(led_cdev, LED_OFF);
+ led_set_brightness(led_cdev, LED_OFF);
}
if (trig) {
write_lock_irqsave(&trig->leddev_list_lock, flags);
diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c
index 5cbab41..2c029aa 100644
--- a/drivers/leds/ledtrig-oneshot.c
+++ b/drivers/leds/ledtrig-oneshot.c
@@ -177,7 +177,7 @@ static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
}
/* Stop blinking */
- led_brightness_set(led_cdev, LED_OFF);
+ led_set_brightness(led_cdev, LED_OFF);
}
static struct led_trigger oneshot_led_trigger = {
diff --git a/drivers/leds/ledtrig-timer.c b/drivers/leds/ledtrig-timer.c
index 9010f7a..f774d05 100644
--- a/drivers/leds/ledtrig-timer.c
+++ b/drivers/leds/ledtrig-timer.c
@@ -104,7 +104,7 @@ static void timer_trig_deactivate(struct led_classdev *led_cdev)
}
/* Stop blinking */
- led_brightness_set(led_cdev, LED_OFF);
+ led_set_brightness(led_cdev, LED_OFF);
}
static struct led_trigger timer_led_trigger = {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index dd93a22..3aade1d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -124,7 +124,7 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
unsigned long *delay_off,
int invert);
/**
- * led_brightness_set - set LED brightness
+ * led_set_brightness - set LED brightness
* @led_cdev: the LED to set
* @brightness: the brightness to set it to
*
@@ -132,7 +132,7 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
* software blink timer that implements blinking when the
* hardware doesn't.
*/
-extern void led_brightness_set(struct led_classdev *led_cdev,
+extern void led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness brightness);
/*
diff --git a/net/mac80211/led.c b/net/mac80211/led.c
index 1bf7903..bcffa69 100644
--- a/net/mac80211/led.c
+++ b/net/mac80211/led.c
@@ -276,7 +276,7 @@ static void ieee80211_stop_tpt_led_trig(struct ieee80211_local *local)
read_lock(&tpt_trig->trig.leddev_list_lock);
list_for_each_entry(led_cdev, &tpt_trig->trig.led_cdevs, trig_list)
- led_brightness_set(led_cdev, LED_OFF);
+ led_set_brightness(led_cdev, LED_OFF);
read_unlock(&tpt_trig->trig.leddev_list_lock);
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/1 v3] Ethtool: Add EEE support
From: Ben Hutchings @ 2012-06-13 21:39 UTC (permalink / raw)
To: Yuval Mintz; +Cc: netdev, eilong, peppe.cavallaro
In-Reply-To: <1339318088-27126-1-git-send-email-yuvalmin@broadcom.com>
On Sun, 2012-06-10 at 11:48 +0300, Yuval Mintz wrote:
> This patch adds 2 new ethtool commands which can be
> used to manipulate network interfaces' support in
> EEE.
>
> Output of 'get' has the following form:
>
> EEE Settings for p2p1:
> EEE status: enabled - active
> Tx LPI: 1000 (us)
> Supported EEE link modes: 10000baseT/Full
> Advertised EEE link modes: 10000baseT/Full
> Link partner advertised EEE link modes: 10000baseT/Full
>
> Thanks goes to Giuseppe Cavallaro for his original patch.
>
> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
[...]
Applied, thanks.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
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 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function
From: Takuya Yoshikawa @ 2012-06-13 22:28 UTC (permalink / raw)
To: Grant Grundler
Cc: Akinobu Mita, Takuya Yoshikawa, akpm, bhutchings, grundler, arnd,
benh, avi, mtosatti, linux-net-drivers, netdev, linux-kernel,
linux-arch, kvm
In-Reply-To: <CAP6odji_JvRkJP1dSzGK5ww3y71h4k6pXyj4pOQBpqP-X7Nn-A@mail.gmail.com>
On Wed, 13 Jun 2012 08:21:18 -0700
Grant Grundler <grantgrundler@gmail.com> wrote:
> >> >> Should this hash_table be converted from u16 hash_table[32] to
> >> >> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
> >> >> on long-word boundary?
> >> >
> >> > I think hash_table is already long-word aligned because it is placed
> >> > right after a pointer.
> >>
> >> I recommend converting to proper bitmap. Because such an implicit
> >> assumption is easily broken by someone touching this function.
> >
> > Do you mean something like:
> > DECLARE_BITMAP(__hash_table, 16 * 32);
> > u16 *hash_table = (u16 *)__hash_table;
> > ?
> >
> > Grant, what do you think about this?
>
> Hi Takuya,
> two thoughts:
> 1) while I agree with Akinobu and thank him for pointing out a
> _potential_ alignment problem, this is a separate issue and your
> existing patch should go in anyway. There are probably other drivers
> with _potential_ alignment issues. Akinobu could get credit for
> finding them by submitting patches after reviewing calls to set_bit
> and set_bit_le() - similar to what you are doing now.
I prefer approach 1.
hash_table is local in build_setup_frame_hash(), so if further
improvement is also required, we can do that locally there later.
Thanks,
Takuya
> 2) I generally do not like declaring one type and then using an alias
> of a different type to reference the same memory address. We have a
> simple alternative since hash_table[] is indexed directly only in one
> hunk of code:
> for (i = 0; i < 32; i++) {
> *setup_frm++ = ((u16 *)hash_table)[i];
> *setup_frm++ = ((u16 *)hash_table)[i];
> }
^ permalink raw reply
* Re: [net-next patch 8/12] bnx2x: Allow up to 63 RSS queues default 8 queues
From: David Miller @ 2012-06-13 22:35 UTC (permalink / raw)
To: eilong; +Cc: eric.dumazet, meravs, netdev
In-Reply-To: <1339595609.13000.15.camel@lb-tlvb-eilong.il.broadcom.com>
From: "Eilon Greenstein" <eilong@broadcom.com>
Date: Wed, 13 Jun 2012 16:53:29 +0300
> Just to emphasis, since this is the patch series that enable the users
> to control the number of queues, we can reduce the default number and
> allow the user to increase it if he has a setup that needs more than 8
> parallel CPUs to receive the traffic. When using a new FW on the board,
> the number can be increased up to 64, so using the maximal number can be
> an overkill (even if the machine has 64 CPUs, it does not mean that the
> user would like us to consume 64 MSI-X vectors and all the memory to set
> up 64 queues) - so a lower default value can be used to satisfy most
> users while allowing them to increase the number if they wish.
I think you should look at other drivers for guidance in this area.
There is zero value in each and every driver author deciding what
is a good default strategy, because this means the user gets a
very inconsistent experience based purely upon the driver author's
whims.
^ permalink raw reply
* Re: [PATCH 0/8] dcbnl: Major simplifications
From: David Miller @ 2012-06-13 22:55 UTC (permalink / raw)
To: tgraf; +Cc: netdev, lucy.liu, john.r.fastabend, alexander.h.duyck
In-Reply-To: <cover.1339591572.git.tgraf@suug.ch>
From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 13 Jun 2012 14:54:53 +0200
> The dcbnl code is overly complicated when constructing the reply
> message. This series of patches simplifies the process and
> introduces consistent error codes allowing user space to figure
> out what went wrong.
>
> Patches have been tested with ixgbe in a FCoE setup.
>
> Thomas Graf (8):
> dcbnl: Prepare framework to shorten handling functions
> dcbnl: Shorten all command handling functions
> dcbnl: Remove now unused dcbnl_reply()
> dcbnl: Use dcbnl_newmsg() where possible
> dcbnl: Return consistent error codes
> dcbnl: Move dcb app lookup code into dcb_app_lookup()
> dcbnl: Move dcb app allocation into dcb_app_add()
> dcbnl: Use type safe nlmsg_data()
Lots of deleted code, I like it :-)
Applied, but could you send a follow-on patch to use BUG_ON() instead
of that "if (!ptr) { /* ... */ BUG(); }" construct?
Thanks.
^ permalink raw reply
* Re: [PATCH] netpoll: fix netpoll_send_udp() bugs
From: David Miller @ 2012-06-13 22:58 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, bogdan.hamciuc, herbert, nhorman
In-Reply-To: <1339565421.22704.310.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 13 Jun 2012 07:30:21 +0200
> From: Eric Dumazet <edumazet@google.com>
>
> Bogdan Hamciuc diagnosed and fixed following bug in netpoll_send_udp() :
>
> "skb->len += len;" instead of "skb_put(skb, len);"
>
> Meaning that _if_ a network driver needs to call skb_realloc_headroom(),
> only packet headers would be copied, leaving garbage in the payload.
>
> However the skb_realloc_headroom() must be avoided as much as possible
> since it requires memory and netpoll tries hard to work even if memory
> is exhausted (using a pool of preallocated skbs)
>
> It appears netpoll_send_udp() reserved 16 bytes for the ethernet header,
> which happens to work for typicall drivers but not all.
>
> Right thing is to use LL_RESERVED_SPACE(dev)
> (And also add dev->needed_tailroom of tailroom)
>
> This patch combines both fixes.
>
> Many thanks to Bogdan for raising this issue.
>
> Reported-by: Bogdan Hamciuc <bogdan.hamciuc@freescale.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Tested-by: Bogdan Hamciuc <bogdan.hamciuc@freescale.com>
Applied and queued up for -stable, thanks Eric.
^ permalink raw reply
* Re: [PATCH] bnx2x: fix checksum validation
From: David Miller @ 2012-06-13 22:59 UTC (permalink / raw)
To: eilong
Cc: eric.dumazet, netdev, therbert, evansr, willemb, yanivr, meravs,
yuvalmin
In-Reply-To: <1339597263.15601.0.camel@lb-tlvb-eilong.il.broadcom.com>
From: "Eilon Greenstein" <eilong@broadcom.com>
Date: Wed, 13 Jun 2012 17:21:03 +0300
> Thanks Erik. This leaves me with just one thing to add:
> Acked-by: Eilon Greenstein <eilong@broadcom.com>
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