* 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: 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: 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: [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: [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: 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: 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 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: 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 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 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
* [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: [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
* 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: [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] bnx2x: fix checksum validation
From: Eilon Greenstein @ 2012-06-13 14:21 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Tom Herbert, Robert Evans, Willem de Bruijn,
Yaniv Rosner, Merav Sicron, Yuval Mintz
In-Reply-To: <1339596605.22704.363.camel@edumazet-glaptop>
On Wed, 2012-06-13 at 16:10 +0200, Eric Dumazet wrote:
> On Wed, 2012-06-13 at 16:42 +0300, Eilon Greenstein wrote:
>
> > So what you are saying is that the indication that the checksum is valid
> > is interpreted as the encapsulated checksum and not just the IP
> > header... This was not the intention of this code, it was meant to
> > indicate that the IP header is valid.
> >
>
> IP header is always checked by our stack.
>
> Its so fast that there is no point spending a bit in skb, and testing
> this bit.
>
> Yes, CHECKSUM_UNNECESSARY really means everything was checked up to L4
>
> > I must admit that the code looks much better with this change. The only
> > down side is that there is no longer IP checksum offload for pure IP
> > packets, but that's negligible. The only thing that bothers me is that
> > there is no way to indicate anything about the encapsulated packet
> > separately from the outer header. Is that the way we want to keep it?
>
> Some NIC allow for csum L3 offloading, providing the 16bit csum.
>
> We then can :
>
> skb->csum = csum;
> skb->ip_summed = CHECKSUM_COMPLETE;
>
> Check drivers/net/ethernet/ibm/ehea/ehea_main.c,
> drivers/net/ethernet/marvell/skge.c ,
> drivers/net/ethernet/marvell/sky2.c , ... for examples.
>
>
> When a layer is removed(pulled), we can use skb_postpull_rcsum() and
> friends (check net/ipv4/ip_gre.c for example) to keep skb->csum up2date.
>
>
> If bnx2x provides this csum (for non TCP/UCP packets or fragments), you
> could fallback to this CHECKSUM_COMPLETE stuff.
>
>
Thanks Erik. This leaves me with just one thing to add:
Acked-by: Eilon Greenstein <eilong@broadcom.com>
^ permalink raw reply
* Re: [PATCH] bnx2x: fix checksum validation
From: Eric Dumazet @ 2012-06-13 14:10 UTC (permalink / raw)
To: eilong
Cc: David Miller, netdev, Tom Herbert, Robert Evans, Willem de Bruijn,
Yaniv Rosner, Merav Sicron, Yuval Mintz
In-Reply-To: <1339594957.13000.7.camel@lb-tlvb-eilong.il.broadcom.com>
On Wed, 2012-06-13 at 16:42 +0300, Eilon Greenstein wrote:
> So what you are saying is that the indication that the checksum is valid
> is interpreted as the encapsulated checksum and not just the IP
> header... This was not the intention of this code, it was meant to
> indicate that the IP header is valid.
>
IP header is always checked by our stack.
Its so fast that there is no point spending a bit in skb, and testing
this bit.
Yes, CHECKSUM_UNNECESSARY really means everything was checked up to L4
> I must admit that the code looks much better with this change. The only
> down side is that there is no longer IP checksum offload for pure IP
> packets, but that's negligible. The only thing that bothers me is that
> there is no way to indicate anything about the encapsulated packet
> separately from the outer header. Is that the way we want to keep it?
Some NIC allow for csum L3 offloading, providing the 16bit csum.
We then can :
skb->csum = csum;
skb->ip_summed = CHECKSUM_COMPLETE;
Check drivers/net/ethernet/ibm/ehea/ehea_main.c,
drivers/net/ethernet/marvell/skge.c ,
drivers/net/ethernet/marvell/sky2.c , ... for examples.
When a layer is removed(pulled), we can use skb_postpull_rcsum() and
friends (check net/ipv4/ip_gre.c for example) to keep skb->csum up2date.
If bnx2x provides this csum (for non TCP/UCP packets or fragments), you
could fallback to this CHECKSUM_COMPLETE stuff.
^ permalink raw reply
* Re: [net-next patch 8/12] bnx2x: Allow up to 63 RSS queues default 8 queues
From: Eilon Greenstein @ 2012-06-13 13:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Merav Sicron, davem, netdev
In-Reply-To: <1339600460.17240.69.camel@lb-tlvb-meravs.il.broadcom.com>
On Wed, 2012-06-13 at 18:14 +0300, Merav Sicron wrote:
> Hi Eric,
>
> On Wed, 2012-06-13 at 11:52 +0200, Eric Dumazet wrote:
> > On Wed, 2012-06-13 at 15:44 +0300, Merav Sicron wrote:
> > > This patch removed the limitation in the code for 16 RSS queues. The default
> > > (without other instruction from the user) number of queues is determined
> > > according to the number of MSI-X vectors supported for that function, the number
> > > of CPUs in the system, and a maximum of 8 queues.
> >
> > Thats a very confusing changelog
> >
> > You meant : " a minimum of 8 queues" ?
> >
> No, I meant maximum...for example in a system with 16 CPUs we will
> allocate 8 RSS queues. We saw that in most scenarios we tested there was
> no need for more than 8 queues to get the maximal throughput, and by
> limiting to 8 by default we reduce the allocated memory. We do provide
> ethtool -L support so that the user could request more RSS queues if
> desired.
>
> > You should give more explanations, because its a sensible area for
> > performances.
> >
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.
Thanks,
Eilon
^ permalink raw reply
* Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function
From: Takuya Yoshikawa @ 2012-06-13 14:00 UTC (permalink / raw)
To: Akinobu Mita
Cc: Takuya Yoshikawa, akpm, bhutchings, grundler, arnd, benh, avi,
mtosatti, linux-net-drivers, netdev, linux-kernel, linux-arch,
kvm
In-Reply-To: <CAC5umygutE35hVObuooWj=ADP6PZDy4cqLtxGnRtxRX8uTtGSw@mail.gmail.com>
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?
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
* Re: [PATCH] netpoll: fix netpoll_send_udp() bugs
From: Cong Wang @ 2012-06-13 13:59 UTC (permalink / raw)
To: netdev
In-Reply-To: <1339565421.22704.310.camel@edumazet-glaptop>
On Wed, 13 Jun 2012 at 05:30 GMT, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 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.
>
Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>
Need to apply to -stable?
Thanks.
^ permalink raw reply
* Re: [net-next patch 8/12] bnx2x: Allow up to 63 RSS queues default 8 queues
From: Eric Dumazet @ 2012-06-13 13:59 UTC (permalink / raw)
To: eilong; +Cc: Merav Sicron, davem, netdev
In-Reply-To: <1339595609.13000.15.camel@lb-tlvb-eilong.il.broadcom.com>
On Wed, 2012-06-13 at 16:53 +0300, Eilon Greenstein wrote:
> On Wed, 2012-06-13 at 18:14 +0300, Merav Sicron wrote:
> > Hi Eric,
> >
> > On Wed, 2012-06-13 at 11:52 +0200, Eric Dumazet wrote:
> > > On Wed, 2012-06-13 at 15:44 +0300, Merav Sicron wrote:
> > > > This patch removed the limitation in the code for 16 RSS queues. The default
> > > > (without other instruction from the user) number of queues is determined
> > > > according to the number of MSI-X vectors supported for that function, the number
> > > > of CPUs in the system, and a maximum of 8 queues.
> > >
> > > Thats a very confusing changelog
> > >
> > > You meant : " a minimum of 8 queues" ?
> > >
> > No, I meant maximum...for example in a system with 16 CPUs we will
> > allocate 8 RSS queues. We saw that in most scenarios we tested there was
> > no need for more than 8 queues to get the maximal throughput, and by
> > limiting to 8 by default we reduce the allocated memory. We do provide
> > ethtool -L support so that the user could request more RSS queues if
> > desired.
> >
> > > You should give more explanations, because its a sensible area for
> > > performances.
> > >
>
> 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 am all for a reduction of default number of queues.
Even a laptop has now 8 cpus, so on servers, using one queue per cpu is
overkill for most uses.
Thanks
^ permalink raw reply
* Re: PPPoE performance regression
From: David Woodhouse @ 2012-06-13 13:50 UTC (permalink / raw)
To: Nathan Williams
Cc: Karl Hiramoto, David S. Miller, netdev, Paul Mackerras,
John Crispin
In-Reply-To: <1339581421.11011.18.camel@shinybook.infradead.org>
[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]
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...
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index cbf7047..ddaf156 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -689,6 +689,8 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
sk->sk_state = PPPOX_CONNECTED;
}
+ atomic_set(&po->inflight, -2);
+
po->num = sp->sa_addr.pppoe.sid;
end:
@@ -952,9 +954,34 @@ abort:
* sends PPP frame over PPPoE socket
*
***********************************************************************/
+static void pppoe_skb_destructor(struct sk_buff *skb)
+{
+ struct sock *sk = skb->sk;
+ struct pppox_sock *po = pppox_sk(sk);
+
+ atomic_dec(&po->inflight);
+ /* Schedule a call to ppp_output_wakeup(chan), if it was already blocked.
+ Mind for race conditions with another CPU which is in pppoe_xmit()
+ right now. See commit 9d02daf7 in pppoatm. */
+ sock_put(sk);
+}
+
static int pppoe_xmit(struct ppp_channel *chan, struct sk_buff *skb)
{
struct sock *sk = (struct sock *)chan->private;
+ struct pppox_sock *po = pppox_sk(sk);
+
+ if (!atomic_inc_not_zero(&po->inflight))
+ return 0;
+
+ /* mine! all mine! */
+ skb_orphan(skb);
+ skb->destructor = pppoe_skb_destructor;
+ /* XXX: Are there other implications of setting this? Should we use ->cb? */
+ skb->sk = sk;
+
+ sock_hold(sk);
+
return __pppoe_xmit(sk, skb);
}
diff --git a/include/linux/if_pppox.h b/include/linux/if_pppox.h
index 09c474c..339c75d 100644
--- a/include/linux/if_pppox.h
+++ b/include/linux/if_pppox.h
@@ -186,6 +186,7 @@ struct pppox_sock {
/* struct sock must be the first member of pppox_sock */
struct sock sk;
struct ppp_channel chan;
+ atomic_t inflight;
struct pppox_sock *next; /* for hash table */
union {
struct pppoe_opt pppoe;
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]
^ permalink raw reply related
* Re: [PATCH] bnx2x: fix checksum validation
From: Eilon Greenstein @ 2012-06-13 13:42 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Tom Herbert, Robert Evans, Willem de Bruijn,
Yaniv Rosner, Merav Sicron, Yuval Mintz
In-Reply-To: <1339581004.22704.340.camel@edumazet-glaptop>
On Wed, 2012-06-13 at 11:50 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> bnx2x driver incorrectly sets ip_summed to CHECKSUM_UNNECESSARY on
> encapsulated segments. TCP stack happily accepts frames with bad
> checksums, if they are inside a GRE or IPIP encapsulation.
So what you are saying is that the indication that the checksum is valid
is interpreted as the encapsulated checksum and not just the IP
header... This was not the intention of this code, it was meant to
indicate that the IP header is valid.
> Our understanding is that if no IP or L4 csum validation was done by the
> hardware, we should leave ip_summed as is (CHECKSUM_NONE), since
> hardware doesn't provide CHECKSUM_COMPLETE support in its cqe.
>
> Then, if IP/L4 checksumming was done by the hardware, set
> CHECKSUM_UNNECESSARY if no error was flagged.
>
> Patch based on findings and analysis from Robert Evans
I must admit that the code looks much better with this change. The only
down side is that there is no longer IP checksum offload for pure IP
packets, but that's negligible. The only thing that bothers me is that
there is no way to indicate anything about the encapsulated packet
separately from the outer header. Is that the way we want to keep it?
> 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.h | 15 -------
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 ++++++++++----
> 2 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> index e30e2a2..7de8241 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> @@ -747,21 +747,6 @@ struct bnx2x_fastpath {
>
> #define ETH_RX_ERROR_FALGS ETH_FAST_PATH_RX_CQE_PHY_DECODE_ERR_FLG
>
> -#define BNX2X_IP_CSUM_ERR(cqe) \
> - (!((cqe)->fast_path_cqe.status_flags & \
> - ETH_FAST_PATH_RX_CQE_IP_XSUM_NO_VALIDATION_FLG) && \
> - ((cqe)->fast_path_cqe.type_error_flags & \
> - ETH_FAST_PATH_RX_CQE_IP_BAD_XSUM_FLG))
> -
> -#define BNX2X_L4_CSUM_ERR(cqe) \
> - (!((cqe)->fast_path_cqe.status_flags & \
> - ETH_FAST_PATH_RX_CQE_L4_XSUM_NO_VALIDATION_FLG) && \
> - ((cqe)->fast_path_cqe.type_error_flags & \
> - ETH_FAST_PATH_RX_CQE_L4_BAD_XSUM_FLG))
> -
> -#define BNX2X_RX_CSUM_OK(cqe) \
> - (!(BNX2X_L4_CSUM_ERR(cqe) || BNX2X_IP_CSUM_ERR(cqe)))
> -
> #define BNX2X_PRS_FLAG_OVERETH_IPV4(flags) \
> (((le16_to_cpu(flags) & \
> PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) >> \
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> index ad0743b..cbc56f2 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -617,6 +617,25 @@ static int bnx2x_alloc_rx_data(struct bnx2x *bp,
> return 0;
> }
>
> +static void bnx2x_csum_validate(struct sk_buff *skb, union eth_rx_cqe *cqe,
> + struct bnx2x_fastpath *fp)
> +{
> + /* Do nothing if no IP/L4 csum validation was done */
> +
> + if (cqe->fast_path_cqe.status_flags &
> + (ETH_FAST_PATH_RX_CQE_IP_XSUM_NO_VALIDATION_FLG |
> + ETH_FAST_PATH_RX_CQE_L4_XSUM_NO_VALIDATION_FLG))
> + return;
> +
> + /* If both IP/L4 validation were done, check if an error was found. */
> +
> + if (cqe->fast_path_cqe.type_error_flags &
> + (ETH_FAST_PATH_RX_CQE_IP_BAD_XSUM_FLG |
> + ETH_FAST_PATH_RX_CQE_L4_BAD_XSUM_FLG))
> + fp->eth_q_stats.hw_csum_err++;
> + else
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> +}
>
> int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
> {
> @@ -806,13 +825,9 @@ reuse_rx:
>
> skb_checksum_none_assert(skb);
>
> - if (bp->dev->features & NETIF_F_RXCSUM) {
> + if (bp->dev->features & NETIF_F_RXCSUM)
> + bnx2x_csum_validate(skb, cqe, fp);
>
> - if (likely(BNX2X_RX_CSUM_OK(cqe)))
> - skb->ip_summed = CHECKSUM_UNNECESSARY;
> - else
> - fp->eth_q_stats.hw_csum_err++;
> - }
>
> skb_record_rx_queue(skb, fp->rx_queue);
>
>
>
>
^ permalink raw reply
* Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function
From: Akinobu Mita @ 2012-06-13 13:31 UTC (permalink / raw)
To: Takuya Yoshikawa
Cc: Takuya Yoshikawa, akpm, bhutchings, grundler, arnd, benh, avi,
mtosatti, linux-net-drivers, netdev, linux-kernel, linux-arch,
kvm
In-Reply-To: <20120613214157.0a5179d5358ec7f1b2646606@gmail.com>
2012/6/13 Takuya Yoshikawa <takuya.yoshikawa@gmail.com>:
> On Wed, 13 Jun 2012 18:43:40 +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.
^ permalink raw reply
* [PATCH 8/8] dcbnl: Use type safe nlmsg_data()
From: Thomas Graf @ 2012-06-13 12:55 UTC (permalink / raw)
To: davem; +Cc: netdev, lucy.liu, john.r.fastabend, alexander.h.duyck
In-Reply-To: <cover.1339591572.git.tgraf@suug.ch>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
net/dcb/dcbnl.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index 6e1c324..70bba3e 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -1659,7 +1659,7 @@ static int dcb_doit(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
{
struct net *net = sock_net(skb->sk);
struct net_device *netdev;
- struct dcbmsg *dcb = (struct dcbmsg *)NLMSG_DATA(nlh);
+ struct dcbmsg *dcb = nlmsg_data(nlh);
struct nlattr *tb[DCB_ATTR_MAX + 1];
u32 pid = skb ? NETLINK_CB(skb).pid : 0;
int ret = -EINVAL;
--
1.7.7.6
^ 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