* [ethtool PATCH 2/4] Cleanup defines and header includes to address several issues
From: Alexander Duyck @ 2011-05-04 18:41 UTC (permalink / raw)
To: davem, jeffrey.t.kirsher, dm, bhutchings; +Cc: netdev
In-Reply-To: <20110504183752.26394.29231.stgit@gitlad.jf.intel.com>
This change is meant to address several issues. First it moves the check
for ethtool-config.h into ethtool-util.h the reason for this change is so
that any references to ethtool-util.h outside of ethtool.c will use the
correct defines for the endian types.
In addition I have pulled several headers that will be common to both
ethtool.c and rxclass.c into the ethtool-util.h header file. I am also
centralizing several macros that will be needed across multiple files when
I implement the network flow classifier rules.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
ethtool-util.h | 17 +++++++++++++++--
ethtool.c | 17 +----------------
2 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/ethtool-util.h b/ethtool-util.h
index f053028..6a4f3f4 100644
--- a/ethtool-util.h
+++ b/ethtool-util.h
@@ -3,8 +3,13 @@
#ifndef ETHTOOL_UTIL_H__
#define ETHTOOL_UTIL_H__
+#ifdef HAVE_CONFIG_H
+#include "ethtool-config.h"
+#endif
#include <sys/types.h>
#include <endian.h>
+#include <sys/ioctl.h>
+#include <net/if.h>
/* ethtool.h expects these to be defined by <linux/types.h> */
#ifndef HAVE_BE_TYPES
@@ -12,14 +17,14 @@ typedef __uint16_t __be16;
typedef __uint32_t __be32;
#endif
-#include "ethtool-copy.h"
-
typedef unsigned long long u64;
typedef __uint32_t u32;
typedef __uint16_t u16;
typedef __uint8_t u8;
typedef __int32_t s32;
+#include "ethtool-copy.h"
+
#if __BYTE_ORDER == __BIG_ENDIAN
static inline u16 cpu_to_be16(u16 value)
{
@@ -40,6 +45,14 @@ static inline u32 cpu_to_be32(u32 value)
}
#endif
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
+#ifndef SIOCETHTOOL
+#define SIOCETHTOOL 0x8946
+#endif
+
/* National Semiconductor DP83815, DP83816 */
int natsemi_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
int natsemi_dump_eeprom(struct ethtool_drvinfo *info,
diff --git a/ethtool.c b/ethtool.c
index 9ad7000..24d4e4f 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -21,19 +21,12 @@
* * show settings for all devices
*/
-#ifdef HAVE_CONFIG_H
-# include "ethtool-config.h"
-#endif
-
-#include <sys/types.h>
+#include "ethtool-util.h"
#include <string.h>
#include <stdlib.h>
-#include <sys/types.h>
-#include <sys/ioctl.h>
#include <sys/stat.h>
#include <stdio.h>
#include <errno.h>
-#include <net/if.h>
#include <sys/utsname.h>
#include <limits.h>
#include <ctype.h>
@@ -43,18 +36,10 @@
#include <arpa/inet.h>
#include <linux/sockios.h>
-#include "ethtool-util.h"
-
-#ifndef SIOCETHTOOL
-#define SIOCETHTOOL 0x8946
-#endif
#ifndef MAX_ADDR_LEN
#define MAX_ADDR_LEN 32
#endif
-#ifndef ARRAY_SIZE
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-#endif
#ifndef HAVE_NETIF_MSG
enum {
^ permalink raw reply related
* [ethtool PATCH 0/4] v6 Add support for network flow classifier
From: Alexander Duyck @ 2011-05-04 18:41 UTC (permalink / raw)
To: davem, jeffrey.t.kirsher, dm, bhutchings; +Cc: netdev
This series is nearly identical to the last series with only a few changes in
the RX packet classification interface patch. Specifically I have narrowed
down the rule manager to the point where it is only used to identify a
location for a rule if the rule location is unspecified. The display
functionality for rules now just pulls the rule list in and goes through the
list from top to bottom displaying the rules instead of initializing the rule
manager and pulling the rules form there.
The main advantage to this approach is that the rule manager could easily be
replaced by a future ioctl call and on the failure of that ioctl call with a
return of EOPNOTSUPP it should be easy to fall back and just use the rule
manager.
Thanks,
Alex
---
Alexander Duyck (3):
Add support for __be64 and bitops, centralize several needed macros
Cleanup defines and header includes to address several issues
ethtool: remove strings based approach for displaying n-tuple
Santwona Behera (1):
v6 Add RX packet classification interface
Makefile.am | 3
ethtool-bitops.h | 25 +
ethtool-util.h | 47 ++
ethtool.8.in | 194 +++++-----
ethtool.c | 449 +++++++++++------------
rxclass.c | 1073 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 1467 insertions(+), 324 deletions(-)
create mode 100644 ethtool-bitops.h
create mode 100644 rxclass.c
--
^ permalink raw reply
* RE: bug fix for out-of-tree igb driver
From: Wyborny, Carolyn @ 2011-05-04 18:36 UTC (permalink / raw)
To: Ben Greear, netdev
In-Reply-To: <4DC1959B.4010609@candelatech.com>
>-----Original Message-----
>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
>On Behalf Of Ben Greear
>Sent: Wednesday, May 04, 2011 11:06 AM
>To: netdev
>Subject: bug fix for out-of-tree igb driver
>
>Just in case someone is using the out-of-tree igb driver, I
>think I found a bug.
>
>The tx_queue_mapping method should have r_idx >= adapter->num_tx_queues
>as shown below. The driver had: r_idx > adapter->num_tx_queues
>originally.
>
>Its possible my code is buggy for sending queue_mapping of 1
>when num_tx_queues is 1, but either way, the driver
>shouldn't crash.
>
>Upstream igb driver in mainline kernels does not have this problem.
>
>static inline struct igb_ring *igb_tx_queue_mapping(struct igb_adapter
>*adapter,
> struct sk_buff
>*skb)
>{
> unsigned int r_idx = skb->queue_mapping & (IGB_MAX_TX_QUEUES - 1);
>
> if (r_idx >= adapter->num_tx_queues)
> r_idx = r_idx % adapter->num_tx_queues;
>
> return adapter->tx_ring[r_idx];
>}
>
>Thanks,
>Ben
>
>--
>Ben Greear <greearb@candelatech.com>
>Candela Technologies Inc http://www.candelatech.com
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Ben,
I'll be fixing that ASAP and updating the driver version there.
Carolyn
Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation
^ permalink raw reply
* Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Alexander Duyck @ 2011-05-04 18:35 UTC (permalink / raw)
To: Dimitris Michailidis
Cc: Ben Hutchings, davem@davemloft.net, Kirsher, Jeffrey T,
netdev@vger.kernel.org
In-Reply-To: <4DC1986D.5020706@chelsio.com>
On 5/4/2011 11:18 AM, Dimitris Michailidis wrote:
> On 05/04/2011 10:41 AM, Alexander Duyck wrote:
>> On 5/4/2011 10:33 AM, Dimitris Michailidis wrote:
>>> On 05/04/2011 10:24 AM, Ben Hutchings wrote:
>>>> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote:
>>>>> On 05/03/2011 04:34 PM, Ben Hutchings wrote:
>>>>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:
>>>>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where
>>>>>>> there is
>>>>>>> enough knowledge to pick an appropriate slot. So I'd remove the
>>>>>>>
>>>>>>> if (loc == RX_CLS_LOC_UNSPEC)
>>>>>>>
>>>>>>> block above, let the driver pick a slot, and then pass the
>>>>>>> selected location
>>>>>>> back for ethtool to report.
>>>>>> But first we have to specify this in the ethtool API. So please
>>>>>> propose
>>>>>> a patch to ethtool.h.
>>>>> In the past we discussed that being able to specify the first
>>>>> available slot or
>>>>> the last available would be useful, so something like the below?
>>>>>
>>>>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>>>>> index 4194a20..909ef79 100644
>>>>> --- a/include/linux/ethtool.h
>>>>> +++ b/include/linux/ethtool.h
>>>>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext {
>>>>> * includes the %FLOW_EXT flag.
>>>>> * @ring_cookie: RX ring/queue index to deliver to, or
>>>>> %RX_CLS_FLOW_DISC
>>>>> * if packets should be discarded
>>>>> - * @location: Index of filter in hardware table
>>>>> + * @location: Index of filter in hardware table, or
>>>>> %RX_CLS_FLOW_FIRST_LOC for
>>>>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last
>>>>> available
>>>> [...]
>>>>
>>>> I think that's reasonable. We should also explicitly state that
>>>> location determines priority, i.e. if a packet matches two filters then
>>>> the one with the lower location wins.
>>>
>>> Easy and true for a TCAM. For hashing would you use the location to
>>> decide how
>>> to order filters that fall in the same bucket?
>>>
>>
>> The problem is none of this is backwards compatible. The niu driver has
>> supported the network flow classifier rules since 2.6.30. Adding this
>> would cause all rule setups for niu to fail because these locations
>> would have to exist outside of the current rule locations.
>
> Looking at niu it already has a problem with its handling of location because
> it does
>
> u16 idx;
>
> idx = nfc->fs.location;
>
> i.e., it disregards the upper 16 bits of location. With the current code niu
> would return -EINVAL for the two new constants, which seems correct.
Actually it looks like this is going to trigger multiple bugs.
Specifically since the upper 16 bits are being discarded it is possible
to specify a value such as 0x10001 hex and it will misread that as
0x0001. This is something that will probably need to be fixed in niu.
>>
>> This is why I was suggesting that the best approach would be to update
>> the kernel to add a separate ioctl for letting the driver setup the
>> location. We can just attempt to make that call and when we get the
>> EOPNOTSUPP errno we know the device driver doesn't support it and can
>> then let the rule manager take over.
>
> The problem with this is the location is dependent on the type of filter being
> added. I.e., the ioctl would need to get all the information the existing
> ioctl carries making the new ioctl a small superset of the current one.
> Additionally, if the driver only allocates a location in a separate ioctl how
> does it know that the app is actually going to use it?
It doesn't know that the application is actually going to use it. What
should happen is that the location should be verified by the driver when
it is used in the rule insertion call. After all it is fully possible
for the user to specify a location out of range since the insert call
does no validation in ethtool if the user specified the location. That
responsibility now lies with the driver.
Thanks,
Alex
^ permalink raw reply
* Re: Divide error in bictcp_cong_avoid ?
From: Randy Dunlap @ 2011-05-04 18:35 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: TB, netdev, linux-kernel
In-Reply-To: <20110504113351.4643a0c9@nehalam>
On Wed, 4 May 2011 11:33:51 -0700 Stephen Hemminger wrote:
> On Wed, 4 May 2011 10:49:40 -0700
> Randy Dunlap <rdunlap@xenotime.net> wrote:
>
> > [add cc to netdev]
> >
> >
> > On Wed, 04 May 2011 12:03:31 -0400 TB wrote:
> >
> > > We're having this issue sporadically on a few servers and this is the
> > > backtrace we get from netconsole.
> > >
> > >
> > > [28522.642419] divide error: 0000 [#1] SMP
> > > [28522.642457] last sysfs file:
> > > /sys/devices/pci0000:00/0000:00:1f.2/host2/target2:0:0/2:0:0:0/vendor
> > > [28522.642504] CPU 0
> > > [28522.642511] Modules linked in:
> > > i2c_i801
> > > i2c_core
> > > evdev
> > > button
> > > [28522.642570]
> > > [28522.642590] Pid: 0, comm: swapper Not tainted 2.6.38.5 #6 <<<<<<<<<<<<<<
> > >
> > > Supermicro X8DTH-i/6/iF/6F
> > > /
> > > X8DTH
> > >
>
> What kernel version? I suspect they are running something really old
> kernel like RHEL 5 since BIC has not been the default congestion control for
> several years.
Please see about 6 lines above.... at the "<<<<<<<<<<<".
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: Divide error in bictcp_cong_avoid ?
From: Stephen Hemminger @ 2011-05-04 18:33 UTC (permalink / raw)
To: Randy Dunlap; +Cc: TB, netdev, linux-kernel
In-Reply-To: <20110504104940.cd0d9e9a.rdunlap@xenotime.net>
On Wed, 4 May 2011 10:49:40 -0700
Randy Dunlap <rdunlap@xenotime.net> wrote:
> [add cc to netdev]
>
>
> On Wed, 04 May 2011 12:03:31 -0400 TB wrote:
>
> > We're having this issue sporadically on a few servers and this is the
> > backtrace we get from netconsole.
> >
> >
> > [28522.642419] divide error: 0000 [#1] SMP
> > [28522.642457] last sysfs file:
> > /sys/devices/pci0000:00/0000:00:1f.2/host2/target2:0:0/2:0:0:0/vendor
> > [28522.642504] CPU 0
> > [28522.642511] Modules linked in:
> > i2c_i801
> > i2c_core
> > evdev
> > button
> > [28522.642570]
> > [28522.642590] Pid: 0, comm: swapper Not tainted 2.6.38.5 #6
> >
> > Supermicro X8DTH-i/6/iF/6F
> > /
> > X8DTH
> >
What kernel version? I suspect they are running something really old
kernel like RHEL 5 since BIC has not been the default congestion control for
several years.
--
^ permalink raw reply
* RE: [ethtool PATCH] FW dump support
From: Ben Hutchings @ 2011-05-04 18:25 UTC (permalink / raw)
To: Ajit.Khaparde; +Cc: anirban.chakraborty, netdev, --no-chain-reply-to, davem
In-Reply-To: <49395329523DD64492581B505F80C86D5A5BCD3990@EXMAIL.ad.emulex.com>
On Wed, 2011-05-04 at 11:15 -0700, Ajit.Khaparde@Emulex.Com wrote:
> ________________________________________
> From: netdev-owner@vger.kernel.org [netdev-owner@vger.kernel.org] On Behalf Of Ben Hutchings [bhutchings@solarflare.com]
> Sent: Wednesday, May 04, 2011 12:40 PM
> To: anirban.chakraborty@qlogic.com
> Cc: netdev@vger.kernel.org; --no-chain-reply-to@mv.qlogic.com; davem@davemloft.net
> Subject: Re: [ethtool PATCH] FW dump support
>
> > On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
> >> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
> >>
> >> Added support to take FW dump via ethtool.
> > [...]
> >> --- a/ethtool.c
> >> +++ b/ethtool.c
> > [...]
> >> @@ -263,6 +270,12 @@ static struct option {
> >> "Get Rx ntuple filters and actions\n" },
> >> { "-P", "--show-permaddr", MODE_PERMADDR,
> >> "Show permanent hardware address" },
> >> + { "-W", "--get-dump", MODE_GET_DUMP,
> >> + "Get dump level\n" },
> >> + { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA,
> >> + "Get dump data", "FILENAME " "Name of the dump file\n" },
> >
> > The short options should only include one letter. Also the general
> > pattern is that 'get' options use lower-case letters and 'set' options
> > use upper-case letters. No, I'm not sure how best to handle a set of 3
> > options. Maybe you can combine --get-dump and --get-dump-data, making
> > the filename optional?
>
> ethtool already has "-f" option to flash/write the FW image.
> Can you use "-F" to get the FW dump data out?
> And then may be, these options can be extended to get the get/set dump levels?
Although '-F' would be nicely mnemonic, firmware update and dump are
different enough that I don't think they make sense as a pair. If a
driver implements both then there is the risk of a typo causing the
firmware image file to be overwritten with a firmware dump. I also
don't think it's possible to extend the '-f' option without breaking
compatibility with scripts that already use it.
Ben.
--
Ben Hutchings, Senior Software 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: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Alexander Duyck @ 2011-05-04 18:21 UTC (permalink / raw)
To: Ben Hutchings
Cc: Dimitris Michailidis, davem@davemloft.net, Kirsher, Jeffrey T,
netdev@vger.kernel.org
In-Reply-To: <1304532342.2926.46.camel@bwh-desktop>
On 5/4/2011 11:05 AM, Ben Hutchings wrote:
> On Wed, 2011-05-04 at 10:41 -0700, Alexander Duyck wrote:
>> On 5/4/2011 10:33 AM, Dimitris Michailidis wrote:
>>> On 05/04/2011 10:24 AM, Ben Hutchings wrote:
>>>> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote:
>>>>> On 05/03/2011 04:34 PM, Ben Hutchings wrote:
>>>>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:
>>>>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is
>>>>>>> enough knowledge to pick an appropriate slot. So I'd remove the
>>>>>>>
>>>>>>> if (loc == RX_CLS_LOC_UNSPEC)
>>>>>>>
>>>>>>> block above, let the driver pick a slot, and then pass the selected location
>>>>>>> back for ethtool to report.
>>>>>> But first we have to specify this in the ethtool API. So please propose
>>>>>> a patch to ethtool.h.
>>>>> In the past we discussed that being able to specify the first available slot or
>>>>> the last available would be useful, so something like the below?
>>>>>
>>>>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>>>>> index 4194a20..909ef79 100644
>>>>> --- a/include/linux/ethtool.h
>>>>> +++ b/include/linux/ethtool.h
>>>>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext {
>>>>> * includes the %FLOW_EXT flag.
>>>>> * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC
>>>>> * if packets should be discarded
>>>>> - * @location: Index of filter in hardware table
>>>>> + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for
>>>>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available
>>>> [...]
>>>>
>>>> I think that's reasonable. We should also explicitly state that
>>>> location determines priority, i.e. if a packet matches two filters then
>>>> the one with the lower location wins.
>>>
>>> Easy and true for a TCAM. For hashing would you use the location to decide how
>>> to order filters that fall in the same bucket?
>>>
>>
>> The problem is none of this is backwards compatible. The niu driver has
>> supported the network flow classifier rules since 2.6.30. Adding this
>> would cause all rule setups for niu to fail because these locations
>> would have to exist outside of the current rule locations.
>
> Those rule setups would have to be using some unofficial patched
> ethtool. I don't think that should be a major concern for us.
> However...
No, what I am saying is that if we were to add those locations to the
ETHTOOL_SRXCLSRLINS then niu would not work anymore as it would treat
them as invalid locations. This existing setup will at least work with
the existing niu from what I can tell. If we were to try adding rules
with locations outside of actual allowable rules then we would likely
receive an indication that we provided an invalid argument.
>> This is why I was suggesting that the best approach would be to update
>> the kernel to add a separate ioctl for letting the driver setup the
>> location.
>>
>> We can just attempt to make that call and when we get the
>> EOPNOTSUPP errno we know the device driver doesn't support it and can
>> then let the rule manager take over.
>
> How about having ETHTOOL_GRXCLSRLCNT set a flag in the 'data' field to
> indicate that the driver can assign locations? (We would have to
> specify that for compatibility with older kernels the application must
> initialise this filed to 0.)
>
> rmgr_init() would then check for this flag.
>
> I hope someone is actually going to test this on niu, as it sounds like
> that will be the only driver using a TCAM... David?
>
> Ben.
>
Honestly what I would prefer to see is a seperate call added such as an
ETHTOOL_GRSCLSRLLOC that we could pass the flow specifier to and perhaps
include first/last location call in that and then let the driver return
where it wants to drop the rule. That way we can avoid having to create
an overly complicated rule manager that can handle all the bizarre rule
ordering options that I am sure all the different network devices support.
The only reason I am not implementing this now is because there aren't
any drivers in place that would currently use it. I figure once cxgb
has a means in place of supporting flow classifier rules then Dimitris
can add the necessary code to ethtool and the kernel to allow the driver
to specify rule locations. I would prefer to avoid adding features
based on speculation of what will be needed and would like to be able to
actually see how the features will be used.
Thanks,
Alex
^ permalink raw reply
* Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Dimitris Michailidis @ 2011-05-04 18:18 UTC (permalink / raw)
To: Alexander Duyck
Cc: Ben Hutchings, davem@davemloft.net, Kirsher, Jeffrey T,
netdev@vger.kernel.org
In-Reply-To: <4DC18FB2.8060604@intel.com>
On 05/04/2011 10:41 AM, Alexander Duyck wrote:
> On 5/4/2011 10:33 AM, Dimitris Michailidis wrote:
>> On 05/04/2011 10:24 AM, Ben Hutchings wrote:
>>> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote:
>>>> On 05/03/2011 04:34 PM, Ben Hutchings wrote:
>>>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:
>>>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where
>>>>>> there is
>>>>>> enough knowledge to pick an appropriate slot. So I'd remove the
>>>>>>
>>>>>> if (loc == RX_CLS_LOC_UNSPEC)
>>>>>>
>>>>>> block above, let the driver pick a slot, and then pass the
>>>>>> selected location
>>>>>> back for ethtool to report.
>>>>> But first we have to specify this in the ethtool API. So please
>>>>> propose
>>>>> a patch to ethtool.h.
>>>> In the past we discussed that being able to specify the first
>>>> available slot or
>>>> the last available would be useful, so something like the below?
>>>>
>>>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>>>> index 4194a20..909ef79 100644
>>>> --- a/include/linux/ethtool.h
>>>> +++ b/include/linux/ethtool.h
>>>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext {
>>>> * includes the %FLOW_EXT flag.
>>>> * @ring_cookie: RX ring/queue index to deliver to, or
>>>> %RX_CLS_FLOW_DISC
>>>> * if packets should be discarded
>>>> - * @location: Index of filter in hardware table
>>>> + * @location: Index of filter in hardware table, or
>>>> %RX_CLS_FLOW_FIRST_LOC for
>>>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last
>>>> available
>>> [...]
>>>
>>> I think that's reasonable. We should also explicitly state that
>>> location determines priority, i.e. if a packet matches two filters then
>>> the one with the lower location wins.
>>
>> Easy and true for a TCAM. For hashing would you use the location to
>> decide how
>> to order filters that fall in the same bucket?
>>
>
> The problem is none of this is backwards compatible. The niu driver has
> supported the network flow classifier rules since 2.6.30. Adding this
> would cause all rule setups for niu to fail because these locations
> would have to exist outside of the current rule locations.
Looking at niu it already has a problem with its handling of location because
it does
u16 idx;
idx = nfc->fs.location;
i.e., it disregards the upper 16 bits of location. With the current code niu
would return -EINVAL for the two new constants, which seems correct.
>
> This is why I was suggesting that the best approach would be to update
> the kernel to add a separate ioctl for letting the driver setup the
> location. We can just attempt to make that call and when we get the
> EOPNOTSUPP errno we know the device driver doesn't support it and can
> then let the rule manager take over.
The problem with this is the location is dependent on the type of filter being
added. I.e., the ioctl would need to get all the information the existing
ioctl carries making the new ioctl a small superset of the current one.
Additionally, if the driver only allocates a location in a separate ioctl how
does it know that the app is actually going to use it?
^ permalink raw reply
* tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG
From: Michael S. Tsirkin @ 2011-05-04 18:18 UTC (permalink / raw)
To: Michał Mirosław; +Cc: netdev, Ben Hutchings, herbert
BTW, I just noticed that net-next spits out
many of the following when I run any VMs:
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Features changed: 0x401b4849 -> 0x40004040
device msttap0 entered promiscuous mode
br0: Dropping NETIF_F_GSO since no SG feature.
br0: port 1(msttap0) entering forwarding state
br0: port 1(msttap0) entering forwarding state
tap0: Features changed: 0x40004040 -> 0x40024849
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Features changed: 0x40024849 -> 0x40004040
br0: Dropping NETIF_F_GSO since no SG feature.
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Features changed: 0x40004040 -> 0x401b4849
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Features changed: 0x401b4849 -> 0x40004040
br0: Dropping NETIF_F_GSO since no SG feature.
My problem is not primarily with warnings:
will that linearize all packets and disable GSO
for tap and bridge? If yes it can't be good
for performance...
--
MST
^ permalink raw reply
* RE: [ethtool PATCH] FW dump support
From: Ajit.Khaparde @ 2011-05-04 18:15 UTC (permalink / raw)
To: anirban.chakraborty; +Cc: netdev, --no-chain-reply-to, davem, bhutchings
In-Reply-To: <1304530807.2926.28.camel@bwh-desktop>
________________________________________
From: netdev-owner@vger.kernel.org [netdev-owner@vger.kernel.org] On Behalf Of Ben Hutchings [bhutchings@solarflare.com]
Sent: Wednesday, May 04, 2011 12:40 PM
To: anirban.chakraborty@qlogic.com
Cc: netdev@vger.kernel.org; --no-chain-reply-to@mv.qlogic.com; davem@davemloft.net
Subject: Re: [ethtool PATCH] FW dump support
> On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
>> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>>
>> Added support to take FW dump via ethtool.
> [...]
>> --- a/ethtool.c
>> +++ b/ethtool.c
> [...]
>> @@ -263,6 +270,12 @@ static struct option {
>> "Get Rx ntuple filters and actions\n" },
>> { "-P", "--show-permaddr", MODE_PERMADDR,
>> "Show permanent hardware address" },
>> + { "-W", "--get-dump", MODE_GET_DUMP,
>> + "Get dump level\n" },
>> + { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA,
>> + "Get dump data", "FILENAME " "Name of the dump file\n" },
>
> The short options should only include one letter. Also the general
> pattern is that 'get' options use lower-case letters and 'set' options
> use upper-case letters. No, I'm not sure how best to handle a set of 3
> options. Maybe you can combine --get-dump and --get-dump-data, making
> the filename optional?
ethtool already has "-f" option to flash/write the FW image.
Can you use "-F" to get the FW dump data out?
And then may be, these options can be extended to get the get/set dump levels?
-Ajit
^ permalink raw reply
* Re: bug fix for out-of-tree igb driver
From: Jeff Kirsher @ 2011-05-04 18:12 UTC (permalink / raw)
To: Ben Greear, Carolyn Wyborny; +Cc: netdev
In-Reply-To: <4DC1959B.4010609@candelatech.com>
On Wed, May 4, 2011 at 11:06, Ben Greear <greearb@candelatech.com> wrote:
> Just in case someone is using the out-of-tree igb driver, I
> think I found a bug.
>
> The tx_queue_mapping method should have r_idx >= adapter->num_tx_queues
> as shown below. The driver had: r_idx > adapter->num_tx_queues originally.
>
> Its possible my code is buggy for sending queue_mapping of 1
> when num_tx_queues is 1, but either way, the driver
> shouldn't crash.
>
> Upstream igb driver in mainline kernels does not have this problem.
>
> static inline struct igb_ring *igb_tx_queue_mapping(struct igb_adapter
> *adapter,
> struct sk_buff *skb)
> {
> unsigned int r_idx = skb->queue_mapping & (IGB_MAX_TX_QUEUES - 1);
>
> if (r_idx >= adapter->num_tx_queues)
> r_idx = r_idx % adapter->num_tx_queues;
>
> return adapter->tx_ring[r_idx];
> }
>
> Thanks,
> Ben
>
Thanks Ben, I have copied Carolyn so that she is aware of it.
--
Cheers,
Jeff
^ permalink raw reply
* Re: 2.6.38.2, kernel panic, probably related to framentation handling
From: Eric Dumazet @ 2011-05-04 18:11 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev
In-Reply-To: <1304528581.32152.15.camel@edumazet-laptop>
Le mercredi 04 mai 2011 à 19:03 +0200, Eric Dumazet a écrit :
> Hi Denys
>
> Is it reproductible, and possibly on latest kernel ?
>
> We fixed some bugs lately (assuming you also use a bridge ?)
>
> Could you send the disassembled code on your kernel of icmp_send() ?
Oh well, I think I found the problem, I am working on a patch and send
it shortly.
Thanks
^ permalink raw reply
* Re: 2.6.38.2, kernel panic, probably related to framentation handling
From: Denys Fedoryshchenko @ 2011-05-04 18:09 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1304528581.32152.15.camel@edumazet-laptop>
On Wed, 04 May 2011 19:03:01 +0200, Eric Dumazet wrote:
> Le mercredi 04 mai 2011 à 14:36 +0300, Denys Fedoryshchenko a écrit :
>> Seems once more, during trying to bring another type of tunnel (this
>> time userspace, working over tun device) and switching routes got
>> one
>> more kernel panic
>> It is vanilla kernel, but many source routing rules, firewall, QoS
>> and
>> etc, including this tunnel now also. Here is what i got on
>> netconsole:
>> Any other info required?
>>
>> netc [1192230.881002]
>> netc [1192230.881002] Pid: 0, comm: kworker/0:1 Not tainted
>> 2.6.38.2-devel2 #2
>> netc
>> netc Dell Inc. PowerEdge 1950
>> netc /
>> netc 0D8635
>> netc
>> netc [1192230.881002] EIP: 0060:[<c03c0847>] EFLAGS: 00010206 CPU:
>> 3
>> netc [1192230.881002] EIP is at icmp_send+0x39/0x396
>> netc [1192230.881002] EAX: 121a8aca EBX: d1d28600 ECX: 00000001
>> EDX:
>> c63b6600
>> netc [1192230.881002] ESI: d1d28600 EDI: c33438a0 EBP: f2a41840
>> ESP:
>> f64b5e8c
>> netc [1192230.881002] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
>> netc [1192230.881002] Process kworker/0:1 (pid: 0, ti=f64b4000
>> task=f64a4a80 task.ti=f64b0000)
>> netc [1192230.881002] Stack:
>> netc [1192230.881002] c0113ea1
>> netc 00000001
>> netc 0000000b
>> netc 00000000
>> netc efed48c7
>> netc 0000114a
>> netc 00000000
>> netc f6b01fa8
>> netc
>> netc [1192230.881002] e21be5e1
>> netc c0148bf3
>> netc e217d5d0
>> netc 00043c53
>> netc 00000001
>> netc f6b02d14
>> netc e21be5e1
>> netc 00043c53
>> netc
>> netc [1192230.881002] e21be5e1
>> netc 00043c53
>> netc c0148d2e
>> netc 00000000
>> netc 00000058
>> netc 00000000
>> netc c0140779
>> netc ce9f5aa9
>> netc
>> netc [1192230.881002] Call Trace:
>> netc [1192230.881002] [<c0113ea1>] ? lapic_next_event+0x13/0x16
>> netc [1192230.881002] [<c0148bf3>] ?
>> tick_dev_program_event+0x26/0x116
>> netc [1192230.881002] [<c0148d2e>] ? tick_program_event+0x1b/0x1f
>> netc [1192230.881002] [<c0140779>] ? hrtimer_interrupt+0x10c/0x1ca
>> netc [1192230.881002] [<c0140e49>] ? hrtimer_start+0x20/0x25
>> netc [1192230.881002] [<c012f18e>] ? irq_exit+0x36/0x59
>> netc [1192230.881002] [<c0114933>] ?
>> smp_apic_timer_interrupt+0x71/0x7d
>> netc [1192230.881002] [<c03f2752>] ?
>> apic_timer_interrupt+0x2a/0x30
>> netc [1192230.881002] [<c039f527>] ? ip_expire+0xf2/0x11b
>> netc [1192230.881002] [<c039f435>] ? ip_expire+0x0/0x11b
>> netc [1192230.881002] [<c0133421>] ? run_timer_softirq+0x140/0x1c7
>> netc [1192230.881002] [<c012f28f>] ? __do_softirq+0x6b/0x104
>> netc [1192230.881002] [<c012f224>] ? __do_softirq+0x0/0x104
>> netc [1192230.881002] [<c012f224>] ? __do_softirq+0x0/0x104
>> netc [1192230.881002] <IRQ>
>> netc
>> netc [1192230.881002] [<c012f17e>] ? irq_exit+0x26/0x59
>> netc [1192230.881002] [<c0103b3d>] ? do_IRQ+0x81/0x95
>> netc [1192230.881002] [<c0114933>] ?
>> smp_apic_timer_interrupt+0x71/0x7d
>> netc [1192230.881002] [<c0102ca9>] ? common_interrupt+0x29/0x30
>> netc [1192230.881002] [<c010807a>] ? mwait_idle+0x51/0x56
>> netc [1192230.881002] [<c0101a97>] ? cpu_idle+0x41/0x5e
>> netc [1192230.881002] Code:
>> netc 08
>> netc 89
>> netc c6
>> netc 89
>> netc 4c
>> netc 24
>> netc 04
>> netc 8b
>> netc 40
>> netc 48
>> netc 89
>> netc c2
>> netc 83
>> netc e2
>> netc fe
>> netc 0f
>> netc 84
>> netc 66
>> netc 03
>> netc 00
>> netc 00
>> netc 89
>> netc 94
>> netc 24
>> netc c0
>> netc 00
>> netc 00
>> netc 00
>> netc 8b
>> netc 42
>> netc 0c
>> netc 8b
>> netc be
>> netc 94
>> netc 00
>> netc 00
>> netc 00
>> netc 3b
>> netc be
>> netc a4
>> netc 00
>> netc 00
>> netc 00
>> May 4 11:17:12 217.151.224.119 unparseable log message: "<8b> "
>> netc 80
>> netc 80
>> netc 02
>> netc 00
>> netc 00
>> netc 89
>> netc 44
>> netc 24
>> netc 10
>> netc 0f
>> netc 82
>> netc 40
>> netc 03
>> netc 00
>> netc 00
>> netc 8d
>> netc 47
>> netc 14
>> netc 39
>> netc 86
>> netc
>> netc [1192230.881002] EIP: [<c03c0847>]
>> netc icmp_send+0x39/0x396
>> netc SS:ESP 0068:f64b5e8c
>> netc [1192230.881002] CR2: 00000000121a8d4a
>> netc [1192230.910072] ---[ end trace 42aae79d7fb08725 ]---
>> netc [1192230.910354] Kernel panic - not syncing: Fatal exception
>> in
>> interrupt
>> netc [1192230.911062] Rebooting in 5 seconds..
>>
>
> Hi Denys
>
> Is it reproductible, and possibly on latest kernel ?
>
> We fixed some bugs lately (assuming you also use a bridge ?)
>
> Could you send the disassembled code on your kernel of icmp_send() ?
>
> Thanks !
No bridge. Tunnel was used in TUN mode, not TAP.
I will try to reproduce at night on latest kernel.
^ permalink raw reply
* Re: [ethtool PATCH] FW dump support
From: Ben Hutchings @ 2011-05-04 18:07 UTC (permalink / raw)
To: anirban.chakraborty; +Cc: netdev, --no-chain-reply-to, davem
In-Reply-To: <1304530807.2926.28.camel@bwh-desktop>
One more thing: please can you update the manual page as well as the
built-in usage information.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH -next] x86/net: only select BPF_JIT when NET is enabled
From: David Miller @ 2011-05-04 18:06 UTC (permalink / raw)
To: randy.dunlap; +Cc: sfr, netdev, linux-next, linux-kernel
In-Reply-To: <20110504095651.60082594.randy.dunlap@oracle.com>
From: Randy Dunlap <randy.dunlap@oracle.com>
Date: Wed, 4 May 2011 09:56:51 -0700
> From: Randy Dunlap <randy.dunlap@oracle.com>
>
> Fix kconfig unmet dependency warning: HAVE_BPF_JIT depends on NET, so
> make the "select" of it depend on NET also.
>
> warning: (X86) selects HAVE_BPF_JIT which has unmet direct dependencies (NET)
>
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
Applied, thanks Randy.
^ permalink raw reply
* bug fix for out-of-tree igb driver
From: Ben Greear @ 2011-05-04 18:06 UTC (permalink / raw)
To: netdev
Just in case someone is using the out-of-tree igb driver, I
think I found a bug.
The tx_queue_mapping method should have r_idx >= adapter->num_tx_queues
as shown below. The driver had: r_idx > adapter->num_tx_queues originally.
Its possible my code is buggy for sending queue_mapping of 1
when num_tx_queues is 1, but either way, the driver
shouldn't crash.
Upstream igb driver in mainline kernels does not have this problem.
static inline struct igb_ring *igb_tx_queue_mapping(struct igb_adapter *adapter,
struct sk_buff *skb)
{
unsigned int r_idx = skb->queue_mapping & (IGB_MAX_TX_QUEUES - 1);
if (r_idx >= adapter->num_tx_queues)
r_idx = r_idx % adapter->num_tx_queues;
return adapter->tx_ring[r_idx];
}
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Ben Hutchings @ 2011-05-04 18:05 UTC (permalink / raw)
To: Alexander Duyck
Cc: Dimitris Michailidis, davem@davemloft.net, Kirsher, Jeffrey T,
netdev@vger.kernel.org
In-Reply-To: <4DC18FB2.8060604@intel.com>
On Wed, 2011-05-04 at 10:41 -0700, Alexander Duyck wrote:
> On 5/4/2011 10:33 AM, Dimitris Michailidis wrote:
> > On 05/04/2011 10:24 AM, Ben Hutchings wrote:
> >> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote:
> >>> On 05/03/2011 04:34 PM, Ben Hutchings wrote:
> >>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:
> >>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is
> >>>>> enough knowledge to pick an appropriate slot. So I'd remove the
> >>>>>
> >>>>> if (loc == RX_CLS_LOC_UNSPEC)
> >>>>>
> >>>>> block above, let the driver pick a slot, and then pass the selected location
> >>>>> back for ethtool to report.
> >>>> But first we have to specify this in the ethtool API. So please propose
> >>>> a patch to ethtool.h.
> >>> In the past we discussed that being able to specify the first available slot or
> >>> the last available would be useful, so something like the below?
> >>>
> >>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> >>> index 4194a20..909ef79 100644
> >>> --- a/include/linux/ethtool.h
> >>> +++ b/include/linux/ethtool.h
> >>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext {
> >>> * includes the %FLOW_EXT flag.
> >>> * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC
> >>> * if packets should be discarded
> >>> - * @location: Index of filter in hardware table
> >>> + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for
> >>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available
> >> [...]
> >>
> >> I think that's reasonable. We should also explicitly state that
> >> location determines priority, i.e. if a packet matches two filters then
> >> the one with the lower location wins.
> >
> > Easy and true for a TCAM. For hashing would you use the location to decide how
> > to order filters that fall in the same bucket?
> >
>
> The problem is none of this is backwards compatible. The niu driver has
> supported the network flow classifier rules since 2.6.30. Adding this
> would cause all rule setups for niu to fail because these locations
> would have to exist outside of the current rule locations.
Those rule setups would have to be using some unofficial patched
ethtool. I don't think that should be a major concern for us.
However...
> This is why I was suggesting that the best approach would be to update
> the kernel to add a separate ioctl for letting the driver setup the
> location.
>
> We can just attempt to make that call and when we get the
> EOPNOTSUPP errno we know the device driver doesn't support it and can
> then let the rule manager take over.
How about having ETHTOOL_GRXCLSRLCNT set a flag in the 'data' field to
indicate that the driver can assign locations? (We would have to
specify that for compatibility with older kernels the application must
initialise this filed to 0.)
rmgr_init() would then check for this flag.
I hope someone is actually going to test this on niu, as it sounds like
that will be the only driver using a TCAM... David?
Ben.
--
Ben Hutchings, Senior Software 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: [ethtool PATCH] FW dump support
From: Anirban Chakraborty @ 2011-05-04 18:02 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, David Miller
In-Reply-To: <1304530807.2926.28.camel@bwh-desktop>
On May 4, 2011, at 10:40 AM, Ben Hutchings wrote:
> On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
>> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>>
>> Added support to take FW dump via ethtool.
> [...]
>> --- a/ethtool.c
>> +++ b/ethtool.c
> [...]
>> @@ -263,6 +270,12 @@ static struct option {
>> "Get Rx ntuple filters and actions\n" },
>> { "-P", "--show-permaddr", MODE_PERMADDR,
>> "Show permanent hardware address" },
>> + { "-W", "--get-dump", MODE_GET_DUMP,
>> + "Get dump level\n" },
>> + { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA,
>> + "Get dump data", "FILENAME " "Name of the dump file\n" },
>
> The short options should only include one letter. Also the general
> pattern is that 'get' options use lower-case letters and 'set' options
> use upper-case letters. No, I'm not sure how best to handle a set of 3
> options. Maybe you can combine --get-dump and --get-dump-data, making
> the filename optional?
Thanks for the info, I was not aware of it. Will address it.
>
>> + { "-w", "--set-dump", MODE_SET_DUMP,
>> + "Set dump level", "DUMPLEVEL " "Dump level for the device\n" },
>
> The field this sets is described as 'flags' so does it consist of flags
> or is it a level?
The idea is to have an opaque flag for the driver that it uses to set the dump level
(and hence the size) of it. I will use flag instead of level here so that there is no
ambiguity.
>
> [...]
>> @@ -3241,6 +3270,86 @@ static int do_grxntuple(int fd, struct ifreq *ifr)
>> return 0;
>> }
>>
>> +static void do_writedump(struct ethtool_dump *dump)
>> +{
>> + FILE *f = fopen(dump_file, "wb+");
>> + size_t bytes;
>> +
>> + if (!f ) {
>> + fprintf(stderr, "Can't open file %s: %s\n",
>> + dump_file, strerror(errno));
>> + return;
>
> On error, we must exit with code 1.
Will do.
>
>> + }
>> +
>> + bytes = fwrite(dump->data, 1, dump->len, f);
>> + fclose(f);
> [...]
>
> These functions can also fail and need to be checked. (Yes, fclose()
> can fail, since it may have to flush buffered data.)
I agree. Will fix it. Thanks for the feedback.
-Anirban
^ permalink raw reply
* Re: Support e1000 M88 PHY registers in -d
From: Jeff Kirsher @ 2011-05-04 18:00 UTC (permalink / raw)
To: Ben Hutchings
Cc: Anthony DeRobertis, netdev, e1000-devel@lists.sourceforge.net,
574574@bugs.debian.org
In-Reply-To: <1304530875.2926.29.camel@bwh-desktop>
[-- Attachment #1: Type: text/plain, Size: 1474 bytes --]
On Wed, 2011-05-04 at 10:41 -0700, Ben Hutchings wrote:
> On Mon, 2011-04-04 at 10:41 +0100, Ben Hutchings wrote:
> > On Mon, 2011-04-04 at 01:36 -0700, Jeff Kirsher wrote:
> > > On Sat, Apr 2, 2011 at 09:24, Ben Hutchings <bhutchings@solarflare.com> wrote:
> > > > Anthony,
> > > >
> > > > I'm now upstream maintainer for ethtool so I've picked up your patch
> > > > again.
> > > >
> > > > On Fri, 2010-03-19 at 00:32 -0400, Anthony DeRobertis wrote:
> > > >> Package: ethtool
> > > >> Version: 1:2.6.33-1
> > > >> Severity: wishlist
> > > >>
> > > >> The M88 PHY registers contain useful information like the cable length
> > > >> estimate and the MDI/MDIX status. The attached patch makes -d dump
> > > >> them.
> > > >
> > > > Patches for ethtool should include a commit message and Signed-off-by
> > > > line, as in the Linux kernel. See sections 2 and 12 of
> > > > <http://www.kernel.org/doc/Documentation/SubmittingPatches>. They
> > > > should be sent to this address and to netdev.
> > > >
> > > > I'm forwarding this patch to netdev and the e1000 developers for review.
> > > >
> > > > Ben.
> > >
> > > Thanks Ben. Just to be clear, have you applied these e1000 changes to
> > > the ethtool?
> > [...]
> >
> > I have not applied these changes either anywhere.
>
> ...but I will if I don't hear back from you soon.
>
> Ben.
>
Go ahead and apply the changes. I apologize, I thought I had already
pushed this patch.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: Divide error in bictcp_cong_avoid ?
From: Randy Dunlap @ 2011-05-04 17:49 UTC (permalink / raw)
To: TB, netdev; +Cc: linux-kernel
In-Reply-To: <4DC178D3.6030308@techboom.com>
[add cc to netdev]
On Wed, 04 May 2011 12:03:31 -0400 TB wrote:
> We're having this issue sporadically on a few servers and this is the
> backtrace we get from netconsole.
>
>
> [28522.642419] divide error: 0000 [#1] SMP
> [28522.642457] last sysfs file:
> /sys/devices/pci0000:00/0000:00:1f.2/host2/target2:0:0/2:0:0:0/vendor
> [28522.642504] CPU 0
> [28522.642511] Modules linked in:
> i2c_i801
> i2c_core
> evdev
> button
> [28522.642570]
> [28522.642590] Pid: 0, comm: swapper Not tainted 2.6.38.5 #6
>
> Supermicro X8DTH-i/6/iF/6F
> /
> X8DTH
>
> [28522.642651] RIP: 0010:[<ffffffff8150b27b>] [<ffffffff8150b27b>]
> bictcp_cong_avoid+0x21a/0x247
> [28522.642708] RSP: 0018:ffff8800bf403a90 EFLAGS: 00010202
> [28522.642735] RAX: 0000000000000010 RBX: ffff880352aa6400 RCX:
> 0000000000000000
> [28522.642765] RDX: 0000000000000000 RSI: ffff880352aa67c0 RDI:
> 0000000000001607
> [28522.642795] RBP: 000000007caa5a1b R08: 00000000000035c2 R09:
> 00000000000000e6
> [28522.642825] R10: ffff88003d499c00 R11: ffff880109831b00 R12:
> ffffffff817cecd0
> [28522.642855] R13: 0000000000000004 R14: 000000000001001b R15:
> 0000000000000123
> [28522.642886] FS: 0000000000000000(0000) GS:ffff8800bf400000(0000)
> knlGS:0000000000000000
> [28522.642932] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [28522.642959] CR2: 00007fb4c6ffd000 CR3: 000000042e4a1000 CR4:
> 00000000000006f0
> [28522.642990] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [28522.643020] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
> 0000000000000400
> [28522.643050] Process swapper (pid: 0, threadinfo ffffffff8176c000,
> task ffffffff81777020)
> [28522.643095] Stack:
> [28522.643116] ffff880352aa6400
> ffffffff817cecd0
> 0000000000000004
> 0000000000000406
>
> [28522.643171] ffff880352aa6400
> ffffffff814e3dc5
> 000000000000111c
> ffff88022a9b3200
>
> [28522.643226] 0000000000000000
> 7caa5a1b7caa4ec3
> 0000000000000000
> 0000000000000000
>
> [28522.643281] Call Trace:
> [28522.643303] <IRQ>
>
> [28522.643330] [<ffffffff814e3dc5>] ? tcp_ack+0x18b5/0x1a89
> [28522.643359] [<ffffffff814e45c2>] ? tcp_rcv_established+0xd1/0xa13
> [28522.643389] [<ffffffff814ec60b>] ? tcp_v4_do_rcv+0x1b2/0x382
> [28522.643418] [<ffffffff814c95d4>] ? nf_iterate+0x40/0x78
> [28522.643446] [<ffffffff814ecc5f>] ? tcp_v4_rcv+0x484/0x797
> [28522.643475] [<ffffffff814d11c7>] ? ip_local_deliver_finish+0xab/0x139
> [28522.643505] [<ffffffff814ae2b3>] ? __netif_receive_skb+0x31c/0x349
> [28522.643535] [<ffffffff814aec82>] ? netif_receive_skb+0x67/0x6d
> [28522.643564] [<ffffffff814af1fb>] ? napi_gro_receive+0x9d/0xab
> [28522.643592] [<ffffffff814aed57>] ? napi_skb_finish+0x1c/0x31
> [28522.643623] [<ffffffff813e4248>] ? igb_poll+0x7d5/0xb2e
> [28522.643653] [<ffffffff812b6b22>] ? blk_run_queue+0x23/0x37
> [28522.643683] [<ffffffff813520d4>] ? scsi_run_queue+0x2ee/0x381
> [28522.643712] [<ffffffff81353810>] ? scsi_io_completion+0x3e0/0x409
> [28522.643741] [<ffffffff814af337>] ? net_rx_action+0xa7/0x212
> [28522.643771] [<ffffffff8103b6c2>] ? __do_softirq+0xbe/0x184
> [28522.643800] [<ffffffff8100364c>] ? call_softirq+0x1c/0x28
> [28522.643828] [<ffffffff81005085>] ? do_softirq+0x31/0x63
> [28522.643856] [<ffffffff8103b56c>] ? irq_exit+0x36/0x78
> [28522.643883] [<ffffffff81004784>] ? do_IRQ+0x98/0xae
> [28522.643912] [<ffffffff81562c13>] ? ret_from_intr+0x0/0xe
> [28522.643938] <EOI>
>
> [28522.643963] [<ffffffff81009a41>] ? mwait_idle+0xb9/0xf3
> [28522.643991] [<ffffffff81001c6e>] ? cpu_idle+0x57/0x8d
> [28522.644019] [<ffffffff81801c49>] ? start_kernel+0x34e/0x35a
> [28522.644048] [<ffffffff81801398>] ? x86_64_start_kernel+0xf3/0xf9
> [28522.644075] Code:
> 39 c9 76 18 44 29 c9 31 d2 44 89 c8 f7 f1 39 83 c0 03
> 00 00
> 76 06 89 83 c0 03 00 00 8b 83 c0 03 00 00 31 d2 c1 e0
> 04 0f
> b7 4e 2c
> f7> f1 ba 01 00 00 00 85 c0 0f 45 d0 89 93 c0 03 00
> 00 8b b3
> c0
>
> [28522.644338] RIP
> [<ffffffff8150b27b>] bictcp_cong_avoid+0x21a/0x247
> [28522.644371] RSP <ffff8800bf403a90>
> [28522.644733] ---[ end trace 9db294ef7ff3a7b5 ]---
> [28522.644800] Kernel panic - not syncing: Fatal exception in interrupt
> [28522.644871] Pid: 0, comm: swapper Tainted: G D 2.6.38.5 #6
> [28522.644942] Call Trace:
> [28522.645012] <IRQ>
> [<ffffffff81560690>] ? panic+0x9d/0x1a0
> [28522.645131] [<ffffffff81562c13>] ? ret_from_intr+0x0/0xe
> [28522.645200] [<ffffffff810365bb>] ? kmsg_dump+0x46/0xec
> [28522.645268] [<ffffffff81006176>] ? oops_end+0x9f/0xac
> [28522.645335] [<ffffffff810040d8>] ? do_divide_error+0x7f/0x89
> [28522.645404] [<ffffffff8150b27b>] ? bictcp_cong_avoid+0x21a/0x247
> [28522.645473] [<ffffffff814b057c>] ? dev_queue_xmit+0x4a4/0x4b2
> [28522.645545] [<ffffffff814d5390>] ? ip_queue_xmit+0x2e9/0x32f
> [28522.645614] [<ffffffff81003375>] ? divide_error+0x15/0x20
> [28522.645685] [<ffffffff8150b27b>] ? bictcp_cong_avoid+0x21a/0x247
> [28522.645754] [<ffffffff814e3dc5>] ? tcp_ack+0x18b5/0x1a89
> [28522.645823] [<ffffffff814e45c2>] ? tcp_rcv_established+0xd1/0xa13
> [28522.645892] [<ffffffff814ec60b>] ? tcp_v4_do_rcv+0x1b2/0x382
> [28522.645961] [<ffffffff814c95d4>] ? nf_iterate+0x40/0x78
> [28522.646029] [<ffffffff814ecc5f>] ? tcp_v4_rcv+0x484/0x797
> [28522.646097] [<ffffffff814d11c7>] ? ip_local_deliver_finish+0xab/0x139
> [28522.646167] [<ffffffff814ae2b3>] ? __netif_receive_skb+0x31c/0x349
> [28522.646240] [<ffffffff814aec82>] ? netif_receive_skb+0x67/0x6d
> [28522.646308] [<ffffffff814af1fb>] ? napi_gro_receive+0x9d/0xab
> [28522.646377] [<ffffffff814aed57>] ? napi_skb_finish+0x1c/0x31
> [28522.646445] [<ffffffff813e4248>] ? igb_poll+0x7d5/0xb2e
> [28522.646513] [<ffffffff812b6b22>] ? blk_run_queue+0x23/0x37
> [28522.646582] [<ffffffff813520d4>] ? scsi_run_queue+0x2ee/0x381
> [28522.646651] [<ffffffff81353810>] ? scsi_io_completion+0x3e0/0x409
> [28522.646721] [<ffffffff814af337>] ? net_rx_action+0xa7/0x212
> [28522.646791] [<ffffffff8103b6c2>] ? __do_softirq+0xbe/0x184
> [28522.646884] [<ffffffff8100364c>] ? call_softirq+0x1c/0x28
> [28522.646953] [<ffffffff81005085>] ? do_softirq+0x31/0x63
> [28522.647021] [<ffffffff8103b56c>] ? irq_exit+0x36/0x78
> [28522.647089] [<ffffffff81004784>] ? do_IRQ+0x98/0xae
> [28522.647164] [<ffffffff81562c13>] ? ret_from_intr+0x0/0xe
> [28522.647239] <EOI>
> [<ffffffff81009a41>] ? mwait_idle+0xb9/0xf3
> [28522.647354] [<ffffffff81001c6e>] ? cpu_idle+0x57/0x8d
> [28522.647422] [<ffffffff81801c49>] ? start_kernel+0x34e/0x35a
> [28522.647491] [<ffffffff81801398>] ? x86_64_start_kernel+0xf3/0xf9
> --
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: Support e1000 M88 PHY registers in -d
From: Ben Hutchings @ 2011-05-04 17:41 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: Anthony DeRobertis, netdev, e1000-devel, 574574
In-Reply-To: <1301910102.2935.52.camel@localhost>
On Mon, 2011-04-04 at 10:41 +0100, Ben Hutchings wrote:
> On Mon, 2011-04-04 at 01:36 -0700, Jeff Kirsher wrote:
> > On Sat, Apr 2, 2011 at 09:24, Ben Hutchings <bhutchings@solarflare.com> wrote:
> > > Anthony,
> > >
> > > I'm now upstream maintainer for ethtool so I've picked up your patch
> > > again.
> > >
> > > On Fri, 2010-03-19 at 00:32 -0400, Anthony DeRobertis wrote:
> > >> Package: ethtool
> > >> Version: 1:2.6.33-1
> > >> Severity: wishlist
> > >>
> > >> The M88 PHY registers contain useful information like the cable length
> > >> estimate and the MDI/MDIX status. The attached patch makes -d dump
> > >> them.
> > >
> > > Patches for ethtool should include a commit message and Signed-off-by
> > > line, as in the Linux kernel. See sections 2 and 12 of
> > > <http://www.kernel.org/doc/Documentation/SubmittingPatches>. They
> > > should be sent to this address and to netdev.
> > >
> > > I'm forwarding this patch to netdev and the e1000 developers for review.
> > >
> > > Ben.
> >
> > Thanks Ben. Just to be clear, have you applied these e1000 changes to
> > the ethtool?
> [...]
>
> I have not applied these changes either anywhere.
...but I will if I don't hear back from you soon.
Ben.
--
Ben Hutchings, Senior Software 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: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Alexander Duyck @ 2011-05-04 17:41 UTC (permalink / raw)
To: Dimitris Michailidis
Cc: Ben Hutchings, davem@davemloft.net, Kirsher, Jeffrey T,
netdev@vger.kernel.org
In-Reply-To: <4DC18DF8.3090707@chelsio.com>
On 5/4/2011 10:33 AM, Dimitris Michailidis wrote:
> On 05/04/2011 10:24 AM, Ben Hutchings wrote:
>> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote:
>>> On 05/03/2011 04:34 PM, Ben Hutchings wrote:
>>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:
>>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is
>>>>> enough knowledge to pick an appropriate slot. So I'd remove the
>>>>>
>>>>> if (loc == RX_CLS_LOC_UNSPEC)
>>>>>
>>>>> block above, let the driver pick a slot, and then pass the selected location
>>>>> back for ethtool to report.
>>>> But first we have to specify this in the ethtool API. So please propose
>>>> a patch to ethtool.h.
>>> In the past we discussed that being able to specify the first available slot or
>>> the last available would be useful, so something like the below?
>>>
>>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>>> index 4194a20..909ef79 100644
>>> --- a/include/linux/ethtool.h
>>> +++ b/include/linux/ethtool.h
>>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext {
>>> * includes the %FLOW_EXT flag.
>>> * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC
>>> * if packets should be discarded
>>> - * @location: Index of filter in hardware table
>>> + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for
>>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available
>> [...]
>>
>> I think that's reasonable. We should also explicitly state that
>> location determines priority, i.e. if a packet matches two filters then
>> the one with the lower location wins.
>
> Easy and true for a TCAM. For hashing would you use the location to decide how
> to order filters that fall in the same bucket?
>
The problem is none of this is backwards compatible. The niu driver has
supported the network flow classifier rules since 2.6.30. Adding this
would cause all rule setups for niu to fail because these locations
would have to exist outside of the current rule locations.
This is why I was suggesting that the best approach would be to update
the kernel to add a separate ioctl for letting the driver setup the
location. We can just attempt to make that call and when we get the
EOPNOTSUPP errno we know the device driver doesn't support it and can
then let the rule manager take over.
Thanks,
Alex
^ permalink raw reply
* Re: [ethtool PATCH] FW dump support
From: Ben Hutchings @ 2011-05-04 17:40 UTC (permalink / raw)
To: anirban.chakraborty; +Cc: netdev, --no-chain-reply-to, davem
In-Reply-To: <1304378957-24123-1-git-send-email-anirban.chakraborty@qlogic.com>
On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>
> Added support to take FW dump via ethtool.
[...]
> --- a/ethtool.c
> +++ b/ethtool.c
[...]
> @@ -263,6 +270,12 @@ static struct option {
> "Get Rx ntuple filters and actions\n" },
> { "-P", "--show-permaddr", MODE_PERMADDR,
> "Show permanent hardware address" },
> + { "-W", "--get-dump", MODE_GET_DUMP,
> + "Get dump level\n" },
> + { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA,
> + "Get dump data", "FILENAME " "Name of the dump file\n" },
The short options should only include one letter. Also the general
pattern is that 'get' options use lower-case letters and 'set' options
use upper-case letters. No, I'm not sure how best to handle a set of 3
options. Maybe you can combine --get-dump and --get-dump-data, making
the filename optional?
> + { "-w", "--set-dump", MODE_SET_DUMP,
> + "Set dump level", "DUMPLEVEL " "Dump level for the device\n" },
The field this sets is described as 'flags' so does it consist of flags
or is it a level?
[...]
> @@ -3241,6 +3270,86 @@ static int do_grxntuple(int fd, struct ifreq *ifr)
> return 0;
> }
>
> +static void do_writedump(struct ethtool_dump *dump)
> +{
> + FILE *f = fopen(dump_file, "wb+");
> + size_t bytes;
> +
> + if (!f ) {
> + fprintf(stderr, "Can't open file %s: %s\n",
> + dump_file, strerror(errno));
> + return;
On error, we must exit with code 1.
> + }
> +
> + bytes = fwrite(dump->data, 1, dump->len, f);
> + fclose(f);
[...]
These functions can also fail and need to be checked. (Yes, fclose()
can fail, since it may have to flush buffered data.)
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] net: add mac_pton() for parsing MAC address
From: Alexey Dobriyan @ 2011-05-04 17:39 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: davem, netdev
In-Reply-To: <20110504081225.267a0833@nehalam>
On Wed, May 04, 2011 at 08:12:25AM -0700, Stephen Hemminger wrote:
> On Wed, 4 May 2011 09:15:51 +0300
> Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> > +int mac_pton(const char *s, u8 *mac)
> > +{
> > + int i;
> > +
> > + /* XX:XX:XX:XX:XX:XX */
> > + if (strlen(s) < 3 * ETH_ALEN - 1)
> > + return 0;
> > +
> > + /* Don't half dirty result. */
> Shouldn't this be "Don't allow dirty result."?
Maybe.
It means "only dirty result, if everything is OK." like inet_pton(3).
> > + for (i = 0; i < ETH_ALEN; i++) {
> > + if (!strchr("0123456789abcdefABCDEF", s[i * 3]))
> > + return 0;
> > + if (!strchr("0123456789abcdefABCDEF", s[i * 3 + 1]))
> > + return 0;
>
> if (!isxdigit(s[i*3]) || !isxdigit(s[i*3+1]))
> return 0;
>
> > + if (i != ETH_ALEN - 1 && s[i * 3 + 2] != ':')
> > + return 0;
> > + }
> > + for (i = 0; i < ETH_ALEN; i++) {
> > + mac[i] = (hex_to_bin(s[i * 3]) << 4) | hex_to_bin(s[i * 3 + 1]);
> hex2bin(&mac[i], &s[i*3], 1);
> > + }
> > + return 1;
> > +}
> > +EXPORT_SYMBOL(mac_pton);
>
> Also don't need two loops, okay to parse partial result.
You need two loops if code is written as sent.
Otherwise, caller need temporary buffer to not corrupt possibly important
previous MAC value.
^ 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