* Re: [PATCH resend] can: c_can: Fix tx_bytes accounting
From: Kurt Van Dijck @ 2011-03-24 10:49 UTC (permalink / raw)
To: Jan Altenberg
Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Wolfgang Grandegger,
b.spranger-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org
In-Reply-To: <1300966010.3295.40.camel@localhost>
On Thu, Mar 24, 2011 at 12:26:50PM +0100, Jan Altenberg wrote:
> The current SocketCAN implementation for the Bosch c_can cell doesn't
> account the TX bytes correctly, because it calls
> c_can_inval_msg_object() (which clears the msg ctrl register) before
> reading the DLC value:
>
> The fix is quite easy: Just move c_can_inval_msg_object() to the end of
> the if() statement. So:
> * We only call c_can_inval_msg_object() if the message was
> actually transmitted
> * We read out the DLC value _before_ clearing the msg ctrl
> register
>
> Signed-off-by: Jan Altenberg <jan-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Acked-by: Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>
I cut the description a bit, to what I would have found sufficient.
^ permalink raw reply
* Re: [RFC] usbnet: use eth%d name for known ethernet devices
From: Andy Green @ 2011-03-24 10:45 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Greg KH, Alan Cox, Benjamin Herrenschmidt, Nicolas Pitre,
Jaswinder Singh, Linux USB list, lkml,
broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E,
roger.quadros-xNZwKgViW5gAvxtiuMwx3w,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
netdev-u79uwXL29TY76Z2rM5mHXA, David Brownell
In-Reply-To: <201103231811.54290.arnd-r2nGTMty4D4@public.gmane.org>
On 03/23/2011 05:11 PM, Somebody in the thread at some point said:
> On Wednesday 23 March 2011, Andy Green wrote:
>>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>>> index bc86f4b..c98d3a7 100644
>>> --- a/drivers/net/usb/smsc95xx.c
>>> +++ b/drivers/net/usb/smsc95xx.c
>>> @@ -1231,7 +1231,7 @@ static const struct driver_info smsc95xx_info = {
>>
>>> - .flags = FLAG_ETHER | FLAG_SEND_ZLP,
>>> + .flags = FLAG_ETHER | FLAG_SEND_ZLP | FLAG_REALLY_ETHER,
>>
>>> if ((dev->driver_info->flags& FLAG_ETHER) != 0&&
>>> + ((dev->driver_info->flags& FLAG_PTP) == 0 ||
>>> + (net->dev_addr [0]& 0x02) == 0))
>>> strcpy (net->name, "eth%d");
>>
>> So it just takes the approach that all smsc95xx are going to be eth%d?
>
> Right, and all other drivers that are obviously ethernet-only, including
> future drivers.
I moved my tree over to using this patch now, it's working fine on Panda
/ smsc95xx as one would expect. Guys with pluggable smsc95xx are going
to find their device comes as eth%d not usb%d but that seems perfectly
defensible as how it always should have been.
So, nice job.
-Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: RFC: [PATCH] can: c_can: disable one shot mode until driver is fixed
From: Marc Kleine-Budde @ 2011-03-24 10:44 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA,
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w
In-Reply-To: <20110324102852.GA339-MxZ6Iy/zr/UdbCeoMzGj59i2O/JbrIOy@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1823 bytes --]
On 03/24/2011 11:28 AM, Kurt Van Dijck wrote:
> On Thu, Mar 24, 2011 at 11:12:29AM +0100, Marc Kleine-Budde wrote:
>> This patch disables the one shot mode, until the driver has been fixed and
>> tested to support it.
>>
> isn't this part necessary now (temporarily) to avoid dead code?
It's not necessary, the driver should still work. But it's dead code. If
someone fixes the driver she/he can easily revert the patch.
Can I fold your patch and add your S-o-b?
Marc
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 1405078..2c3d090 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -588,14 +588,9 @@ static void c_can_chip_config(struct net_device *dev)
> {
> struct c_can_priv *priv = netdev_priv(dev);
>
> - if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> - /* disable automatic retransmission */
> - priv->write_reg(priv, &priv->regs->control,
> - CONTROL_DISABLE_AR);
> - else
> - /* enable automatic retransmission */
> - priv->write_reg(priv, &priv->regs->control,
> - CONTROL_ENABLE_AR);
> + /* enable automatic retransmission */
> + priv->write_reg(priv, &priv->regs->control,
> + CONTROL_ENABLE_AR);
>
> if (priv->can.ctrlmode & (CAN_CTRLMODE_LISTENONLY &
> CAN_CTRLMODE_LOOPBACK)) {
>
> _______________________________________________
> Socketcan-core mailing list
> Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> https://lists.berlios.de/mailman/listinfo/socketcan-core
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
[-- Attachment #2: Type: text/plain, Size: 188 bytes --]
_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core
^ permalink raw reply
* [PATCH resend] can: c_can: Fix tx_bytes accounting
From: Jan Altenberg @ 2011-03-24 11:26 UTC (permalink / raw)
To: Wolfgang Grandegger
Cc: Bhupesh SHARMA, kurt.van.dijck@eia.be, b.spranger@linutronix.de,
netdev@vger.kernel.org, Socketcan-core@lists.berlios.de
In-Reply-To: <4D8B1641.6000706@grandegger.com>
The current SocketCAN implementation for the Bosch c_can cell doesn't
account the TX bytes correctly, because it calls
c_can_inval_msg_object() (which clears the msg ctrl register) before
reading the DLC value:
for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
msg_obj_no = get_tx_echo_msg_obj(priv);
c_can_inval_msg_object(dev, 0, msg_obj_no);
val = c_can_read_reg32(priv, &priv->regs->txrqst1);
if (!(val & (1 << msg_obj_no))) {
can_get_echo_skb(dev,
msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
stats->tx_bytes += priv->read_reg(priv,
&priv->regs->ifregs[0].msg_cntrl)
& IF_MCONT_DLC_MASK;
stats->tx_packets++;
}
}
So, we will always read 0 for the DLC value and "ifconfig" will report
*0* TX Bytes.
The fix is quite easy: Just move c_can_inval_msg_object() to the end of
the if() statement. So:
* We only call c_can_inval_msg_object() if the message was
actually transmitted
* We read out the DLC value _before_ clearing the msg ctrl
register
Signed-off-by: Jan Altenberg <jan@linutronix.de>
---
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 110eda0..f895c04 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -704,7 +704,6 @@ static void c_can_do_tx(struct net_device *dev)
for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
msg_obj_no = get_tx_echo_msg_obj(priv);
- c_can_inval_msg_object(dev, 0, msg_obj_no);
val = c_can_read_reg32(priv, &priv->regs->txrqst1);
if (!(val & (1 << msg_obj_no))) {
can_get_echo_skb(dev,
@@ -713,6 +712,7 @@ static void c_can_do_tx(struct net_device *dev)
&priv->regs->ifregs[0].msg_cntrl)
& IF_MCONT_DLC_MASK;
stats->tx_packets++;
+ c_can_inval_msg_object(dev, 0, msg_obj_no);
}
}
^ permalink raw reply related
* Re: RFC: [PATCH] can: c_can: disable one shot mode until driver is fixed
From: Kurt Van Dijck @ 2011-03-24 10:28 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1300961549-28882-1-git-send-email-mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On Thu, Mar 24, 2011 at 11:12:29AM +0100, Marc Kleine-Budde wrote:
> This patch disables the one shot mode, until the driver has been fixed and
> tested to support it.
>
isn't this part necessary now (temporarily) to avoid dead code?
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 1405078..2c3d090 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -588,14 +588,9 @@ static void c_can_chip_config(struct net_device *dev)
{
struct c_can_priv *priv = netdev_priv(dev);
- if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
- /* disable automatic retransmission */
- priv->write_reg(priv, &priv->regs->control,
- CONTROL_DISABLE_AR);
- else
- /* enable automatic retransmission */
- priv->write_reg(priv, &priv->regs->control,
- CONTROL_ENABLE_AR);
+ /* enable automatic retransmission */
+ priv->write_reg(priv, &priv->regs->control,
+ CONTROL_ENABLE_AR);
if (priv->can.ctrlmode & (CAN_CTRLMODE_LISTENONLY &
CAN_CTRLMODE_LOOPBACK)) {
^ permalink raw reply related
* RFC: [PATCH] can: c_can: disable one shot mode until driver is fixed
From: Marc Kleine-Budde @ 2011-03-24 10:12 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA
Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w, Marc Kleine-Budde,
jan-hfZtesqFncYOwBW4kG4KsQ
This patch disables the one shot mode, until the driver has been fixed and
tested to support it.
> I'm quite sure I've seen a situation where msg_obj 17 "seemed" to be
> pending, while msg_obj 18 and 19 already have been transmitted. But
> in that case, I enabled ONESHOT for the can interface, which enables
> the DA mode (automatic retransmission is disabled).
Reported-by: Jan Altenberg <jan-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Bhupesh Sharma <bhupesh.sharma-qxv4g6HH51o@public.gmane.org>
---
drivers/net/can/c_can/c_can.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 110eda0..2040959 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -1112,8 +1112,7 @@ struct net_device *alloc_c_can_dev(void)
priv->can.bittiming_const = &c_can_bittiming_const;
priv->can.do_set_mode = c_can_set_mode;
priv->can.do_get_berr_counter = c_can_get_berr_counter;
- priv->can.ctrlmode_supported = CAN_CTRLMODE_ONE_SHOT |
- CAN_CTRLMODE_LOOPBACK |
+ priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
CAN_CTRLMODE_LISTENONLY |
CAN_CTRLMODE_BERR_REPORTING;
--
1.7.2.3
^ permalink raw reply related
* RE: c_can: TX handling
From: Jan Altenberg @ 2011-03-24 10:56 UTC (permalink / raw)
To: Bhupesh SHARMA
Cc: wg@grandegger.com, kurt.van.dijck@eia.be,
b.spranger@linutronix.de, netdev@vger.kernel.org, jan
In-Reply-To: <D5ECB3C7A6F99444980976A8C6D896384DF0D6F684@EAPEX1MAIL1.st.com>
Hi,
> Thanks for your work on c_can.
> This week has been very busy for me, so please excuse me for the late reply.
No problem.
> Just to better understand this can you send me your *candump* output
> when you see that the message object put earlier in the message RAM
> being pending on the line.
>
> Your point seems valid but I want to make sure that this is not done by the
> C_CAN core implicitly..
I'll try to prepare a (stripped down) test-case, but since I'm quite
busy at the moment, that might take a couple of days.
I'm quite sure, I've seen such a situation, but first of all, I'd like
to sort out, if that was related to the "DA mode".
Cheers,
Jan
^ permalink raw reply
* Re: can: c_can: TX delivery
From: Marc Kleine-Budde @ 2011-03-24 10:02 UTC (permalink / raw)
To: Jan Altenberg; +Cc: Kurt Van Dijck, bhupesh.sharma, wg, b.spranger, netdev
In-Reply-To: <ff5ade24e148af2e46832e5a64444d05.squirrel@www2.linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2538 bytes --]
On 03/23/2011 04:32 PM, Jan Altenberg wrote:
> Hi,
>
>> I split your 2 questions in 2 replies.
>
> Thanks :)
>
>> not sure if I made my point. Note that this will eliminate the need
>> for explicit wrap-around. It's done implicitely.
>
> Hmmm, I double-checked the datasheet, which gives the following statement:
> "The receive/transmit priority for the Message Objects is attached to
> the message number. Message Object 1 has the highest priority, while
> Message Object 32 has the lowest priority. If more than one
> transmission request is pending, they are serviced due to the priority
> of the corresponding Message Object."
>
> So, we shouldn't run into the scenario I described in my previous mail
ACK - the tx implementation is borrowed from the at91 driver. There we
have a prio field per mailbox (which is _not_ the CAN id), and the
mailbox number itself. If two mailboxes have the same prio the mailbox
with the lower number is send first. So we start with the highest prio
in mailbox 0, until the last mb, then decrease the prio and start with
mb 0. Special care is taken in prio wrap around and all mailboxes full.
Please recheck if this is implemented on the c_can driver correctly.
> and the existing implementation should be OK, right?! I'm quite sure
> I've seen a situation where msg_obj 17 "seemed" to be pending, while
> msg_obj 18 and 19 already have been transmitted. But in that case, I
> enabled ONESHOT for the can interface, which enables the DA mode
> (automatic
> retransmission is disabled). The errata sheet for c_can covers that
Oh...Oneshot means if TX fails (due to higher prio frame on the bus) it
won't be tx'ed again. IIRC in the at91 there's a bit signalling that TX
has been aborted due to oneshot. Maybe an interrupt can be enabled, too.
Hmm....the driver advertised it supports oneshot mode. Has this been
tested? If oneshot isn't working we should disable it in the driver
until it has been fixed.
> mode. There's a problem with "Concurrent transmission requests" and I'm
> quite sure my test-case hit that problem.
>
> I'm quite new to Bosch's c_can, so maybe Bhupesh can give some feedback
> (or beat me for causing some confusion ;-)).
cheers, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: [PATCH] can: c_can: Fix tx_bytes accounting
From: Wolfgang Grandegger @ 2011-03-24 10:00 UTC (permalink / raw)
To: Jan Altenberg
Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
b.spranger-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org
In-Reply-To: <1300963120.3295.7.camel@localhost>
Hi Jan,
On 03/24/2011 11:38 AM, Jan Altenberg wrote:
> Hi Bhupesh,
>
> as discussed I moved c_can_inval_msg_object() to the end of the if()
> statement. That should fix the tx_bytes accounting. For me it's working
> fine now.
>
> Signed-off-by: Jan Altenberg <jan-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
Could you please provide a proper commit message and put other comments
below "---".
Thanks,
Wolfgang.
^ permalink raw reply
* [PATCH] can: c_can: Fix tx_bytes accounting
From: Jan Altenberg @ 2011-03-24 10:38 UTC (permalink / raw)
To: Bhupesh SHARMA
Cc: wg@grandegger.com, kurt.van.dijck@eia.be,
b.spranger@linutronix.de, netdev@vger.kernel.org,
Socketcan-core@lists.berlios.de, jan
In-Reply-To: <D5ECB3C7A6F99444980976A8C6D896384DF0D6F721@EAPEX1MAIL1.st.com>
Hi Bhupesh,
as discussed I moved c_can_inval_msg_object() to the end of the if()
statement. That should fix the tx_bytes accounting. For me it's working
fine now.
Signed-off-by: Jan Altenberg <jan@linutronix.de>
---
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 110eda0..f895c04 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -704,7 +704,6 @@ static void c_can_do_tx(struct net_device *dev)
for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
msg_obj_no = get_tx_echo_msg_obj(priv);
- c_can_inval_msg_object(dev, 0, msg_obj_no);
val = c_can_read_reg32(priv, &priv->regs->txrqst1);
if (!(val & (1 << msg_obj_no))) {
can_get_echo_skb(dev,
@@ -713,6 +712,7 @@ static void c_can_do_tx(struct net_device *dev)
&priv->regs->ifregs[0].msg_cntrl)
& IF_MCONT_DLC_MASK;
stats->tx_packets++;
+ c_can_inval_msg_object(dev, 0, msg_obj_no);
}
}
^ permalink raw reply related
* [PATCH] mlx4_en: Removing HW info from ethtool -i report.
From: Yevgeny Petrilin @ 2011-03-24 8:36 UTC (permalink / raw)
To: davem; +Cc: netdev, yevgenyp
Avoiding abuse of ethtool_drvinfo.driver field.
HW specific info can be retrieved using lspci.
Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
---
drivers/net/mlx4/en_ethtool.c | 15 +--------------
1 files changed, 1 insertions(+), 14 deletions(-)
diff --git a/drivers/net/mlx4/en_ethtool.c b/drivers/net/mlx4/en_ethtool.c
index 62ace6c..d54b7ab 100644
--- a/drivers/net/mlx4/en_ethtool.c
+++ b/drivers/net/mlx4/en_ethtool.c
@@ -45,20 +45,7 @@ mlx4_en_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *drvinfo)
struct mlx4_en_priv *priv = netdev_priv(dev);
struct mlx4_en_dev *mdev = priv->mdev;
- switch (mdev->dev->rev_id) {
- case 0xa0:
- if (dev->dev_id >= MLX4_EN_CX3_LOW_ID && dev->dev_id <= MLX4_EN_CX3_HIGH_ID)
- sprintf(drvinfo->driver, DRV_NAME " (%s_CX-3)", mdev->dev->board_id);
- else
- sprintf(drvinfo->driver, DRV_NAME " (%s_CX)", mdev->dev->board_id);
- break;
- case 0xb0:
- sprintf(drvinfo->driver, DRV_NAME " (%s_CX-2)", mdev->dev->board_id);
- break;
- default:
- sprintf(drvinfo->driver, DRV_NAME " (%s)", mdev->dev->board_id);
- break;
- }
+ strncpy(drvinfo->driver, DRV_NAME, 32);
strncpy(drvinfo->version, DRV_VERSION " (" DRV_RELDATE ")", 32);
sprintf(drvinfo->fw_version, "%d.%d.%d",
(u16) (mdev->dev->caps.fw_ver >> 32),
--
1.6.0.2
^ permalink raw reply related
* Re: [RFC] myri10ge: small rx_done refactoring
From: Stanislaw Gruszka @ 2011-03-24 8:16 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Andrew Gallatin, Brice Goglin
In-Reply-To: <20110323083357.457f10aa@nehalam>
On Wed, Mar 23, 2011 at 08:33:57AM -0700, Stephen Hemminger wrote:
> On Wed, 23 Mar 2011 13:52:04 +0100
> Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>
> > Add lro_enable variable to read NETIF_F_LRO flag only once per napi poll
> > call. This should fix theoretical race condition with
> > myri10ge_set_rx_csum() and myri10ge_set_flags() where flag NETIF_F_LRO
> > can be changed.
>
> You may need a barrier or the race may still be there.
I don't understand why barrier in that case is need.
What I tried to avoid is.
myri10ge_clean_rx_done():
if (dev->features & NETIF_F_LRO)
setup lro
myri10ge_set_flags()
if (dev->features & NETIF_F_LRO)
flush lro
Now we read dev->features & NETIF_F_LRO only once to local
lro_enabled variable. So we can not flush without setup
or setup without flush. No idea why memory barries is still
needed.
> The driver seems to use mb() where wmb() is intended, and never use rmb()?
Yes, I think we can have some optimalization here.
Stanislaw
^ permalink raw reply
* Re: [PATCH] net_sched: fix THROTTLED/RUNNING race
From: David Miller @ 2011-03-24 7:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: shemminger, fchecconi, netdev
In-Reply-To: <1300949637.2810.75.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 24 Mar 2011 07:53:57 +0100
> [PATCH] net_sched: fix THROTTLED/RUNNING race
>
> commit fd245a4adb52 (net_sched: move TCQ_F_THROTTLED flag)
> added a race.
>
> qdisc_watchdog() is run from softirq, so special care should be taken or
> we can lose one state transition (THROTTLED/RUNNING)
>
> Prior to fd245a4adb52, we were manipulating q->flags (qdisc->flags &=
> ~TCQ_F_THROTTLED;) and this manipulation could only race with
> qdisc_warn_nonwc().
>
> Since we want to avoid atomic ops in qdisc fast path - it was the
> meaning of commit 371121057607e (QDISC_STATE_RUNNING dont need atomic
> bit ops) - fix is to move THROTTLE bit into 'state' field, this one
> being manipulated with SMP and IRQ safe operations.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, thanks a lot for tracking this down Eric.
^ permalink raw reply
* [PATCH] net_sched: fix THROTTLED/RUNNING race
From: Eric Dumazet @ 2011-03-24 6:53 UTC (permalink / raw)
To: Stephen Hemminger, David Miller; +Cc: Fabio Checconi, netdev
In-Reply-To: <1300862758.2717.41.camel@edumazet-laptop>
Le mercredi 23 mars 2011 à 07:45 +0100, Eric Dumazet a écrit :
> While polishing QFQ scheduler, and tracking a bug in it, I finally
> replaced in my tc scripts "QFQ experimental" by "SFQ rock solid" and
> found I could have a hang in some situations :(
>
> I made a bisection and found :
>
> # git bisect good
> 7a6362800cb7d1d618a697a650c7aaed3eb39320 is the first bad commit
>
> It seems I am stuck...
>
> # git bisect log
> git bisect start
> # bad: [c360d5b53a7fec44ae4402e1f13fc888f57ddc3b] Merge branch 'master'
> of git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
> git bisect bad c360d5b53a7fec44ae4402e1f13fc888f57ddc3b
> # good: [07a2039b8eb0af4ff464efd3dfd95de5c02648c6] Linux 2.6.30
> git bisect good 07a2039b8eb0af4ff464efd3dfd95de5c02648c6
> # good: [2ec8c6bb5d8f3a62a79f463525054bae1e3d4487] Merge branch 'master'
> of /home/davem/src/GIT/linux-2.6/
> git bisect good 2ec8c6bb5d8f3a62a79f463525054bae1e3d4487
> # good: [e0e170bd7ded2ec16e2813d63c0faff43193fde8] Merge branch 'next'
> of git://git.monstr.eu/linux-2.6-microblaze
> git bisect good e0e170bd7ded2ec16e2813d63c0faff43193fde8
> # good: [40c73abbb37e399eba274fe49e520ffa3dd65bdb] Merge branch
> 'for_linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs-2.6
> git bisect good 40c73abbb37e399eba274fe49e520ffa3dd65bdb
> # good: [4b66fef9b591b95f447aea12242a1133deb0bd22] mcast: net_device dev
> not used
> git bisect good 4b66fef9b591b95f447aea12242a1133deb0bd22
> # bad: [7a6362800cb7d1d618a697a650c7aaed3eb39320] Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6
> git bisect bad 7a6362800cb7d1d618a697a650c7aaed3eb39320
> # good: [971f115a50afbe409825c9f3399d5a3b9aca4381] Merge branch
> 'usb-next' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb-2.6
> git bisect good 971f115a50afbe409825c9f3399d5a3b9aca4381
> # good: [5917def58ab9f5848f2d1da835a33a490d0c8c69] staging/easycap:
> reduce code nesting in easycap_sound.c
> git bisect good 5917def58ab9f5848f2d1da835a33a490d0c8c69
> # good: [6445ced8670f37cfc2c5e24a9de9b413dbfc788d] Merge branch
> 'staging-next' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6
> git bisect good 6445ced8670f37cfc2c5e24a9de9b413dbfc788d
> # good: [da91981bee8de20bcd06ee0dbddd53d62d23b1bd] ipv4: Use flowi4 in
> ipmr code.
> git bisect good da91981bee8de20bcd06ee0dbddd53d62d23b1bd
> # good: [106af2c99a5249b809aaed45b8353ac087821f4a] Merge branch 'master'
> of
> git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-next-2.6
> into for-davem
> git bisect good 106af2c99a5249b809aaed45b8353ac087821f4a
> # good: [638be344593b66ccca6802c6076a5b3d9200829d] Phonet: fix
> aligned-mode pipe socket buffer header reserve
> git bisect good 638be344593b66ccca6802c6076a5b3d9200829d
> # good: [c337ffb68e1e71bad069b14d2246fa1e0c31699c] Merge branch 'master'
> of master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6
> git bisect good c337ffb68e1e71bad069b14d2246fa1e0c31699c
> # good: [ee0caa79569a9c44febc18480beef4847aa8cecd] Merge branch 'master'
> of git://git.kernel.org/pub/scm/linux/kernel/git/kaber/nf-next-2.6
> git bisect good ee0caa79569a9c44febc18480beef4847aa8cecd
> # good: [0bd80dad57d82676ee484fb1f9aa4c5e8b5bc469] net: get rid of
> multiple bond-related netdevice->priv_flags
> git bisect good 0bd80dad57d82676ee484fb1f9aa4c5e8b5bc469
> # good: [8a4eb5734e8d1dc60a8c28576bbbdfdcc643626d] net: introduce
> rx_handler results and logic around that
> git bisect good 8a4eb5734e8d1dc60a8c28576bbbdfdcc643626d
> # good: [ceda86a108671294052cbf51660097b6534672f5] bonding: enable
> netpoll without checking link status
> git bisect good ceda86a108671294052cbf51660097b6534672f5
>
> Any idea how we can find the problem ?
>
> Script to reproduce the problem :
>
>
> modprobe dummy
>
> ifconfig dummy0 10.2.2.254 netmask 255.255.255.0 up
>
> for i in `seq 1 240`
> do
> arp -H ether -i dummy0 -s 10.2.2.$i 00:00:0c:07:ac:$(printf %02x $i)
> done
>
>
> DEV=dummy0
> RATE="rate 40Mbit"
> TNETS="10.2.2.0/25"
> ALLOT="allot 20000"
>
>
> tc qdisc del dev dummy0 root 2>/dev/null
>
>
> tc qdisc add dev $DEV root handle 1: est 1sec 8sec cbq avpkt 1000 rate 100Mbit \
> bandwidth 100Mbit
> tc class add dev $DEV parent 1: classid 1:1 \
> est 1sec 8sec cbq allot 10000 mpu 64 \
> rate 100Mbit prio 1 avpkt 1500 bounded
>
> # output to test nets : 40 Mbit limit
> tc class add dev $DEV parent 1:1 classid 1:11 \
> est 1sec 8sec cbq $ALLOT mpu 64 \
> $RATE prio 2 avpkt 1400 bounded
>
> tc qdisc add dev $DEV parent 1:11 handle 11: \
> est 1sec 8sec sfq
>
>
> for privnet in $TNETS
> do
> tc filter add dev $DEV parent 1: protocol ip prio 100 u32 \
> match ip dst $privnet flowid 1:11
> done
>
> tc filter add dev $DEV parent 1: protocol ip prio 100 u32 \
> match ip protocol 0 0x00 flowid 1:1
>
>
> iperf -u -c 10.2.2.1 -P 32 -l 50
> iperf -u -c 10.2.2.1 -P 32 -l 50
> iperf -u -c 10.2.2.1 -P 32 -l 50
> tc -s -d qdisc show dev dummy0
>
Okay... this bug was hard to find.
David, would you be OK if we send QFQ patch for 2.6.39 ?
I know its a bit late but its a new qdisc, very well tested now, just
tell us :)
Thanks
[PATCH] net_sched: fix THROTTLED/RUNNING race
commit fd245a4adb52 (net_sched: move TCQ_F_THROTTLED flag)
added a race.
qdisc_watchdog() is run from softirq, so special care should be taken or
we can lose one state transition (THROTTLED/RUNNING)
Prior to fd245a4adb52, we were manipulating q->flags (qdisc->flags &=
~TCQ_F_THROTTLED;) and this manipulation could only race with
qdisc_warn_nonwc().
Since we want to avoid atomic ops in qdisc fast path - it was the
meaning of commit 371121057607e (QDISC_STATE_RUNNING dont need atomic
bit ops) - fix is to move THROTTLE bit into 'state' field, this one
being manipulated with SMP and IRQ safe operations.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Stephen Hemminger <shemminger@vyatta.com>
Cc: Fabio Checconi <fchecconi@gmail.com>
---
include/net/sch_generic.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index a9505b6..b931f02 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -25,6 +25,7 @@ struct qdisc_rate_table {
enum qdisc_state_t {
__QDISC_STATE_SCHED,
__QDISC_STATE_DEACTIVATED,
+ __QDISC_STATE_THROTTLED,
};
/*
@@ -32,7 +33,6 @@ enum qdisc_state_t {
*/
enum qdisc___state_t {
__QDISC___STATE_RUNNING = 1,
- __QDISC___STATE_THROTTLED = 2,
};
struct qdisc_size_table {
@@ -106,17 +106,17 @@ static inline void qdisc_run_end(struct Qdisc *qdisc)
static inline bool qdisc_is_throttled(const struct Qdisc *qdisc)
{
- return (qdisc->__state & __QDISC___STATE_THROTTLED) ? true : false;
+ return test_bit(__QDISC_STATE_THROTTLED, &qdisc->state) ? true : false;
}
static inline void qdisc_throttled(struct Qdisc *qdisc)
{
- qdisc->__state |= __QDISC___STATE_THROTTLED;
+ set_bit(__QDISC_STATE_THROTTLED, &qdisc->state);
}
static inline void qdisc_unthrottled(struct Qdisc *qdisc)
{
- qdisc->__state &= ~__QDISC___STATE_THROTTLED;
+ clear_bit(__QDISC_STATE_THROTTLED, &qdisc->state);
}
struct Qdisc_class_ops {
^ permalink raw reply related
* Re: [PATCH 2/2] virtio_net: remove send completion interrupts and avoid TX queue overrun through packet drop
From: Michael S. Tsirkin @ 2011-03-24 6:39 UTC (permalink / raw)
To: Rusty Russell; +Cc: Shirley Ma, Herbert Xu, davem, kvm, netdev, Anthony Liguori
In-Reply-To: <87tyetbcem.fsf@rustcorp.com.au>
On Thu, Mar 24, 2011 at 10:46:49AM +1030, Rusty Russell wrote:
> On Tue, 22 Mar 2011 13:36:50 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, Mar 21, 2011 at 11:03:07AM -0700, Shirley Ma wrote:
> > > On Fri, 2011-03-18 at 18:41 -0700, Shirley Ma wrote:
> > > > > > + /* Drop packet instead of stop queue for better
> > > > performance
> > > > > */
> > > > >
> > > > > I would like to see some justification as to why this is the right
> > > > > way to go and not just papering over the real problem.
> > > >
> > > > Fair. KVM guest virtio_net TX queue stop/restart is pretty expensive,
> > > > which involves:
> > > >
> > > > 1. Guest enable callback: one memory barrier, interrupt flag set
> > >
> > > Missed this cost: for history reason, it also involves a guest exit from
> > > I/O write (PCI_QUEUE_NOTIFY).
> >
> > OK, after some research, it looks like the reason was the tx timer that
> > qemu used to use. So the hack of avoiding the add_buf call will
> > avoid this kick and so break these hosts.
> > I guess we can add a feature bit to detect a new host
> > and so avoid the kick. We are running low on feature bits
> > unfortunately, but just fo testing, could you quantify the difference
> > that this makes using the following patch:
>
> Performance would suffer for those ancient qemus if we didn't do this,
> but it wouldn't be fatal to them.
>
> I think we should just remove it; the standard certainly doesn't mention
> it.
>
> Cheers,
> Rusty.
I agree here. Anthony, agree?
--
MST
^ permalink raw reply
* RE: [PATCH v2 08/16] mlx4_en: Reporting HW revision in ethtool -i
From: Yevgeny Petrilin @ 2011-03-24 6:26 UTC (permalink / raw)
To: Ben Hutchings
Cc: davem@davemloft.net, netdev@vger.kernel.org, Eugenia Emantayev
In-Reply-To: <1300938019.26693.528.camel@localhost>
I will send the patch removing the HW information.
Yevgeny
> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Thursday, March 24, 2011 5:40 AM
> To: Yevgeny Petrilin
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Eugenia Emantayev
> Subject: RE: [PATCH v2 08/16] mlx4_en: Reporting HW revision in ethtool
> -i
>
> On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:
> > >
> > > This is an abuse of the ethtool_drvinfo::driver field.
> > >
> > > Your users can use lspci -v, can't they?
> > >
> > I don't think there is a problem here.
> > We have always reported the HW model via Ethtool, we just expanded
> the information
> > we provide.
> > Our users prefer to see the information in ethtool.
>
> I can provide a patch for David if you won't.
>
> Ben.
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* RE: c_can: TX handling
From: Bhupesh SHARMA @ 2011-03-24 5:43 UTC (permalink / raw)
To: Jan Altenberg
Cc: wg@grandegger.com, kurt.van.dijck@eia.be,
b.spranger@linutronix.de, netdev@vger.kernel.org,
Socketcan-core@lists.berlios.de
In-Reply-To: <16a340801622a96218c76dbbabc7a23f.squirrel@www.linutronix.de>
Hi Jan,
> > I did some more testing on the SocketCAN driver for the Bosch c_can
> > controller and I observed some strange behaviour for the TX handling.
> > First of all the TX bytes are not accounted correctly. The reason for
> > that
> > seems to be quite obvious if we look into c_can_do_tx():
> >
> > [...]
> >
> > c_can_inval_msg_object(dev, 0, msg_obj_no);
> > val = c_can_read_reg32(priv, &priv->regs->txrqst1);
> > if (!(val & (1 << msg_obj_no))) {
> > can_get_echo_skb(dev,
> > msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> > stats->tx_bytes += priv->read_reg(priv,
> > &priv->regs->ifregs[0].msg_cntrl)
> > & IF_MCONT_DLC_MASK;
> >
> > So, we first invalidate the message object and afterwards we read the
> > DLC
> > value from the msg_cntrl (which is 0 after invalidating the
> > message object) to account the TX bytes. So tx_bytes will always be
> 0.
> > The
> > fix should be easy, I think, we can just move
> > c_can_inval_msg_object to the end of that loop.
>
> Hmmm..
> As far as I remember, *ifconfig* used to provide the correct tx_bytes
> stats
> on my SPEAr board. Let me recheck this again today evening and get back
> to you.
I managed to get hold of one SPEAr board and did some quick tests.
Indeed the tx_bytes field is 0 when we execute *ifconfig*
I think the easy fix would be to add c_can_inval_msg_object(dev, 0, msg_obj_no);
call inside the *if* construct. Could you please prepare a patch after
testing the same at your end.
Regards,
Bhupesh
^ permalink raw reply
* RE: c_can: TX handling
From: Bhupesh SHARMA @ 2011-03-24 4:18 UTC (permalink / raw)
To: Jan Altenberg
Cc: wg@grandegger.com, kurt.van.dijck@eia.be,
b.spranger@linutronix.de, netdev@vger.kernel.org
In-Reply-To: <16a340801622a96218c76dbbabc7a23f.squirrel@www.linutronix.de>
Hi Jan,
Thanks for your work on c_can.
This week has been very busy for me, so please excuse me for the late reply.
> Hi all,
>
> I did some more testing on the SocketCAN driver for the Bosch c_can
> controller and I observed some strange behaviour for the TX handling.
> First of all the TX bytes are not accounted correctly. The reason for
> that
> seems to be quite obvious if we look into c_can_do_tx():
>
> [...]
>
> c_can_inval_msg_object(dev, 0, msg_obj_no);
> val = c_can_read_reg32(priv, &priv->regs->txrqst1);
> if (!(val & (1 << msg_obj_no))) {
> can_get_echo_skb(dev,
> msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> stats->tx_bytes += priv->read_reg(priv,
> &priv->regs->ifregs[0].msg_cntrl)
> & IF_MCONT_DLC_MASK;
>
> So, we first invalidate the message object and afterwards we read the
> DLC
> value from the msg_cntrl (which is 0 after invalidating the
> message object) to account the TX bytes. So tx_bytes will always be 0.
> The
> fix should be easy, I think, we can just move
> c_can_inval_msg_object to the end of that loop.
Hmmm..
As far as I remember, *ifconfig* used to provide the correct tx_bytes stats
on my SPEAr board. Let me recheck this again today evening and get back to you.
> The second problem is related to tx_next, which should hold the number
> of
> the oldest CAN frame, which was not on the line:
>
> for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
> msg_obj_no = get_tx_echo_msg_obj(priv);
> c_can_inval_msg_object(dev, 0, msg_obj_no);
> val = c_can_read_reg32(priv, &priv->regs->txrqst1);
> if (!(val & (1 << msg_obj_no))) {
> can_get_echo_skb(dev,
> msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> stats->tx_bytes += priv->read_reg(priv,
> &priv->regs->ifregs[0].msg_cntrl)
> & IF_MCONT_DLC_MASK;
> stats->tx_packets++;
> }
> }
>
> But tx_echo is incremented unconditionally and we don't actually track
> the
> number of the oldest unsent frame.
> Let's assume the following scenario: We bring up can0 and send 3
> frames: TX object: 0, 1, 2; 1 and 2 make it on the line, but 0 is
> still pending. If we go through the above loop in that situation, we
> will
> skip message object 0, because the txrqst bit is still set. We will
> account message object 1 and 2. That's correct, but afterwards tx_echo
> is
> set to 2, BUT the oldest message which is pending is 0. Am I right or
> did
> I get something wrong?
> The operation of c_can_do_tx() is described as follows: "We iterate
> from
> priv->tx_echo to priv->tx_next and check if the packet has been
> transmitted, echo it back to the CAN framework. If we discover a not
> yet
> transmitted package, stop looking for more." The actual
> implementation doesn't seem to stop if we discover a not yet
> transmitted package. But I'm not sure if just stopping might be a
> good idea, because in that case, the echo skb for already transmitted
> messages might be delayed by not yet transmitted messages.
>
Just to better understand this can you send me your *candump* output
when you see that the message object put earlier in the message RAM
being pending on the line.
Your point seems valid but I want to make sure that this is not done by the
C_CAN core implicitly..
Regards,
Bhupesh
^ permalink raw reply
* Re: [PATCH 2/2] virtio_net: remove send completion interrupts and avoid TX queue overrun through packet drop
From: Shirley Ma @ 2011-03-24 4:14 UTC (permalink / raw)
To: Rusty Russell; +Cc: Michael S. Tsirkin, Herbert Xu, davem, kvm, netdev
In-Reply-To: <87r59xbbr6.fsf@rustcorp.com.au>
On Thu, 2011-03-24 at 11:00 +1030, Rusty Russell wrote:
> > With simply removing the notify here, it does help the case when TX
> > overrun hits too often, for example for 1K message size, the single
> > TCP_STREAM performance improved from 2.xGb/s to 4.xGb/s.
>
> OK, we'll be getting rid of the "kick on full", so please delete that
> on
> all benchmarks.
>
> Now, does the capacity check before add_buf() still win anything? I
> can't see how unless we have some weird bug.
>
> Once we've sorted that out, we should look at the more radical change
> of publishing last_used and using that to intuit whether interrupts
> should be sent. If we're not careful with ordering and barriers that
> could introduce more bugs.
Without the kick, it's not necessary for capacity check. I am
regenerating the patch with add_buf check and summit the patch after
passing all tests.
> Anything else on the optimization agenda I've missed?
Tom found small performance gain with freeing the used buffers when half
of the ring is full in TCP_RR workload.
I think we need a new API in virtio, which frees all used buffers at
once, I am testing the performance now, the new API looks like:
drivers/virtio/virtio_ring.c | 40 +++++++++++++++++++++++++++++++++++
include/linux/virtio.h | 6 +++++
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cc2f73e..6d2dc16 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -329,6 +329,46 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
}
EXPORT_SYMBOL_GPL(virtqueue_get_buf);
+int virtqueue_free_used(struct virtqueue *_vq, void (*free)(void *))
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ unsigned int i;
+ void *buf;
+
+ START_USE(vq);
+
+ if (unlikely(vq->broken)) {
+ END_USE(vq);
+ return NULL;
+ }
+
+ /* Only get used array entries after they have been exposed by host. */
+ virtio_rmb();
+
+ while (vq->last_used_idx != vq->vring.used->idx) {
+ i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
+
+ if (unlikely(i >= vq->vring.num)) {
+ BAD_RING(vq, "id %u out of range\n", i);
+ return NULL;
+ }
+ if (unlikely(!vq->data[i])) {
+ BAD_RING(vq, "id %u is not a head!\n", i);
+ return NULL;
+ }
+
+ /* detach_buf clears data, so grab it now. */
+ buf = vq->data[i];
+ detach_buf(vq, i);
+ free(buf);
+ vq->last_used_idx++;
+ }
+ END_USE(vq);
+ return vq->num_free;
+}
+
+EXPORT_SYMBOL_GPL(virtqueue_free_used);
+
void virtqueue_disable_cb(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index aff5b4f..19acc66 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -42,6 +42,10 @@ struct virtqueue {
* vq: the struct virtqueue we're talking about.
* len: the length written into the buffer
* Returns NULL or the "data" token handed to add_buf.
+ * virtqueue_free_used: free all used buffers in the queue
+ * vq: the struct virtqueue we're talking about.
+ * free: free buf function from caller.
+ * Returns remaining capacity of the queue.
* virtqueue_disable_cb: disable callbacks
* vq: the struct virtqueue we're talking about.
* Note that this is not necessarily synchronous, hence unreliable and only
@@ -82,6 +86,8 @@ void virtqueue_kick(struct virtqueue *vq);
void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len);
+int virtqueue_free_used(struct virtqueue *vq, void (*free)(void *buf));
+
void virtqueue_disable_cb(struct virtqueue *vq);
bool virtqueue_enable_cb(struct virtqueue *vq);
Thanks
Shirley
^ permalink raw reply related
* RE: [PATCH v2 08/16] mlx4_en: Reporting HW revision in ethtool -i
From: Ben Hutchings @ 2011-03-24 3:40 UTC (permalink / raw)
To: Yevgeny Petrilin
Cc: davem@davemloft.net, netdev@vger.kernel.org, Eugenia Emantayev
In-Reply-To: <953B660C027164448AE903364AC447D20705BE28@mtldag01.mtl.com>
On Wed, 2011-03-23 at 15:10 +0000, Yevgeny Petrilin wrote:
> >
> > This is an abuse of the ethtool_drvinfo::driver field.
> >
> > Your users can use lspci -v, can't they?
> >
> I don't think there is a problem here.
> We have always reported the HW model via Ethtool, we just expanded the information
> we provide.
> Our users prefer to see the information in ethtool.
I can provide a patch for David if you won't.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH net-next] net:set valid name before calling ndo_init()
From: Weiping Pan(潘卫平) @ 2011-03-24 3:16 UTC (permalink / raw)
To: davem, eric.dumazet, xiaosuo, mirq-linux, jpirko
Cc: netdev, linux-kernel, Pan Weiping
From: Pan Weiping <panweiping3@gmail.com>
A bug of bonding was invloved by e815d19ffe02bdfda1260949ef2b1806171,
see example 1 and 2.
In register_netdevice(), the name of net_device is not valid until
dev_get_valid_name() is called. But dev->netdev_ops->ndo_init(that is
bond_init) is called before dev_get_valid_name(),
and it uses the invalid name of net_device.
I think register_netdevice() should make sure that the name of net_device is
valid before calling ndo_init().
example 1:
modprobe bonding
ls /proc/net/bonding/bond%d
ps -eLf
root 3398 2 3398 0 1 21:34 ? 00:00:00 [bond%d]
example 2:
modprobe bonding max_bonds=3
[ 170.100292] bonding: Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011)
[ 170.101090] bonding: Warning: either miimon or arp_interval and arp_ip_target module parameters must be specified, otherwise bonding will not detect link failures! see bonding.txt for details.
[ 170.102469] ------------[ cut here ]------------
[ 170.103150] WARNING: at /home/pwp/net-next-2.6/fs/proc/generic.c:586 proc_register+0x126/0x157()
[ 170.104075] Hardware name: VirtualBox
[ 170.105065] proc_dir_entry 'bonding/bond%d' already registered
[ 170.105613] Modules linked in: bonding(+) sunrpc ipv6 uinput microcode ppdev parport_pc parport joydev e1000 pcspkr i2c_piix4 i2c_core [last unloaded: bonding]
[ 170.108397] Pid: 3457, comm: modprobe Not tainted 2.6.39-rc2+ #14
[ 170.108935] Call Trace:
[ 170.109382] [<c0438f3b>] warn_slowpath_common+0x6a/0x7f
[ 170.109911] [<c051a42a>] ? proc_register+0x126/0x157
[ 170.110329] [<c0438fc3>] warn_slowpath_fmt+0x2b/0x2f
[ 170.110846] [<c051a42a>] proc_register+0x126/0x157
[ 170.111870] [<c051a4dd>] proc_create_data+0x82/0x98
[ 170.112335] [<f94e6af6>] bond_create_proc_entry+0x3f/0x73 [bonding]
[ 170.112905] [<f94dd806>] bond_init+0x77/0xa5 [bonding]
[ 170.113319] [<c0721ac6>] register_netdevice+0x8c/0x1d3
[ 170.113848] [<f94e0e30>] bond_create+0x6c/0x90 [bonding]
[ 170.114322] [<f94f4763>] bonding_init+0x763/0x7b1 [bonding]
[ 170.114879] [<c0401240>] do_one_initcall+0x76/0x122
[ 170.115317] [<f94f4000>] ? 0xf94f3fff
[ 170.115799] [<c0463f1e>] sys_init_module+0x1286/0x140d
[ 170.116879] [<c07c6d9f>] sysenter_do_call+0x12/0x28
[ 170.117404] ---[ end trace 64e4fac3ae5fff1a ]---
[ 170.117924] bond%d: Warning: failed to register to debugfs
[ 170.128728] ------------[ cut here ]------------
[ 170.129360] WARNING: at /home/pwp/net-next-2.6/fs/proc/generic.c:586 proc_register+0x126/0x157()
[ 170.130323] Hardware name: VirtualBox
[ 170.130797] proc_dir_entry 'bonding/bond%d' already registered
[ 170.131315] Modules linked in: bonding(+) sunrpc ipv6 uinput microcode ppdev parport_pc parport joydev e1000 pcspkr i2c_piix4 i2c_core [last unloaded: bonding]
[ 170.133731] Pid: 3457, comm: modprobe Tainted: G W 2.6.39-rc2+ #14
[ 170.134308] Call Trace:
[ 170.134743] [<c0438f3b>] warn_slowpath_common+0x6a/0x7f
[ 170.135305] [<c051a42a>] ? proc_register+0x126/0x157
[ 170.135820] [<c0438fc3>] warn_slowpath_fmt+0x2b/0x2f
[ 170.137168] [<c051a42a>] proc_register+0x126/0x157
[ 170.137700] [<c051a4dd>] proc_create_data+0x82/0x98
[ 170.138174] [<f94e6af6>] bond_create_proc_entry+0x3f/0x73 [bonding]
[ 170.138745] [<f94dd806>] bond_init+0x77/0xa5 [bonding]
[ 170.139278] [<c0721ac6>] register_netdevice+0x8c/0x1d3
[ 170.139828] [<f94e0e30>] bond_create+0x6c/0x90 [bonding]
[ 170.140361] [<f94f4763>] bonding_init+0x763/0x7b1 [bonding]
[ 170.140927] [<c0401240>] do_one_initcall+0x76/0x122
[ 170.141494] [<f94f4000>] ? 0xf94f3fff
[ 170.141975] [<c0463f1e>] sys_init_module+0x1286/0x140d
[ 170.142463] [<c07c6d9f>] sysenter_do_call+0x12/0x28
[ 170.142974] ---[ end trace 64e4fac3ae5fff1b ]---
[ 170.144949] bond%d: Warning: failed to register to debugfs
Signed-off-by: Pan Weiping <panweiping3@gmail.com>
---
net/core/dev.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 75898a3..f289117 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5412,6 +5412,10 @@ int register_netdevice(struct net_device *dev)
dev->iflink = -1;
+ ret = dev_get_valid_name(dev, dev->name);
+ if (ret < 0)
+ goto out;
+
/* Init, if this function is available */
if (dev->netdev_ops->ndo_init) {
ret = dev->netdev_ops->ndo_init(dev);
@@ -5422,10 +5426,6 @@ int register_netdevice(struct net_device *dev)
}
}
- ret = dev_get_valid_name(dev, dev->name);
- if (ret < 0)
- goto err_uninit;
-
dev->ifindex = dev_new_index(net);
if (dev->iflink == -1)
dev->iflink = dev->ifindex;
--
1.7.4
^ permalink raw reply related
* Re: [PATCH 2/2] virtio_net: remove send completion interrupts and avoid TX queue overrun through packet drop
From: Rusty Russell @ 2011-03-24 0:30 UTC (permalink / raw)
To: Shirley Ma, Michael S. Tsirkin; +Cc: Herbert Xu, davem, kvm, netdev
In-Reply-To: <1300847204.3441.26.camel@localhost.localdomain>
> With simply removing the notify here, it does help the case when TX
> overrun hits too often, for example for 1K message size, the single
> TCP_STREAM performance improved from 2.xGb/s to 4.xGb/s.
OK, we'll be getting rid of the "kick on full", so please delete that on
all benchmarks.
Now, does the capacity check before add_buf() still win anything? I
can't see how unless we have some weird bug.
Once we've sorted that out, we should look at the more radical change
of publishing last_used and using that to intuit whether interrupts
should be sent. If we're not careful with ordering and barriers that
could introduce more bugs.
Anything else on the optimization agenda I've missed?
Thanks,
Rusty.
^ permalink raw reply
* Re: [RFC PATCH] et1011c: Replaced PHY driver by a small dm646x board fixup
From: Moffett, Kyle D @ 2011-03-24 1:38 UTC (permalink / raw)
To: Andy Fleming
Cc: Randy Dunlap, Russell King, Kevin Hilman, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, David J. Choi,
Uwe Kleine-König, Giuseppe Cavallaro, David S. Miller,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <AANLkTin14WSZsc7+B-rK1dSn=XShNrF63k_47RXu-3G6@mail.gmail.com>
On Mar 23, 2011, at 20:24, Andy Fleming wrote:
> On Wed, Mar 23, 2011 at 5:33 PM, Kyle Moffett <Kyle.D.Moffett@boeing.com> wrote:
>> -
>> -static struct phy_driver et1011c_driver = {
>> - .phy_id = 0x0282f014,
>> - .name = "ET1011C",
>> - .phy_id_mask = 0xfffffff0,
>> - .features = (PHY_BASIC_FEATURES | SUPPORTED_1000baseT_Full),
>> - .flags = PHY_POLL,
>> - .config_aneg = et1011c_config_aneg,
>> - .read_status = et1011c_read_status,
>> - .driver = { .owner = THIS_MODULE,},
>> -};
>
> Might I suggest that you not eliminate the whole driver? If you leave
> just this part (and the init stuff below it), and convert the
> config_aneg and read_status pointers to use the genphy versions, the
> kernel will, at least, be able to report what type of PHY it is. And
> maybe one day, someone who is familiar with the inner workings of this
> PHY will fill in a more correct driver.
Hmm, I suppose that's an option, but really that's missing the point of
the "genphy" driver, which gives full functionality with any PHY that
correctly implements that portion of the standard IEEE 802.3 spec.
I've looked through the ET1011C datasheet/programming-guide, and I can't
see anything (aside from hardware bugs/errata) that would interfere with
the proper operation of the PHY using the generic driver.
There are a *lot* of PHYs which don't have their own Linux drivers,
simply because a custom driver is mostly unnecessary for a properly
designed PHY. The only exception is that PHYs supporting IRQ-driven
operation need .ack_interrupt() and .config_intr(), but that wasn't
included in the first driver either (IE: it is "PHY_POLL").
Looking at the representative sample of the PHY drivers in linux:
bcm63xx: IRQ support
bcm54xx: IRQ support, hardware errata, custom LED settings
cicada: IRQ support, hardware errata
davicom: IRQ support, hardware errata
icplus: Ethernet switch posing as a PHY
lxt: IRQ support, copper/fiber switching
marvell: IRQ support, hardware errata, copper/fiber switching
micrel: IRQ support
national: IRQ support, hardware-specific initialization
qsemi: IRQ support
realtek: IRQ support
ste10Xp: IRQ support
vitesse: IRQ support, copper/fiber switching
Besides which, for just identifying unknown PHYs we can look in the PHY
registers 2 and 3, they're defined to contain a 22-bit manufacturer OUI
and some additional model/revision numbers. We can map that to
human-readable strings in userspace today the same way as PCI, USB, etc.
Cheers,
Kyle Moffett
^ permalink raw reply
* Re: [PATCH 02/36] scsi,rcu: convert call_rcu(fc_rport_free_rcu) to kfree_rcu()
From: Paul E. McKenney @ 2011-03-24 0:32 UTC (permalink / raw)
To: James Bottomley
Cc: Robert Love, Lai Jiangshan, Ingo Molnar, Jens Axboe, Neil Horman,
David S. Miller, Alexey Kuznetsov, Pekka Savola (ipv6),
James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Eric Dumazet,
Stephen Hemminger, Tejun Heo, Jarek Poplawski,
linux-kernel@vger.kernel.org, devel@open-fcoe.org,
linux-scsi@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <1300920332.15899.45.camel@mulgrave.site>
On Wed, Mar 23, 2011 at 05:45:32PM -0500, James Bottomley wrote:
> On Wed, 2011-03-23 at 15:24 -0700, Paul E. McKenney wrote:
> > On Wed, Mar 23, 2011 at 09:05:51AM -0500, James Bottomley wrote:
> > > On Tue, 2011-03-22 at 23:50 -0700, Paul E. McKenney wrote:
> > > > The kfree_rcu() definition is as
> > > > follows:
> > > >
> > > > #define kfree_rcu(ptr, rcu_head) \
> > > > __kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
> > >
> > > Isn't this one of those cases where the obvious use of the interface is
> > > definitely wrong?
> > >
> > > It's also another nasty pseudo C prototype. I know we do this sort of
> > > thing for container_of et al, but I don't really think we want to extend
> > > it.
> > >
> > > Why not make the interface take a pointer to the embedding structure and
> > > one to the rcu_head ... that way all pointer mathematics can be
> > > contained inside the RCU routines.
> >
> > Hello, James,
> >
> > If you pass in a pair of pointers, then it is difficult for RCU to detect
> > bugs where the two pointers are unrelated. Yes, you can do some sanity
> > checks, but these get cumbersome and have corner cases where they can
> > be fooled. In contrast, Lai's interface allows the compiler to do the
> > needed type checking -- unless the second argument is a field of type
> > struct rcu_head in the structure pointed to by the first argument, the
> > compiler will complain.
> >
> > Either way, the pointer mathematics are buried in the RCU API.
> >
> > Or am I missing something here?
>
> No ... I like the utility ... I just dislike the inelegance of having to
> name a structure element in what looks like a C prototype.
>
> I can see this proliferating everywhere since most of our reference
> counting release callbacks basically free the enclosing object ...
Indeed! Improvements are welcome -- it is just that I am not convinced
that the dual-pointer approach is really an improvement.
The C preprocessor... It is ugly, inelegant, painful, annoying, and
should have been strangled at birth -- but it is always there when you
need it!
Thanx, Paul
^ permalink raw reply
* Re: [RFC PATCH] et1011c: Replaced PHY driver by a small dm646x board fixup
From: Andy Fleming @ 2011-03-24 0:24 UTC (permalink / raw)
To: Kyle Moffett
Cc: netdev, Kevin Hilman, Russell King, David S. Miller,
Uwe Kleine-König, David J. Choi, Giuseppe Cavallaro,
Randy Dunlap, linux-arm-kernel, linux-kernel
In-Reply-To: <1300919616-6780-1-git-send-email-Kyle.D.Moffett@boeing.com>
On Wed, Mar 23, 2011 at 5:33 PM, Kyle Moffett <Kyle.D.Moffett@boeing.com> wrote:
> -
> -static struct phy_driver et1011c_driver = {
> - .phy_id = 0x0282f014,
> - .name = "ET1011C",
> - .phy_id_mask = 0xfffffff0,
> - .features = (PHY_BASIC_FEATURES | SUPPORTED_1000baseT_Full),
> - .flags = PHY_POLL,
> - .config_aneg = et1011c_config_aneg,
> - .read_status = et1011c_read_status,
> - .driver = { .owner = THIS_MODULE,},
> -};
Might I suggest that you not eliminate the whole driver? If you leave
just this part (and the init stuff below it), and convert the
config_aneg and read_status pointers to use the genphy versions, the
kernel will, at least, be able to report what type of PHY it is. And
maybe one day, someone who is familiar with the inner workings of this
PHY will fill in a more correct driver.
Andy
^ 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