* Re: [patch net-next-2.6] net: convert bonding to use rx_handler
From: Patrick McHardy @ 2011-02-18 14:46 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jiri Pirko, netdev, davem, shemminger, fubar, nicolas.2p.debian,
andy
In-Reply-To: <1298039252.6201.66.camel@edumazet-laptop>
Am 18.02.2011 15:27, schrieb Eric Dumazet:
> Le vendredi 18 février 2011 à 15:14 +0100, Jiri Pirko a écrit :
>
>> Do not know how to do it better. As for percpu variable, not only
>> origdev would have to be remembered but also probably skb pointer to
>> know if it's the first run on the skb or not. Can't really figure out a
>> better solution. Can you?
>
> I'll try and let you know.
Why not simply do a lookup on skb->iif?
^ permalink raw reply
* Re: [patch net-next-2.6] net: convert bonding to use rx_handler
From: Jiri Pirko @ 2011-02-18 14:58 UTC (permalink / raw)
To: Patrick McHardy
Cc: Eric Dumazet, netdev, davem, shemminger, fubar, nicolas.2p.debian,
andy
In-Reply-To: <4D5E8655.5070304@trash.net>
Fri, Feb 18, 2011 at 03:46:45PM CET, kaber@trash.net wrote:
>Am 18.02.2011 15:27, schrieb Eric Dumazet:
>> Le vendredi 18 février 2011 à 15:14 +0100, Jiri Pirko a écrit :
>>
>>> Do not know how to do it better. As for percpu variable, not only
>>> origdev would have to be remembered but also probably skb pointer to
>>> know if it's the first run on the skb or not. Can't really figure out a
>>> better solution. Can you?
>>
>> I'll try and let you know.
>
>Why not simply do a lookup on skb->iif?
Well I was trying to avoid iterating over list of devices for each
incoming frame.
^ permalink raw reply
* Re: [PATCH v2 09/13] can: pruss CAN driver.
From: Arnd Bergmann @ 2011-02-18 15:07 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Subhasish Ghosh, davinci-linux-open-source, sachi, nsekhar,
open list, open list:CAN NETWORK DRIVERS,
open list:CAN NETWORK DRIVERS, m-watkins, Wolfgang Grandegger
In-Reply-To: <1297435892-28278-10-git-send-email-subhasish@mistralsolutions.com>
On Friday 11 February 2011, Subhasish Ghosh wrote:
> This patch adds support for the CAN device emulated on PRUSS.
Hi Subhasish,
This is a detailed walk through the can driver. The pruss_can.c
file mostly looks good, there are very tiny changes that I'm
suggesting to improve the code. I assume that you wrote that file.
The pruss_can_api.c is a bit of a mess and looks like it was copied
from some other code base and just barely changed to follow Linux
coding style. I can tell from the main driver file that you can do
better than that.
My recommendation for that would be to throw it away and reimplement
the few parts that you actually need, in proper coding style.
You can also try to fix the file according to the comments I give
you below, but I assume that would be signficantly more work.
Moving everything into one file also makes things easier to read
here and lets you identifer more quickly what is unused.
Arnd
> +#ifdef __CAN_DEBUG
> +#define __can_debug(fmt, args...) printk(KERN_DEBUG "can_debug: " fmt, ## args)
> +#else
> +#define __can_debug(fmt, args...)
> +#endif
> +#define __can_err(fmt, args...) printk(KERN_ERR "can_err: " fmt, ## args)
Better use the existing dev_dbg() and dev_err() macros that provide the
same functionality in a more standard way. You already use them in
some places, as I noticed.
If you don't have a way to pass a meaningful device, you can use
pr_debug/pr_err.
> +void omapl_pru_can_rx_wQ(struct work_struct *work)
> +{
This is only used in the same file, so better make it static.
> + if (-1 == pru_can_get_intr_status(priv->dev, &priv->can_rx_hndl))
> + return;
Don't make up your own return values, just use the standard error codes,
e.g. -EIO or -EAGAIN, whatever fits.
The more common way to write the comparison would be the other way round,
if (pru_can_get_intr_status(priv->dev, &priv->can_rx_hndl) == -EAGAIN)
return;
or, simpler
if (pru_can_get_intr_status(priv->dev, &priv->can_rx_hndl))
return;
> +irqreturn_t omapl_tx_can_intr(int irq, void *dev_id)
> +{
This also should be static
> + for (bit_set = 0; ((priv->can_tx_hndl.u32interruptstatus & 0xFF)
> + >> bit_set != 0); bit_set++)
> + ;
bit_set = fls(priv->can_tx_hndl.u32interruptstatus & 0xFF); ?
> +irqreturn_t omapl_rx_can_intr(int irq, void *dev_id)
static
> diff --git a/drivers/net/can/da8xx_pruss/pruss_can_api.c b/drivers/net/can/da8xx_pruss/pruss_can_api.c
> new file mode 100644
> index 0000000..2f7438a
> --- /dev/null
> +++ b/drivers/net/can/da8xx_pruss/pruss_can_api.c
A lot of code in this file seems to be unused. Is that right?
I would suggest adding only the code that is actually being
used. If you add more functionality later, you can always
add back the low-level functions, but dead code usually
turns into broken code quickly.
> +static can_emu_drv_inst gstr_can_inst[ecanmaxinst];
This is global data and probably needs some for of locking
> +/*
> + * pru_can_set_brp() Updates the BRP register of PRU0
> + * and PRU1 of OMAP L138. This API will be called by the
> + * Application to updtae the BRP register of PRU0 and PRU1
> + *
> + * param u16bitrateprescaler The can bus bitrate
> + * prescaler value be set
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_set_brp(struct device *dev, u16 u16bitrateprescaler)
> +{
unused.
> + u32 u32offset;
> +
> + if (u16bitrateprescaler > 255) {
> + return -1;
> + }
non-standard error code. It also doesn't match the comment, which
claims it is SUCCESS or FAILURE, both of which are (rightfully)
not defined.
> + u32offset = (PRU_CAN_RX_CLOCK_BRP_REGISTER);
> + pruss_writel(dev, u32offset, (u32 *) &u16bitrateprescaler, 1);
> +
> + u32offset = (PRU_CAN_TX_CLOCK_BRP_REGISTER);
> + pruss_writel(dev, u32offset, (u32 *) &u16bitrateprescaler, 1);
You pass a 32 bit pointer to a 16 bit local variable here.
This has an undefined effect, and if you build this code on
a big-endian platform, it cannot possibly do anything good.
pruss_writel() is defined in a funny way if it takes a thirty-two bit
input argument by reference, rather than by value. What is going
on there?
> +s16 pru_can_set_bit_timing(struct device *dev,
> + can_bit_timing_consts *pstrbittiming)
unused.
> + u32 u32offset;
> + u32 u32serregister;
It's a bit silly to put the name of the type into the name
of a variable. You already spell it out in the definition.
> +s16 pru_can_write_data_to_mailbox(struct device *dev,
> + can_emu_app_hndl *pstremuapphndl)
> +{
> + s16 s16subrtnretval;
> + u32 u32offset;
> +
> + if (pstremuapphndl == NULL) {
> + return -1;
> + }
nonstandard error code. Also, why the heck is type function
return type s16 when the only possible return values are 0
and -1? Just make this an int.
> + switch ((u8) pstremuapphndl->ecanmailboxnumber) {
> + case 0:
> + u32offset = (PRU_CAN_TX_MAILBOX0);
> + break;
> + case 1:
> + u32offset = (PRU_CAN_TX_MAILBOX1);
> + break;
> + case 2:
> + u32offset = (PRU_CAN_TX_MAILBOX2);
> + break;
> + case 3:
> + u32offset = (PRU_CAN_TX_MAILBOX3);
> + break;
> + case 4:
> + u32offset = (PRU_CAN_TX_MAILBOX4);
> + break;
> + case 5:
> + u32offset = (PRU_CAN_TX_MAILBOX5);
> + break;
> + case 6:
> + u32offset = (PRU_CAN_TX_MAILBOX6);
> + break;
> + case 7:
> + u32offset = (PRU_CAN_TX_MAILBOX7);
> + break;
> + default:
> + return -1;
> + }
Lovely switch statement. I'm sure you find a better way to express this ;-)
> + s16subrtnretval = pruss_writel(dev, u32offset,
> + (u32 *) &(pstremuapphndl->strcanmailbox), 4);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> + return 0;
> +}
return pruss_writel(...) ?
> +
> +/*
> + * pru_can_get_intr_status()
> + * Gets the interrupts status register value.
> + * This API will be called by the Application
> + * to get the interrupts status register value
> + *
> + * param u8prunumber PRU number for which IntStatusReg
> + * has to be read
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_get_intr_status(struct device *dev,
> + can_emu_app_hndl *pstremuapphndl)
> +{
> + u32 u32offset;
> + s16 s16subrtnretval = -1;
> +
> + if (pstremuapphndl == NULL) {
> + return -1;
> + }
In every function you check that pstremuapphndl is present. This seems
rather pointless. How about just making sure you never pass a NULL
value to these functions? That should not be hard at all from the
high-level driver.
> + if (pstremuapphndl->u8prunumber == DA8XX_PRUCORE_1) {
> + u32offset = (PRU_CAN_TX_INTERRUPT_STATUS_REGISTER);
> + } else if (pstremuapphndl->u8prunumber == DA8XX_PRUCORE_0) {
> + u32offset = (PRU_CAN_RX_INTERRUPT_STATUS_REGISTER);
> + } else {
> + return -1;
> + }
> +
> + s16subrtnretval = pruss_readl(dev, u32offset,
> + (u32 *) &pstremuapphndl->u32interruptstatus, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
You can also get rid of all these {} braces around one-line statements.
> +/*
> + * pru_can_get_mailbox_status() Gets the mailbox status
> + * register value. This API will be called by the Application
> + * to get the mailbox status register value
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_get_mailbox_status(struct device *dev,
> + can_emu_app_hndl *pstremuapphndl)
unused.
> +/*
> + * pru_can_config_mode_set() Sets the timing value
> + * for data transfer. This API will be called by the Application
> + * to set timing valus for data transfer
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_config_mode_set(struct device *dev, bool bconfigmodeflag)
unused.
> + u32offset = (PRU_CAN_TX_GLOBAL_CONTROL_REGISTER & 0xFFFF);
> + u32value = 0x00000000;
> + s16subrtnretval = pruss_writel(dev, u32offset, (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> +
> + u32offset = (PRU_CAN_TX_GLOBAL_STATUS_REGISTER & 0xFFFF);
> + u32value = 0x00000040;
> + s16subrtnretval = pruss_writel(dev, u32offset, (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> + u32offset = (PRU_CAN_RX_GLOBAL_STATUS_REGISTER & 0xFFFF);
> + u32value = 0x00000040;
> + s16subrtnretval = pruss_writel(dev, u32offset, (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
<skipping 50 (!) more of these>
After the third time of writing the same code, you should have noticed that
there is some duplication involved that can trivially be reduced. A good
way to express the same would be a table with the contents:
static struct pru_can_register_init {
u16 offset;
u32 value;
} = {
{ PRU_CAN_TX_GLOBAL_CONTROL_REGISTER, 0, },
{ PRU_CAN_TX_GLOBAL_STATUS_REGISTER, 0x40, },
{ PRU_CAN_RX_GLOBAL_STATUS_REGISTER, 0x40, },
...
};
> +
> +
> +/*
> + * pru_can_emu_open() Opens the can emu for
> + * application to use. This API will be called by the Application
> + * to Open the can emu for application to use.
> + *
> + * param pstremuapphndl Pointer to application handler
> + * structure
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_emu_open(struct device *dev, can_emu_app_hndl *pstremuapphndl)
unused.
> +/*
> + * brief pru_can_emu_close() Closes the can emu for other
> + * applications to use. This API will be called by the Application to Close
> + * the can emu for other applications to use
> + *
> + * param pstremuapphndl Pointer to application handler structure
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_emu_close(struct device *dev, can_emu_app_hndl *pstremuapphndl)
unused
> +s16 pru_can_emu_sreset(struct device *dev)
> +{
> + return 0;
> +}
pointless.
> +s16 pru_can_tx(struct device *dev, u8 u8mailboxnumber, u8 u8prunumber)
> +{
> + u32 u32offset = 0;
> + u32 u32value = 0;
> + s16 s16subrtnretval = -1;
> +
> + if (DA8XX_PRUCORE_1 == u8prunumber) {
> + switch (u8mailboxnumber) {
> + case 0:
> + u32offset = (PRU_CAN_TX_MAILBOX0_STATUS_REGISTER & 0xFFFF);
> + u32value = 0x00000080;
> + s16subrtnretval = pruss_writel(dev, u32offset,
> + (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> + break;
> + case 1:
> + u32offset = (PRU_CAN_TX_MAILBOX1_STATUS_REGISTER & 0xFFFF);
> + u32value = 0x00000080;
> + s16subrtnretval = pruss_writel(dev, u32offset,
> + (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> + break;
> + case 2:
> + u32offset = (PRU_CAN_TX_MAILBOX2_STATUS_REGISTER & 0xFFFF);
Another pointless switch statement.
> +s16 pru_can_mask_ints(struct device *dev, u32 int_mask)
> +{
> + return 0;
> +}
> +
> +int pru_can_get_error_cnt(struct device *dev, u8 u8prunumber)
> +{
> + return 0;
> +}
useless.
> +int pru_can_get_intc_status(struct device *dev)
> +{
> + u32 u32offset = 0;
> + u32 u32getvalue = 0;
> + u32 u32clrvalue = 0;
> +
> + u32offset = (PRUSS_INTC_STATCLRINT1 & 0xFFFF);
> + pruss_readl(dev, u32offset, (u32 *) &u32getvalue, 1);
> +
> + if (u32getvalue & 4)
> + u32clrvalue = 34; /* CLR Event 34 */
> +
> + if (u32getvalue & 2)
> + u32clrvalue = 33; /* CLR Event 33 */
> +
> + if (u32clrvalue) {
> + u32offset = (PRUSS_INTC_STATIDXCLR & 0xFFFF);
> + pruss_writel(dev, u32offset, &u32clrvalue, 1);
> + } else
> + return -1;
Could the controller signal both event 34 and 33 simultaneously?
The only user of this function looks at the individual bits
of the return value again, which looks wrong for all possible
return values here.
> +#ifndef _PRU_CAN_API_H_
> +#define CAN_BIT_TIMINGS (0x273)
> +
> +/* Timer Clock is sourced from DDR freq (PLL1 SYS CLK 2) */
> +#define TIMER_CLK_FREQ 132000000
> +
> +#define TIMER_SETUP_DELAY 14
> +#define GPIO_SETUP_DELAY 150
> +
> +#define CAN_RX_PRU_0 PRUSS_NUM0
> +#define CAN_TX_PRU_1 PRUSS_NUM1
> +
> +/* Number of Instruction in the Delay loop */
> +#define DELAY_LOOP_LENGTH 2
> +
> +#define PRU1_BASE_ADDR 0x2000
> +
> +#define PRU_CAN_TX_GLOBAL_CONTROL_REGISTER (PRU1_BASE_ADDR)
> +#define PRU_CAN_TX_GLOBAL_STATUS_REGISTER (PRU1_BASE_ADDR + 0x04)
> +#define PRU_CAN_TX_INTERRUPT_MASK_REGISTER (PRU1_BASE_ADDR + 0x08)
> +#define PRU_CAN_TX_INTERRUPT_STATUS_REGISTER (PRU1_BASE_ADDR + 0x0C)
> +#define PRU_CAN_TX_MAILBOX0_STATUS_REGISTER (PRU1_BASE_ADDR + 0x10)
> +#define PRU_CAN_TX_MAILBOX1_STATUS_REGISTER (PRU1_BASE_ADDR + 0x14)
The header file should be used for interfaces between the two .c files,
don't mix that with hardware specific definitions. Sometimes you may want
to have register number lists in a header, if that list is going to be
used in multiple places. In this case, there is just one user, so better
move all those definitions over there.
> +typedef enum {
> + ecaninst0 = 0,
> + ecaninst1,
> + ecanmaxinst
> +} can_instance_enum;
> +
> +typedef enum {
> + ecanmailbox0 = 0,
> + ecanmailbox1,
> + ecanmailbox2,
> + ecanmailbox3,
> + ecanmailbox4,
> + ecanmailbox5,
> + ecanmailbox6,
> + ecanmailbox7
> +} can_mailbox_number;
> +
> +typedef enum {
> + ecandirectioninit = 0,
> + ecantransmit,
> + ecanreceive
> +} can_transfer_direction;
The values are all unused, you only use the typedefs.
IMHO it would be more sensible to just pass these as unsigned int
or u32 values, but if you prefer, there is no reason to just do
typedef u32 can_mailbox_number;
etc.
> +typedef struct {
> + u16 u16extendedidentifier;
> + u16 u16baseidentifier;
> + u8 u8data7;
> + u8 u8data6;
> + u8 u8data5;
> + u8 u8data4;
> + u8 u8data3;
> + u8 u8data2;
> + u8 u8data1;
> + u8 u8data0;
> + u16 u16datalength;
> + u16 u16crc;
> +} can_mail_box_structure;
Please don't use typedef for complex data structures, and learn about
better naming of identifiers. I would suggest writing this as
struct pru_can_mailbox {
u16 ext_id;
u16 base_id;
u8 data[8]; /* note: reverse order */
u16 len;
u16 crc;
};
Sames rules apply to the other structures.
Arnd
^ permalink raw reply
* Re: [patch net-next-2.6] net: convert bonding to use rx_handler
From: Patrick McHardy @ 2011-02-18 15:50 UTC (permalink / raw)
To: Jiri Pirko
Cc: Eric Dumazet, netdev, davem, shemminger, fubar, nicolas.2p.debian,
andy
In-Reply-To: <20110218145850.GF2939@psychotron.redhat.com>
On 18.02.2011 15:58, Jiri Pirko wrote:
> Fri, Feb 18, 2011 at 03:46:45PM CET, kaber@trash.net wrote:
>> Am 18.02.2011 15:27, schrieb Eric Dumazet:
>>> Le vendredi 18 février 2011 à 15:14 +0100, Jiri Pirko a écrit :
>>>
>>>> Do not know how to do it better. As for percpu variable, not only
>>>> origdev would have to be remembered but also probably skb pointer to
>>>> know if it's the first run on the skb or not. Can't really figure out a
>>>> better solution. Can you?
>>>
>>> I'll try and let you know.
>>
>> Why not simply do a lookup on skb->iif?
>
> Well I was trying to avoid iterating over list of devices for each
> incoming frame.
>
Well, there are a couple of holes on 64 bit, perhaps you can rearrange
things and eliminate either iif or input_dev without increasing size
since they appear to be redundant.
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: Warn if we cannot set the advertising mask for specified speed/duplex
From: Ben Hutchings @ 2011-02-18 15:56 UTC (permalink / raw)
To: netdev; +Cc: linux-net-drivers, Naveen Chouksey
In-Reply-To: <1297969576.2637.33.camel@bwh-desktop>
On Thu, 2011-02-17 at 19:06 +0000, Ben Hutchings wrote:
> When autonegotiation is enabled, drivers must determine link speed and
> duplex through the autonegotiation process and will generally ignore
> the speed and duplex specified in struct ethtool_cmd. If the user
> specifies a recognised combination of speed and duplex, we set the
> advertising mask to include only the matching mode. Otherwise, we
> advertise all supported modes. We should really limit the advertised
> modes separately by speed and duplex, but for now we just warn.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> This is a longstanding bug in ethtool which people keep getting bitten
> by. Naveen was just the latest to report it. I have been working on
> some changes to improve link advertising and reporting, but until those
> are ready I'm adding this warning.
That patch fixes an additional bug: we will now update the advertising
mask based on speed/duplex when autoneg is already on. Unfortunately it
introduces a bug: we will update advertising even if none of autoneg,
speed or duplex are specified!
Here's a second version which I think will do the right thing in all
cases, though I haven't tested it yet. I'll include this in ethtool
2.6.38 if no-one can spot a flaw.
Ben.
---
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 17 Feb 2011 18:51:15 +0000
Subject: [PATCH ethtool] ethtool: Don't silently ignore speed/duplex when autoneg is on
When autonegotiation is enabled, drivers must determine link speed and
duplex through the autonegotiation process and will generally ignore
the speed and duplex specified in struct ethtool_cmd. Currently, if
the user specifies autoneg on but does not specify the advertising
mask then:
- If the user specifies a recognised combination of speed and duplex,
we set the advertising mask to the flag for that mode. (Currently
only one mode is recognised per combination of speed and duplex.)
- Otherwise, we advertise all recognised and supported modes.
But we should also set the advertising mask if autoneg is *already*
on. Also, we should be able to limit the advertised modes separately
by speed and duplex. For now, we just warn if we fail to do that.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
ethtool.c | 45 +++++++++++++++++++++++++++++++--------------
1 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/ethtool.c b/ethtool.c
index 1afdfe4..2f4b6ee 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1126,7 +1126,7 @@ static void parse_cmdline(int argc, char **argp)
}
}
- if ((autoneg_wanted == AUTONEG_ENABLE) && (advertising_wanted < 0)) {
+ if (advertising_wanted < 0) {
if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
advertising_wanted = ADVERTISED_10baseT_Half;
else if (speed_wanted == SPEED_10 &&
@@ -2528,19 +2528,36 @@ static int do_sset(int fd, struct ifreq *ifr)
ecmd.phy_address = phyad_wanted;
if (xcvr_wanted != -1)
ecmd.transceiver = xcvr_wanted;
- if (advertising_wanted != -1) {
- if (advertising_wanted == 0)
- ecmd.advertising = ecmd.supported &
- (ADVERTISED_10baseT_Half |
- ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half |
- ADVERTISED_100baseT_Full |
- ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full |
- ADVERTISED_2500baseX_Full |
- ADVERTISED_10000baseT_Full);
- else
- ecmd.advertising = advertising_wanted;
+ /* XXX If the user specified speed or duplex
+ * then we should mask the advertised modes
+ * accordingly. For now, warn that we aren't
+ * doing that.
+ */
+ if ((speed_wanted != -1 || duplex_wanted != -1) &&
+ ecmd.autoneg && advertising_wanted == 0) {
+ fprintf(stderr, "Cannot advertise");
+ if (speed_wanted >= 0)
+ fprintf(stderr, " speed %d",
+ speed_wanted);
+ if (duplex_wanted >= 0)
+ fprintf(stderr, " duplex %s",
+ duplex_wanted ?
+ "full" : "half");
+ fprintf(stderr, "\n");
+ }
+ if (autoneg_wanted == AUTONEG_ENABLE &&
+ advertising_wanted == 0) {
+ ecmd.advertising = ecmd.supported &
+ (ADVERTISED_10baseT_Half |
+ ADVERTISED_10baseT_Full |
+ ADVERTISED_100baseT_Half |
+ ADVERTISED_100baseT_Full |
+ ADVERTISED_1000baseT_Half |
+ ADVERTISED_1000baseT_Full |
+ ADVERTISED_2500baseX_Full |
+ ADVERTISED_10000baseT_Full);
+ } else if (advertising_wanted != -1) {
+ ecmd.advertising = advertising_wanted;
}
/* Try to perform the update. */
--
1.7.3.4
--
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 related
* pull request: sfc-next-2.6 2011-02-18
From: Ben Hutchings @ 2011-02-18 16:00 UTC (permalink / raw)
To: David Miller; +Cc: netdev, sf-linux-drivers, Tom Herbert
The following changes since commit f878b995b0f746f5726af9e66940f3bf373dae91:
Merge branch 'for-davem' of git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next-2.6 (2011-02-15 12:25:19 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next-2.6.git for-davem
Add support for RFS acceleration in the sfc driver, and allow it to be
enabled and disabled dynamically using ethtool.
Ben.
Ben Hutchings (3):
net: RPS: Make hardware-accelerated RFS conditional on NETIF_F_NTUPLE
sfc: Limit filter search depth further for performance hints (i.e. RFS)
sfc: Implement hardware acceleration of RFS
drivers/net/sfc/efx.c | 49 +++++++++++++++++-
drivers/net/sfc/efx.h | 15 +++++
drivers/net/sfc/filter.c | 117 ++++++++++++++++++++++++++++++++++++++++--
drivers/net/sfc/net_driver.h | 3 +
net/core/dev.c | 3 +-
5 files changed, 180 insertions(+), 7 deletions(-)
--
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-2.6 1/3] net: RPS: Make hardware-accelerated RFS conditional on NETIF_F_NTUPLE
From: Ben Hutchings @ 2011-02-18 16:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1298044839.2570.7.camel@bwh-desktop>
For testing and debugging purposes it is useful to be able to disable
hardware acceleration of RFS without disabling RFS altogether. Since
this is a similar feature to 'n-tuple' flow steering through the
ethtool API, test the same feature flag that controls that.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
net/core/dev.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 30c71f9..54aaca6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2607,7 +2607,8 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
int rc;
/* Should we steer this flow to a different hardware queue? */
- if (!skb_rx_queue_recorded(skb) || !dev->rx_cpu_rmap)
+ if (!skb_rx_queue_recorded(skb) || !dev->rx_cpu_rmap ||
+ !(dev->features & NETIF_F_NTUPLE))
goto out;
rxq_index = cpu_rmap_lookup_index(dev->rx_cpu_rmap, next_cpu);
if (rxq_index == skb_get_rx_queue(skb))
--
1.7.3.4
--
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 related
* [PATCH net-next-2.6 2/3] sfc: Limit filter search depth further for performance hints (i.e. RFS)
From: Ben Hutchings @ 2011-02-18 16:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1298044839.2570.7.camel@bwh-desktop>
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/sfc/filter.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/net/sfc/filter.c b/drivers/net/sfc/filter.c
index d4722c4..47a1b79 100644
--- a/drivers/net/sfc/filter.c
+++ b/drivers/net/sfc/filter.c
@@ -27,6 +27,10 @@
*/
#define FILTER_CTL_SRCH_MAX 200
+/* Don't try very hard to find space for performance hints, as this is
+ * counter-productive. */
+#define FILTER_CTL_SRCH_HINT_MAX 5
+
enum efx_filter_table_id {
EFX_FILTER_TABLE_RX_IP = 0,
EFX_FILTER_TABLE_RX_MAC,
@@ -325,15 +329,16 @@ static int efx_filter_search(struct efx_filter_table *table,
struct efx_filter_spec *spec, u32 key,
bool for_insert, int *depth_required)
{
- unsigned hash, incr, filter_idx, depth;
+ unsigned hash, incr, filter_idx, depth, depth_max;
struct efx_filter_spec *cmp;
hash = efx_filter_hash(key);
incr = efx_filter_increment(key);
+ depth_max = (spec->priority <= EFX_FILTER_PRI_HINT ?
+ FILTER_CTL_SRCH_HINT_MAX : FILTER_CTL_SRCH_MAX);
for (depth = 1, filter_idx = hash & (table->size - 1);
- depth <= FILTER_CTL_SRCH_MAX &&
- test_bit(filter_idx, table->used_bitmap);
+ depth <= depth_max && test_bit(filter_idx, table->used_bitmap);
++depth) {
cmp = &table->spec[filter_idx];
if (efx_filter_equal(spec, cmp))
@@ -342,7 +347,7 @@ static int efx_filter_search(struct efx_filter_table *table,
}
if (!for_insert)
return -ENOENT;
- if (depth > FILTER_CTL_SRCH_MAX)
+ if (depth > depth_max)
return -EBUSY;
found:
*depth_required = depth;
--
1.7.3.4
--
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 related
* [PATCH net-next-2.6 3/3] sfc: Implement hardware acceleration of RFS
From: Ben Hutchings @ 2011-02-18 16:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1298044839.2570.7.camel@bwh-desktop>
Use the existing filter management functions to insert TCP/IPv4 and
UDP/IPv4 4-tuple filters for Receive Flow Steering.
For each channel, track how many RFS filters are being added during
processing of received packets and scan the corresponding number of
table entries for filters that may be reclaimed. Do this in batches
to reduce lock overhead.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/sfc/efx.c | 49 +++++++++++++++++++-
drivers/net/sfc/efx.h | 15 ++++++
drivers/net/sfc/filter.c | 104 ++++++++++++++++++++++++++++++++++++++++++
drivers/net/sfc/net_driver.h | 3 +
4 files changed, 169 insertions(+), 2 deletions(-)
diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index d4e0425..35b7bc5 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -21,6 +21,7 @@
#include <linux/ethtool.h>
#include <linux/topology.h>
#include <linux/gfp.h>
+#include <linux/cpu_rmap.h>
#include "net_driver.h"
#include "efx.h"
#include "nic.h"
@@ -307,6 +308,8 @@ static int efx_poll(struct napi_struct *napi, int budget)
channel->irq_mod_score = 0;
}
+ efx_filter_rfs_expire(channel);
+
/* There is no race here; although napi_disable() will
* only wait for napi_complete(), this isn't a problem
* since efx_channel_processed() will have no effect if
@@ -1175,10 +1178,32 @@ static int efx_wanted_channels(void)
return count;
}
+static int
+efx_init_rx_cpu_rmap(struct efx_nic *efx, struct msix_entry *xentries)
+{
+#ifdef CONFIG_RFS_ACCEL
+ int i, rc;
+
+ efx->net_dev->rx_cpu_rmap = alloc_irq_cpu_rmap(efx->n_rx_channels);
+ if (!efx->net_dev->rx_cpu_rmap)
+ return -ENOMEM;
+ for (i = 0; i < efx->n_rx_channels; i++) {
+ rc = irq_cpu_rmap_add(efx->net_dev->rx_cpu_rmap,
+ xentries[i].vector);
+ if (rc) {
+ free_irq_cpu_rmap(efx->net_dev->rx_cpu_rmap);
+ efx->net_dev->rx_cpu_rmap = NULL;
+ return rc;
+ }
+ }
+#endif
+ return 0;
+}
+
/* Probe the number and type of interrupts we are able to obtain, and
* the resulting numbers of channels and RX queues.
*/
-static void efx_probe_interrupts(struct efx_nic *efx)
+static int efx_probe_interrupts(struct efx_nic *efx)
{
int max_channels =
min_t(int, efx->type->phys_addr_channels, EFX_MAX_CHANNELS);
@@ -1220,6 +1245,11 @@ static void efx_probe_interrupts(struct efx_nic *efx)
efx->n_tx_channels = efx->n_channels;
efx->n_rx_channels = efx->n_channels;
}
+ rc = efx_init_rx_cpu_rmap(efx, xentries);
+ if (rc) {
+ pci_disable_msix(efx->pci_dev);
+ return rc;
+ }
for (i = 0; i < n_channels; i++)
efx_get_channel(efx, i)->irq =
xentries[i].vector;
@@ -1253,6 +1283,8 @@ static void efx_probe_interrupts(struct efx_nic *efx)
efx->n_tx_channels = 1;
efx->legacy_irq = efx->pci_dev->irq;
}
+
+ return 0;
}
static void efx_remove_interrupts(struct efx_nic *efx)
@@ -1289,7 +1321,9 @@ static int efx_probe_nic(struct efx_nic *efx)
/* Determine the number of channels and queues by trying to hook
* in MSI-X interrupts. */
- efx_probe_interrupts(efx);
+ rc = efx_probe_interrupts(efx);
+ if (rc)
+ goto fail;
if (efx->n_channels > 1)
get_random_bytes(&efx->rx_hash_key, sizeof(efx->rx_hash_key));
@@ -1304,6 +1338,10 @@ static int efx_probe_nic(struct efx_nic *efx)
efx_init_irq_moderation(efx, tx_irq_mod_usec, rx_irq_mod_usec, true);
return 0;
+
+fail:
+ efx->type->remove(efx);
+ return rc;
}
static void efx_remove_nic(struct efx_nic *efx)
@@ -1837,6 +1875,9 @@ static const struct net_device_ops efx_netdev_ops = {
.ndo_poll_controller = efx_netpoll,
#endif
.ndo_setup_tc = efx_setup_tc,
+#ifdef CONFIG_RFS_ACCEL
+ .ndo_rx_flow_steer = efx_filter_rfs,
+#endif
};
static void efx_update_name(struct efx_nic *efx)
@@ -2274,6 +2315,10 @@ static void efx_fini_struct(struct efx_nic *efx)
*/
static void efx_pci_remove_main(struct efx_nic *efx)
{
+#ifdef CONFIG_RFS_ACCEL
+ free_irq_cpu_rmap(efx->net_dev->rx_cpu_rmap);
+ efx->net_dev->rx_cpu_rmap = NULL;
+#endif
efx_nic_fini_interrupt(efx);
efx_fini_channels(efx);
efx_fini_port(efx);
diff --git a/drivers/net/sfc/efx.h b/drivers/net/sfc/efx.h
index 0cb198a..cbce62b 100644
--- a/drivers/net/sfc/efx.h
+++ b/drivers/net/sfc/efx.h
@@ -76,6 +76,21 @@ extern int efx_filter_remove_filter(struct efx_nic *efx,
struct efx_filter_spec *spec);
extern void efx_filter_clear_rx(struct efx_nic *efx,
enum efx_filter_priority priority);
+#ifdef CONFIG_RFS_ACCEL
+extern int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
+ u16 rxq_index, u32 flow_id);
+extern bool __efx_filter_rfs_expire(struct efx_nic *efx, unsigned quota);
+static inline void efx_filter_rfs_expire(struct efx_channel *channel)
+{
+ if (channel->rfs_filters_added >= 60 &&
+ __efx_filter_rfs_expire(channel->efx, 100))
+ channel->rfs_filters_added -= 60;
+}
+#define efx_filter_rfs_enabled() 1
+#else
+static inline void efx_filter_rfs_expire(struct efx_channel *channel) {}
+#define efx_filter_rfs_enabled() 0
+#endif
/* Channels */
extern void efx_process_channel_now(struct efx_channel *channel);
diff --git a/drivers/net/sfc/filter.c b/drivers/net/sfc/filter.c
index 47a1b79..95a980f 100644
--- a/drivers/net/sfc/filter.c
+++ b/drivers/net/sfc/filter.c
@@ -8,6 +8,7 @@
*/
#include <linux/in.h>
+#include <net/ip.h>
#include "efx.h"
#include "filter.h"
#include "io.h"
@@ -51,6 +52,10 @@ struct efx_filter_table {
struct efx_filter_state {
spinlock_t lock;
struct efx_filter_table table[EFX_FILTER_TABLE_COUNT];
+#ifdef CONFIG_RFS_ACCEL
+ u32 *rps_flow_id;
+ unsigned rps_expire_index;
+#endif
};
/* The filter hash function is LFSR polynomial x^16 + x^3 + 1 of a 32-bit
@@ -567,6 +572,13 @@ int efx_probe_filters(struct efx_nic *efx)
spin_lock_init(&state->lock);
if (efx_nic_rev(efx) >= EFX_REV_FALCON_B0) {
+#ifdef CONFIG_RFS_ACCEL
+ state->rps_flow_id = kcalloc(FR_BZ_RX_FILTER_TBL0_ROWS,
+ sizeof(*state->rps_flow_id),
+ GFP_KERNEL);
+ if (!state->rps_flow_id)
+ goto fail;
+#endif
table = &state->table[EFX_FILTER_TABLE_RX_IP];
table->id = EFX_FILTER_TABLE_RX_IP;
table->offset = FR_BZ_RX_FILTER_TBL0;
@@ -612,5 +624,97 @@ void efx_remove_filters(struct efx_nic *efx)
kfree(state->table[table_id].used_bitmap);
vfree(state->table[table_id].spec);
}
+#ifdef CONFIG_RFS_ACCEL
+ kfree(state->rps_flow_id);
+#endif
kfree(state);
}
+
+#ifdef CONFIG_RFS_ACCEL
+
+int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
+ u16 rxq_index, u32 flow_id)
+{
+ struct efx_nic *efx = netdev_priv(net_dev);
+ struct efx_channel *channel;
+ struct efx_filter_state *state = efx->filter_state;
+ struct efx_filter_spec spec;
+ const struct iphdr *ip;
+ const __be16 *ports;
+ int nhoff;
+ int rc;
+
+ nhoff = skb_network_offset(skb);
+
+ if (skb->protocol != htons(ETH_P_IP))
+ return -EPROTONOSUPPORT;
+
+ /* RFS must validate the IP header length before calling us */
+ EFX_BUG_ON_PARANOID(!pskb_may_pull(skb, nhoff + sizeof(*ip)));
+ ip = (const struct iphdr *)(skb->data + nhoff);
+ if (ip->frag_off & htons(IP_MF | IP_OFFSET))
+ return -EPROTONOSUPPORT;
+ EFX_BUG_ON_PARANOID(!pskb_may_pull(skb, nhoff + 4 * ip->ihl + 4));
+ ports = (const __be16 *)(skb->data + nhoff + 4 * ip->ihl);
+
+ efx_filter_init_rx(&spec, EFX_FILTER_PRI_HINT, 0, rxq_index);
+ rc = efx_filter_set_ipv4_full(&spec, ip->protocol,
+ ip->daddr, ports[1], ip->saddr, ports[0]);
+ if (rc)
+ return rc;
+
+ rc = efx_filter_insert_filter(efx, &spec, true);
+ if (rc < 0)
+ return rc;
+
+ /* Remember this so we can check whether to expire the filter later */
+ state->rps_flow_id[rc] = flow_id;
+ channel = efx_get_channel(efx, skb_get_rx_queue(skb));
+ ++channel->rfs_filters_added;
+
+ netif_info(efx, rx_status, efx->net_dev,
+ "steering %s %pI4:%u:%pI4:%u to queue %u [flow %u filter %d]\n",
+ (ip->protocol == IPPROTO_TCP) ? "TCP" : "UDP",
+ &ip->saddr, ntohs(ports[0]), &ip->daddr, ntohs(ports[1]),
+ rxq_index, flow_id, rc);
+
+ return rc;
+}
+
+bool __efx_filter_rfs_expire(struct efx_nic *efx, unsigned quota)
+{
+ struct efx_filter_state *state = efx->filter_state;
+ struct efx_filter_table *table = &state->table[EFX_FILTER_TABLE_RX_IP];
+ unsigned mask = table->size - 1;
+ unsigned index;
+ unsigned stop;
+
+ if (!spin_trylock_bh(&state->lock))
+ return false;
+
+ index = state->rps_expire_index;
+ stop = (index + quota) & mask;
+
+ while (index != stop) {
+ if (test_bit(index, table->used_bitmap) &&
+ table->spec[index].priority == EFX_FILTER_PRI_HINT &&
+ rps_may_expire_flow(efx->net_dev,
+ table->spec[index].dmaq_id,
+ state->rps_flow_id[index], index)) {
+ netif_info(efx, rx_status, efx->net_dev,
+ "expiring filter %d [flow %u]\n",
+ index, state->rps_flow_id[index]);
+ efx_filter_table_clear_entry(efx, table, index);
+ }
+ index = (index + 1) & mask;
+ }
+
+ state->rps_expire_index = stop;
+ if (table->used == 0)
+ efx_filter_table_reset_search_depth(table);
+
+ spin_unlock_bh(&state->lock);
+ return true;
+}
+
+#endif /* CONFIG_RFS_ACCEL */
diff --git a/drivers/net/sfc/net_driver.h b/drivers/net/sfc/net_driver.h
index 96e22ad..15b9068 100644
--- a/drivers/net/sfc/net_driver.h
+++ b/drivers/net/sfc/net_driver.h
@@ -362,6 +362,9 @@ struct efx_channel {
unsigned int irq_count;
unsigned int irq_mod_score;
+#ifdef CONFIG_RFS_ACCEL
+ unsigned int rfs_filters_added;
+#endif
int rx_alloc_level;
int rx_alloc_push_pages;
--
1.7.3.4
--
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 related
* [RFC PATCH net-next-2.6 0/2] Automatic XPS mapping
From: Ben Hutchings @ 2011-02-18 16:13 UTC (permalink / raw)
To: Tom Herbert; +Cc: netdev, sf-linux-drivers
In the same way that we maintain a mapping CPUs to RX queues for RFS
acceleration based on current IRQ affinity and the CPU topology, we can
maintain a mapping of CPUs to TX queues for queue selection in XPS. (In
fact this may be the same mapping.)
Questions:
- Does this make a real difference to performance?
(I've only barely tested this.)
- Should there be a way to disable it?
- Should the automatic mapping be made visible?
(This applies RFS acceleration too.)
- Should different mappings be allowed for different traffic classes,
in case they have separate sets of TX interrupts with different
affinity?
(This applies to manual XPS configuration too.)
Ben Hutchings (2):
net: XPS: Allow driver to provide a default mapping of CPUs to TX
queues
sfc: Add CPU queue mapping for XPS
drivers/net/sfc/efx.c | 59 +++++++++++++++++++++++++++++++-------------
include/linux/netdevice.h | 10 +++++--
net/Kconfig | 17 +++++++++----
net/core/dev.c | 41 +++++++++++++++++-------------
4 files changed, 83 insertions(+), 44 deletions(-)
--
1.7.3.4
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [patch net-next-2.6] net: convert bonding to use rx_handler
From: Eric Dumazet @ 2011-02-18 16:14 UTC (permalink / raw)
To: Patrick McHardy, Jiri Pirko
Cc: netdev, davem, shemminger, fubar, nicolas.2p.debian, andy
In-Reply-To: <4D5E953F.6010606@trash.net>
Le vendredi 18 février 2011 à 16:50 +0100, Patrick McHardy a écrit :
> On 18.02.2011 15:58, Jiri Pirko wrote:
> > Fri, Feb 18, 2011 at 03:46:45PM CET, kaber@trash.net wrote:
> >> Am 18.02.2011 15:27, schrieb Eric Dumazet:
> >>> Le vendredi 18 février 2011 à 15:14 +0100, Jiri Pirko a écrit :
> >>>
> >>>> Do not know how to do it better. As for percpu variable, not only
> >>>> origdev would have to be remembered but also probably skb pointer to
> >>>> know if it's the first run on the skb or not. Can't really figure out a
> >>>> better solution. Can you?
> >>>
> >>> I'll try and let you know.
> >>
> >> Why not simply do a lookup on skb->iif?
> >
> > Well I was trying to avoid iterating over list of devices for each
> > incoming frame.
> >
>
> Well, there are a couple of holes on 64 bit, perhaps you can rearrange
> things and eliminate either iif or input_dev without increasing size
> since they appear to be redundant.
Jiri
I dont understand why netif_rx() is needed in your patch.
Can we stack 10 bond devices or so ???
If we avoid this stage and call the real thing (netif_receive_skb()),
then we dont need adding a field in each skb, since it can be carried by
a global variable (per cpu of course)
bond_handle_frame() being called from __netif_receive_skb() I believe it
can use netif_receive_skb() instead of netif_rx().
Same remark for vlan_on_bond_hook()
^ permalink raw reply
* [RFC PATCH net-next-2.6 1/2] net: XPS: Allow driver to provide a default mapping of CPUs to TX queues
From: Ben Hutchings @ 2011-02-18 16:15 UTC (permalink / raw)
To: Tom Herbert; +Cc: netdev, linux-net-drivers
In-Reply-To: <1298045607.2570.17.camel@bwh-desktop>
As with RFS acceleration, drivers may use the irq_cpu_rmap facility to
provide a mapping from each CPU to the 'nearest' TX queue.
Define CONFIG_PACKET_STEERING and CONFIG_NET_IRQ_CPU_RMAP so that
drivers can make all cpu_rmap initialisation conditional on the
latter.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
include/linux/netdevice.h | 10 +++++++---
net/Kconfig | 17 ++++++++++++-----
net/core/dev.c | 41 +++++++++++++++++++++++------------------
3 files changed, 42 insertions(+), 26 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c7d7074..c5cba16 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1080,14 +1080,13 @@ struct net_device {
/* Number of RX queues currently active in device */
unsigned int real_num_rx_queues;
-
-#ifdef CONFIG_RFS_ACCEL
+#endif
+#ifdef CONFIG_NET_IRQ_CPU_RMAP
/* CPU reverse-mapping for RX completion interrupts, indexed
* by RX queue number. Assigned by driver. This must only be
* set if the ndo_rx_flow_steer operation is defined. */
struct cpu_rmap *rx_cpu_rmap;
#endif
-#endif
rx_handler_func_t __rcu *rx_handler;
void __rcu *rx_handler_data;
@@ -1114,6 +1113,11 @@ struct net_device {
#ifdef CONFIG_XPS
struct xps_dev_maps __rcu *xps_maps;
#endif
+#ifdef CONFIG_NET_IRQ_CPU_RMAP
+ /* CPU reverse-mapping for TX completion interrupts. Assigned
+ * by driver. */
+ struct cpu_rmap *tx_cpu_rmap;
+#endif
/* These may be needed for future network-power-down code. */
diff --git a/net/Kconfig b/net/Kconfig
index 79cabf1..fa0a093 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -216,21 +216,28 @@ source "net/dcb/Kconfig"
source "net/dns_resolver/Kconfig"
source "net/batman-adv/Kconfig"
-config RPS
+config PACKET_STEERING
boolean
depends on SMP && SYSFS && USE_GENERIC_SMP_HELPERS
+ select RPS
+ select XPS
default y
-config RFS_ACCEL
+config NET_IRQ_CPU_RMAP
boolean
- depends on RPS && GENERIC_HARDIRQS
+ depends on PACKET_STEERING && GENERIC_HARDIRQS
select CPU_RMAP
+ select RFS_ACCEL
default y
+config RPS
+ boolean
+
+config RFS_ACCEL
+ boolean
+
config XPS
boolean
- depends on SMP && SYSFS && USE_GENERIC_SMP_HELPERS
- default y
menu "Network testing"
diff --git a/net/core/dev.c b/net/core/dev.c
index 54aaca6..dd38b88 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2257,27 +2257,32 @@ static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
rcu_read_lock();
dev_maps = rcu_dereference(dev->xps_maps);
- if (dev_maps) {
+ if (dev_maps)
map = rcu_dereference(
- dev_maps->cpu_map[raw_smp_processor_id()]);
- if (map) {
- if (map->len == 1)
- queue_index = map->queues[0];
- else {
- u32 hash;
- if (skb->sk && skb->sk->sk_hash)
- hash = skb->sk->sk_hash;
- else
- hash = (__force u16) skb->protocol ^
- skb->rxhash;
- hash = jhash_1word(hash, hashrnd);
- queue_index = map->queues[
- ((u64)hash * map->len) >> 32];
- }
- if (unlikely(queue_index >= dev->real_num_tx_queues))
- queue_index = -1;
+ dev_maps->cpu_map[raw_smp_processor_id()]);
+ if (map) {
+ if (map->len == 1)
+ queue_index = map->queues[0];
+ else {
+ u32 hash;
+ if (skb->sk && skb->sk->sk_hash)
+ hash = skb->sk->sk_hash;
+ else
+ hash = (__force u16) skb->protocol ^
+ skb->rxhash;
+ hash = jhash_1word(hash, hashrnd);
+ queue_index = map->queues[
+ ((u64)hash * map->len) >> 32];
}
+ if (unlikely(queue_index >= dev->real_num_tx_queues))
+ queue_index = -1;
+ }
+#ifdef CONFIG_PACKET_STEERING_CPU_RMAP
+ else if (dev->tx_cpu_rmap) {
+ queue_index = cpu_rmap_lookup_index(dev->tx_cpu_rmap,
+ raw_smp_processor_id());
}
+#endif
rcu_read_unlock();
return queue_index;
--
1.7.3.4
--
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 related
* [RFC PATCH net-next-2.6 2/2] sfc: Add CPU queue mapping for XPS
From: Ben Hutchings @ 2011-02-18 16:15 UTC (permalink / raw)
To: Tom Herbert; +Cc: netdev, linux-net-drivers
In-Reply-To: <1298045607.2570.17.camel@bwh-desktop>
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/sfc/efx.c | 59 ++++++++++++++++++++++++++++++++++---------------
1 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 35b7bc5..6d698c3 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1179,9 +1179,11 @@ static int efx_wanted_channels(void)
}
static int
-efx_init_rx_cpu_rmap(struct efx_nic *efx, struct msix_entry *xentries)
+efx_init_cpu_rmaps(struct efx_nic *efx, struct msix_entry *xentries)
{
-#ifdef CONFIG_RFS_ACCEL
+#ifndef CONFIG_NET_IRQ_CPU_RMAP
+ return 0;
+#else
int i, rc;
efx->net_dev->rx_cpu_rmap = alloc_irq_cpu_rmap(efx->n_rx_channels);
@@ -1190,14 +1192,38 @@ efx_init_rx_cpu_rmap(struct efx_nic *efx, struct msix_entry *xentries)
for (i = 0; i < efx->n_rx_channels; i++) {
rc = irq_cpu_rmap_add(efx->net_dev->rx_cpu_rmap,
xentries[i].vector);
- if (rc) {
- free_irq_cpu_rmap(efx->net_dev->rx_cpu_rmap);
- efx->net_dev->rx_cpu_rmap = NULL;
- return rc;
+ if (rc)
+ goto fail_rx;
+ }
+
+ if (efx->tx_channel_offset == 0) {
+ efx->net_dev->tx_cpu_rmap = efx->net_dev->rx_cpu_rmap;
+ } else {
+ efx->net_dev->tx_cpu_rmap =
+ alloc_irq_cpu_rmap(efx->n_tx_channels);
+ if (!efx->net_dev->tx_cpu_rmap) {
+ rc = -ENOMEM;
+ goto fail_rx;
+ }
+ for (i = 0; i < efx->n_tx_channels; i++) {
+ rc = irq_cpu_rmap_add(
+ efx->net_dev->tx_cpu_rmap,
+ xentries[efx->tx_channel_offset + i].vector);
+ if (rc)
+ goto fail_tx;
}
}
-#endif
+
return 0;
+
+fail_tx:
+ free_irq_cpu_rmap(efx->net_dev->tx_cpu_rmap);
+ efx->net_dev->tx_cpu_rmap = NULL;
+fail_rx:
+ free_irq_cpu_rmap(efx->net_dev->rx_cpu_rmap);
+ efx->net_dev->rx_cpu_rmap = NULL;
+ return rc;
+#endif
}
/* Probe the number and type of interrupts we are able to obtain, and
@@ -1238,14 +1264,15 @@ static int efx_probe_interrupts(struct efx_nic *efx)
if (separate_tx_channels) {
efx->n_tx_channels =
max(efx->n_channels / 2, 1U);
+ efx->tx_channel_offset =
+ efx->n_channels - efx->n_tx_channels;
efx->n_rx_channels =
- max(efx->n_channels -
- efx->n_tx_channels, 1U);
+ max(efx->tx_channel_offset, 1U);
} else {
efx->n_tx_channels = efx->n_channels;
efx->n_rx_channels = efx->n_channels;
}
- rc = efx_init_rx_cpu_rmap(efx, xentries);
+ rc = efx_init_cpu_rmaps(efx, xentries);
if (rc) {
pci_disable_msix(efx->pci_dev);
return rc;
@@ -1301,12 +1328,6 @@ static void efx_remove_interrupts(struct efx_nic *efx)
efx->legacy_irq = 0;
}
-static void efx_set_channels(struct efx_nic *efx)
-{
- efx->tx_channel_offset =
- separate_tx_channels ? efx->n_channels - efx->n_tx_channels : 0;
-}
-
static int efx_probe_nic(struct efx_nic *efx)
{
size_t i;
@@ -1330,7 +1351,6 @@ static int efx_probe_nic(struct efx_nic *efx)
for (i = 0; i < ARRAY_SIZE(efx->rx_indir_table); i++)
efx->rx_indir_table[i] = i % efx->n_rx_channels;
- efx_set_channels(efx);
netif_set_real_num_tx_queues(efx->net_dev, efx->n_tx_channels);
netif_set_real_num_rx_queues(efx->net_dev, efx->n_rx_channels);
@@ -2315,7 +2335,10 @@ static void efx_fini_struct(struct efx_nic *efx)
*/
static void efx_pci_remove_main(struct efx_nic *efx)
{
-#ifdef CONFIG_RFS_ACCEL
+#ifdef CONFIG_NET_IRQ_CPU_RMAP
+ if (efx->net_dev->tx_cpu_rmap != efx->net_dev->rx_cpu_rmap)
+ free_irq_cpu_rmap(efx->net_dev->tx_cpu_rmap);
+ efx->net_dev->tx_cpu_rmap = NULL;
free_irq_cpu_rmap(efx->net_dev->rx_cpu_rmap);
efx->net_dev->rx_cpu_rmap = NULL;
#endif
--
1.7.3.4
--
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 related
* IGMPMSG_WRONGVIF only happens when true VIF is an OIF of correct IIF?
From: Dan Langlois @ 2011-02-18 15:46 UTC (permalink / raw)
To: netdev
Hi, I have a question about the IGMPMSG_WRONGVIF message.
I have MRT_ASSERT enabled but do not receive notifications.
However, if I enable MRT_PIM, I do get them. I'm curious
why MRT_ASSERT alone isn't sufficient.
This if-statement appears in ip_mr_forward() in net/ipv4/ipmr.c
if (true_vifi >= 0 && net->ipv4.mroute_do_assert &&
/* pimsm uses asserts, when switching from RPT to SPT,
so that we cannot check that packet arrived on an oif.
It is bad, but otherwise we would need to move pretty
large chunk of pimd to kernel. Ough... --ANK
*/
(net->ipv4.mroute_do_pim ||
cache->mfc_un.res.ttls[true_vifi] < 255) &&
time_after(jiffies,
cache->mfc_un.res.last_assert +
MFC_ASSERT_THRESH)) {
cache->mfc_un.res.last_assert = jiffies;
ipmr_cache_report(net, skb, true_vifi,
IGMPMSG_WRONGVIF);
}
By the time this statement is reached, it is known that the packet
did not arrive on the expected incoming interface (IIF) and thus a
"WRONGVIF" condition. This if-statement decides whether a PIM-assert
notification needs to be sent to the multicast routing daemon.
The part of this if-statement I'm questioning is:
(net->ipv4.mroute_do_pim ||
cache->mfc_un.res.ttls[true_vifi] < 255) &&
I read this as:
"send WRONGVIF if PIM is enabled -OR-
the packet arrived on an outgoing interface OIF of the correct IIF"
So this statement is always true when PIM is enabled.
However, if only ASSERT is enabled, this statement is only true when
a packet is reflected back on an OIF.
Why not always send the assert for any WRONGVIF condition regardless
of the interface it came in on? I mean, isn't that the point of
enabling MRT_ASSERT in the first place? If the assertions weren't
wanted, just don't enable that feature, right?
In our system, multicast streams may get rerouted through a different
VIF than what was first installed in the MFC cache. I would very much
like to get these WRONGVIF assertions in this case, which is NOT the
case that a packet is seen on an OIF.
Of course I can simply enable MRT_PIM to get the messages, but in that
case I don't understand the reason for having two separate toggles
(MRT_ASSERT versus MRT_PIM).
Could someone please explain the reasoning here, as I don't understand
the comment, and I'm tempted to patch out this part of the if-statement.
cheers,
Dan
^ permalink raw reply
* Re: [PATCH 1/1] tproxy: do not assign timewait sockets to skb->sk
From: Patrick McHardy @ 2011-02-18 16:19 UTC (permalink / raw)
To: Florian Westphal
Cc: Balazs Scheidler, netfilter-devel, netdev, KOVACS Krisztian
In-Reply-To: <20110217212717.GG8821@Chamillionaire.breakpoint.cc>
Am 17.02.2011 22:27, schrieb Florian Westphal:
> Balazs Scheidler <bazsi@balabit.hu> wrote:
>> the destination port in the packet can be different in the two lookups. --on-port tproxy option.
>
> Hrm... The initial lookup uses the header ip addresses:
> sk = nf_tproxy_get_sock_v4(dev_net(skb->dev), iph->protocol,
> iph->saddr, iph->daddr,
> hp->source, hp->dest,
> skb->dev, NFT_LOOKUP_ESTABLISHED);
>
> 3 possible cases:
> - no socket -- try to find listener. This case is not changed by my patch.
> - sk is normal socket. set nfmark and skb->sk. Also not changed.
> - sk is in TW state. This is not changed either:
> tproxy_handle_time_wait4() will check if this is a SYN. If it is, a new
> listener lookup is made, and TW socket is kicked out.
>
> If the packet is not a SYN, then tproxy_handle_time_wait4() won't do anything.
> Previously, the timewait sk would now be assigned to skb->sk, which my patch
> prevents. But I don't see where the '--on-port' port number is involved in the
> TW socket lookup?
>
> Thanks for reviwing the patch!
For some reason I've not received Balazs' email. Balazs, I'm about to
submit my queued patches upstream, if you wish to object to this patch,
please do so now.
Thanks.
^ permalink raw reply
* Loan
From: Orbit Loans @ 2011-02-18 13:38 UTC (permalink / raw)
Good day,
Do you have bad credit?
Do you want to upgrade your business?
I give loans to both private and public organizations at
very low and considerable interest rates.
For more information you could contact me at my email
address below:orbitfinanceltduk@mail2uk.com
Regards
Hill Colin Neil Esq.
^ permalink raw reply
* [PATCH][RFC] sctp: fix reporting of unknown parameters
From: Jiri Bohac @ 2011-02-17 23:12 UTC (permalink / raw)
To: linux-sctp, Neil Horman
Hi,
commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the
handling of unknown parameters. sctp_init_cause_fixed() can now
return -ENOSPC if there is not enough tailroom in the error
chunk skb. When this happens, the error header is not appended to
the error chunk. In that case, the payload of the unknown parameter
should not be appended either.
Signed-off-by: Jiri Bohac <jbohac@suse.cz>
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 2cc46f0..b23428f 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2029,11 +2029,11 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
*errp = sctp_make_op_error_fixed(asoc, chunk);
if (*errp) {
- sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
- WORD_ROUND(ntohs(param.p->length)));
- sctp_addto_chunk_fixed(*errp,
- WORD_ROUND(ntohs(param.p->length)),
- param.v);
+ if (!sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
+ WORD_ROUND(ntohs(param.p->length))))
+ sctp_addto_chunk_fixed(*errp,
+ WORD_ROUND(ntohs(param.p->length)),
+ param.v);
} else {
/* If there is no memory for generating the ERROR
* report as specified, an ABORT will be triggered
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply related
* Re: [PATCH][RFC] sctp: fix reporting of unknown parameters
From: Shan Wei @ 2011-02-18 11:26 UTC (permalink / raw)
To: Jiri Bohac; +Cc: linux-sctp, Neil Horman, Vlad Yasevich
In-Reply-To: <20110217231208.GA28281@midget.suse.cz>
(cc vlad Yasevich)
Jiri Bohac wrote, at 02/18/2011 07:12 AM:
> Hi,
>
> commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the
> handling of unknown parameters. sctp_init_cause_fixed() can now
> return -ENOSPC if there is not enough tailroom in the error
> chunk skb. When this happens, the error header is not appended to
> the error chunk. In that case, the payload of the unknown parameter
> should not be appended either.
>
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
For this case, there is no more tailroom in skb, originally
we send incomplete INIT-ACK chunk. With your patch, this chunk also
can't info the sender of INIT Chunk which parameter is not unrecognized.
So maybe this handle is not perfect.
So, for this case, i think send ABORT is more appropriate.
Just take same handle with SCTP_IERROR_NOMEM
>
>
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 2cc46f0..b23428f 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2029,11 +2029,11 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
> *errp = sctp_make_op_error_fixed(asoc, chunk);
>
> if (*errp) {
> - sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> - WORD_ROUND(ntohs(param.p->length)));
> - sctp_addto_chunk_fixed(*errp,
> - WORD_ROUND(ntohs(param.p->length)),
> - param.v);
> + if (!sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> + WORD_ROUND(ntohs(param.p->length))))
> + sctp_addto_chunk_fixed(*errp,
> + WORD_ROUND(ntohs(param.p->length)),
> + param.v);
> } else {
> /* If there is no memory for generating the ERROR
> * report as specified, an ABORT will be triggered
>
>
--
Best Regards
-----
Shan Wei
^ permalink raw reply
* Re: [PATCH][RFC] sctp: fix reporting of unknown parameters
From: Jiri Bohac @ 2011-02-18 12:01 UTC (permalink / raw)
To: Shan Wei; +Cc: Jiri Bohac, linux-sctp, Neil Horman, Vlad Yasevich
In-Reply-To: <4D5E575A.9000905@cn.fujitsu.com>
On Fri, Feb 18, 2011 at 07:26:18PM +0800, Shan Wei wrote:
> Jiri Bohac wrote, at 02/18/2011 07:12 AM:
> > commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the
> > handling of unknown parameters. sctp_init_cause_fixed() can now
> > return -ENOSPC if there is not enough tailroom in the error
> > chunk skb. When this happens, the error header is not appended to
> > the error chunk. In that case, the payload of the unknown parameter
> > should not be appended either.
> >
> > Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
> For this case, there is no more tailroom in skb
not always true:
both sctp_init_cause_fixed() and sctp_addto_chunk_fixed() get
passesd WORD_ROUND(ntohs(param.p->length) as the paylen/len
parameter.
sctp_init_cause_fixed() does:
len = sizeof(sctp_errhdr_t) + paylen;
if (skb_tailroom(chunk->skb) < len)
return -ENOSPC;
if paylen is only slightly smaller than the tailroom (by less
than sizeof(sctp_errhdr_t) bytes), -ENOSPEC will be returned and
the header will not be appended to the error chunk.
But later sctp_addto_chunk_fixed() does this check:
if (skb_tailroom(chunk->skb) >= len)
which will pass and the unknown parameter value will be appended
to the error chunk without the error header.
> we send incomplete INIT-ACK chunk. With your patch, this chunk also
> can't info the sender of INIT Chunk which parameter is not unrecognized.
> So maybe this handle is not perfect.
The logic of not reporting unknown parameters for which we don't
have space in the pre-allocated buffer remains unchanged.
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply
* Re: [PATCH][RFC] sctp: fix reporting of unknown parameters
From: Vladislav Yasevich @ 2011-02-18 14:43 UTC (permalink / raw)
To: Jiri Bohac; +Cc: Shan Wei, linux-sctp, Neil Horman
In-Reply-To: <20110218120142.GA24525@midget.suse.cz>
On 02/18/2011 07:01 AM, Jiri Bohac wrote:
> On Fri, Feb 18, 2011 at 07:26:18PM +0800, Shan Wei wrote:
>> Jiri Bohac wrote, at 02/18/2011 07:12 AM:
>>> commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the
>>> handling of unknown parameters. sctp_init_cause_fixed() can now
>>> return -ENOSPC if there is not enough tailroom in the error
>>> chunk skb. When this happens, the error header is not appended to
>>> the error chunk. In that case, the payload of the unknown parameter
>>> should not be appended either.
>>>
>>> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>>
>> For this case, there is no more tailroom in skb
>
> not always true:
>
> both sctp_init_cause_fixed() and sctp_addto_chunk_fixed() get
> passesd WORD_ROUND(ntohs(param.p->length) as the paylen/len
> parameter.
>
> sctp_init_cause_fixed() does:
> len = sizeof(sctp_errhdr_t) + paylen;
> if (skb_tailroom(chunk->skb) < len)
> return -ENOSPC;
>
> if paylen is only slightly smaller than the tailroom (by less
> than sizeof(sctp_errhdr_t) bytes), -ENOSPEC will be returned and
> the header will not be appended to the error chunk.
>
> But later sctp_addto_chunk_fixed() does this check:
> if (skb_tailroom(chunk->skb) >= len)
> which will pass and the unknown parameter value will be appended
> to the error chunk without the error header.
>
>> we send incomplete INIT-ACK chunk. With your patch, this chunk also
>> can't info the sender of INIT Chunk which parameter is not unrecognized.
>> So maybe this handle is not perfect.
>
> The logic of not reporting unknown parameters for which we don't
> have space in the pre-allocated buffer remains unchanged.
>
Hi Jiri
I agree. Good catch. If we can not add the error header to that packet, then we definitely
should not be adding the error payload either.
We also don't want to abort as Shan Wei suggested. It is allowed, when exceeding the MTU to
drop errors that would not otherwise fit into the packet. It just may so happen that we
can report subsequent unknown parameters, but not this one in particular.
Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
-vlad
^ permalink raw reply
* Re: IGMPMSG_WRONGVIF only happens when true VIF is an OIF of correct IIF?
From: David Lamparter @ 2011-02-18 17:12 UTC (permalink / raw)
To: Dan Langlois; +Cc: netdev
In-Reply-To: <1298044016.2898.12.camel@thor>
[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]
On Fri, Feb 18, 2011 at 04:46:56PM +0100, Dan Langlois wrote:
> Hi, I have a question about the IGMPMSG_WRONGVIF message.
> I have MRT_ASSERT enabled but do not receive notifications.
> However, if I enable MRT_PIM, I do get them. I'm curious
> why MRT_ASSERT alone isn't sufficient.
>
> This if-statement appears in ip_mr_forward() in net/ipv4/ipmr.c
>
> if (true_vifi >= 0 && net->ipv4.mroute_do_assert &&
> /* pimsm uses asserts, when switching from RPT to SPT,
> so that we cannot check that packet arrived on an oif.
> It is bad, but otherwise we would need to move pretty
> large chunk of pimd to kernel. Ough... --ANK
> */
> (net->ipv4.mroute_do_pim ||
> cache->mfc_un.res.ttls[true_vifi] < 255) &&
> time_after(jiffies,
> cache->mfc_un.res.last_assert +
> MFC_ASSERT_THRESH)) {
> cache->mfc_un.res.last_assert = jiffies;
> ipmr_cache_report(net, skb, true_vifi,
> IGMPMSG_WRONGVIF);
> }
>
>
> By the time this statement is reached, it is known that the packet
> did not arrive on the expected incoming interface (IIF) and thus a
> "WRONGVIF" condition. This if-statement decides whether a PIM-assert
> notification needs to be sent to the multicast routing daemon.
>
> The part of this if-statement I'm questioning is:
> (net->ipv4.mroute_do_pim ||
> cache->mfc_un.res.ttls[true_vifi] < 255) &&
>
> I read this as:
> "send WRONGVIF if PIM is enabled -OR-
> the packet arrived on an outgoing interface OIF of the correct IIF"
>
> So this statement is always true when PIM is enabled.
> However, if only ASSERT is enabled, this statement is only true when
> a packet is reflected back on an OIF.
>
> Why not always send the assert for any WRONGVIF condition regardless
> of the interface it came in on? I mean, isn't that the point of
> enabling MRT_ASSERT in the first place? If the assertions weren't
> wanted, just don't enable that feature, right?
The point of MRT_ASSERT is to aid in resolving duplication issues where
you end up with multiple mrouters thinking they're responsible for the
same subnet. This condition is indicated by more than 1 router on the
subnet having the subnet as OIF on the (*,G) or (S,G). Therefore, your
daemon receives the assertion if it is one of the forwarding mrouters,
which it will only be if it's an OIF.
If your daemon doesn't have the target subnet listed as oif, the
assumption is that a different mrouter has been elected to forward
packets for this G/S,G to this subnet. The kernel knows that your daemon
decided to not forward things to this subnet, so you are not involved in
duplicates, so you don't get an assert.
The "PIM or" is a kludge to make PIM work, as indicated by the comment
above the if. I'd have to refreshen my PIM knowledge to fully understand
it or explain it ;)
> In our system, multicast streams may get rerouted through a different
> VIF than what was first installed in the MFC cache. I would very much
> like to get these WRONGVIF assertions in this case, which is NOT the
> case that a packet is seen on an OIF.
>
> Of course I can simply enable MRT_PIM to get the messages, but in that
> case I don't understand the reason for having two separate toggles
> (MRT_ASSERT versus MRT_PIM).
I don't really understand you use case -- is this a case of another
router showing up on the subnet and directing traffic to it? If so, why
wasn't the first router directing traffic to it? Do you have a primary
multicast forwarder election system in place?
-David
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* how to listen() on single IP address but very many ports?
From: Chris Friesen @ 2011-02-18 17:55 UTC (permalink / raw)
To: netdev
I have an application team that needs to listen() for tcp connections on
many ports (and by many I mean pretty much all 64K ports). However, the
connections are short-lived, and the number of active connections at any
given time is small.
Apparently when they tried this before on an older kernel the
performance of the naive "open 60K sockets and call listen()" solution
was not acceptable, so they used NAT with port mapping to direct all the
incoming packets to a single real port. However, they now want to add
support for IPv6 and this solution won't work.
What's the recommended method for efficiently listening on this many
ports? Should I be able to efficiently listen() on that many sockets
using epoll or similar? If there isn't a way to do this, is there an
equivalent IPv6 workaround?
One possible solution that came up was to implement a PORT_ANY which
would match any incoming request that didn't already have an explicit
listener. Even better would be a way to bind a single listening socket
to a range of ports.
Has anyone ever considered something like this?
Chris
--
Chris Friesen
Software Developer
GENBAND
chris.friesen@genband.com
www.genband.com
^ permalink raw reply
* Re: BUG: Bad page map in process udevd (anon_vma: (null)) in 2.6.38-rc4
From: Eric W. Biederman @ 2011-02-18 18:08 UTC (permalink / raw)
To: Linus Torvalds
Cc: Michal Hocko, Ingo Molnar, linux-mm, LKML, David Miller,
Eric Dumazet, netdev
In-Reply-To: <AANLkTimO=M5xG_mnDBSxPKwSOTrp3JhHVBa8=wHsiVHY@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Fri, Feb 18, 2011 at 8:26 AM, Michal Hocko <mhocko@suse.cz> wrote:
>>> Now, I will try with the 2 patches patches in this thread. I will also
>>> turn on DEBUG_LIST and DEBUG_PAGEALLOC.
>>
>> I am not able to reproduce with those 2 patches applied.
>
> Thanks for verifying. Davem/EricD - you can add Michal's tested-by to
> the patches too.
>
> And I think we can consider this whole thing solved. It hopefully also
> explains all the other random crashes that EricB saw - just random
> memory corruption in other datastructures.
>
> EricB - do all your stress-testers run ok now?
Things are looking better and PAGEALLOC debug isn't firing.
So this looks like one bug down. I have not seen the bad page map
symptom.
I am still getting programs segfaulting but that is happening on other
machines running on older kernels so I am going to chalk that up to a
buggy test and a false positive.
I am have OOM problems getting my tests run to complete. On a good
day that happens about 1 time in 3 right now. I'm guess I will have
to turn off DEBUG_PAGEALLOC to get everything to complete.
DEBUG_PAGEALLOC causes us to use more memory doesn't it?
The most interesting thing I have right now is a networking lockdep
issue. Does anyone know what is going on there?
Eric
=================================
[ INFO: inconsistent lock state ]
2.6.38-rc4-359399.2010AroraKernelBeta.fc14.x86_64 #1
---------------------------------
inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
kworker/u:1/10833 [HC0[0]:SC0[0]:HE1:SE1] takes:
(tcp_death_row.death_lock){+.?...}, at: [<ffffffff81460e69>] inet_twsk_deschedule+0x29/0xa0
{IN-SOFTIRQ-W} state was registered at:
[<ffffffff810840ce>] __lock_acquire+0x70e/0x1d30
[<ffffffff81085cff>] lock_acquire+0x9f/0x120
[<ffffffff814deb6c>] _raw_spin_lock+0x2c/0x40
[<ffffffff8146066b>] inet_twsk_schedule+0x3b/0x1e0
[<ffffffff8147bf7d>] tcp_time_wait+0x20d/0x380
[<ffffffff8146b46e>] tcp_fin.clone.39+0x10e/0x1c0
[<ffffffff8146c628>] tcp_data_queue+0x798/0xd50
[<ffffffff8146fdd9>] tcp_rcv_state_process+0x799/0xbb0
[<ffffffff814786d8>] tcp_v4_do_rcv+0x238/0x500
[<ffffffff8147a90a>] tcp_v4_rcv+0x86a/0xbe0
[<ffffffff81455d4d>] ip_local_deliver_finish+0x10d/0x380
[<ffffffff81456180>] ip_local_deliver+0x80/0x90
[<ffffffff81455832>] ip_rcv_finish+0x192/0x5a0
[<ffffffff814563c4>] ip_rcv+0x234/0x300
[<ffffffff81420e83>] __netif_receive_skb+0x443/0x700
[<ffffffff81421d68>] netif_receive_skb+0xb8/0xf0
[<ffffffff81421ed8>] napi_skb_finish+0x48/0x60
[<ffffffff81422d35>] napi_gro_receive+0xb5/0xc0
[<ffffffffa006b4cf>] igb_poll+0x89f/0xd20 [igb]
[<ffffffff81422279>] net_rx_action+0x149/0x270
[<ffffffff81054bc0>] __do_softirq+0xc0/0x1f0
[<ffffffff81003d1c>] call_softirq+0x1c/0x30
[<ffffffff81005825>] do_softirq+0xa5/0xe0
[<ffffffff81054dfd>] irq_exit+0x8d/0xa0
[<ffffffff81005391>] do_IRQ+0x61/0xe0
[<ffffffff814df793>] ret_from_intr+0x0/0x1a
[<ffffffff810ec9ed>] ____pagevec_lru_add+0x16d/0x1a0
[<ffffffff810ed073>] lru_add_drain+0x73/0xe0
[<ffffffff8110a44c>] exit_mmap+0x5c/0x180
[<ffffffff8104aad2>] mmput+0x52/0xe0
[<ffffffff810513c0>] exit_mm+0x120/0x150
[<ffffffff81051522>] do_exit+0x132/0x8c0
[<ffffffff81051f39>] do_group_exit+0x59/0xd0
[<ffffffff81051fc2>] sys_exit_group+0x12/0x20
[<ffffffff81002d92>] system_call_fastpath+0x16/0x1b
irq event stamp: 187417
hardirqs last enabled at (187417): [<ffffffff81127db5>] kmem_cache_free+0x125/0x160
hardirqs last disabled at (187416): [<ffffffff81127d02>] kmem_cache_free+0x72/0x160
softirqs last enabled at (187410): [<ffffffff81411c52>] sk_common_release+0x62/0xc0
softirqs last disabled at (187408): [<ffffffff814dec41>] _raw_write_lock_bh+0x11/0x40
other info that might help us debug this:
3 locks held by kworker/u:1/10833:
#0: (netns){.+.+.+}, at: [<ffffffff81068be1>] process_one_work+0x121/0x4b0
#1: (net_cleanup_work){+.+.+.}, at: [<ffffffff81068be1>] process_one_work+0x121/0x4b0
#2: (net_mutex){+.+.+.}, at: [<ffffffff8141c4a0>] cleanup_net+0x80/0x1b0
stack backtrace:
Pid: 10833, comm: kworker/u:1 Not tainted 2.6.38-rc4-359399.2010AroraKernelBeta.fc14.x86_64 #1
Call Trace:
[<ffffffff810835b0>] ? print_usage_bug+0x170/0x180
[<ffffffff8108393f>] ? mark_lock+0x37f/0x400
[<ffffffff81084150>] ? __lock_acquire+0x790/0x1d30
[<ffffffff81083d8f>] ? __lock_acquire+0x3cf/0x1d30
[<ffffffff81124acf>] ? check_object+0xaf/0x270
[<ffffffff81460e69>] ? inet_twsk_deschedule+0x29/0xa0
[<ffffffff81085cff>] ? lock_acquire+0x9f/0x120
[<ffffffff81460e69>] ? inet_twsk_deschedule+0x29/0xa0
[<ffffffff81410f49>] ? __sk_free+0xd9/0x160
[<ffffffff814deb6c>] ? _raw_spin_lock+0x2c/0x40
[<ffffffff81460e69>] ? inet_twsk_deschedule+0x29/0xa0
[<ffffffff81460e69>] ? inet_twsk_deschedule+0x29/0xa0
[<ffffffff81460fd6>] ? inet_twsk_purge+0xf6/0x180
[<ffffffff81460f10>] ? inet_twsk_purge+0x30/0x180
[<ffffffff814760fc>] ? tcp_sk_exit_batch+0x1c/0x20
[<ffffffff8141c1d3>] ? ops_exit_list.clone.0+0x53/0x60
[<ffffffff8141c520>] ? cleanup_net+0x100/0x1b0
[<ffffffff81068c47>] ? process_one_work+0x187/0x4b0
[<ffffffff81068be1>] ? process_one_work+0x121/0x4b0
[<ffffffff8141c420>] ? cleanup_net+0x0/0x1b0
[<ffffffff8106a65c>] ? worker_thread+0x15c/0x330
[<ffffffff8106a500>] ? worker_thread+0x0/0x330
[<ffffffff8106f226>] ? kthread+0xb6/0xc0
[<ffffffff8108678d>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff81003c24>] ? kernel_thread_helper+0x4/0x10
[<ffffffff814df854>] ? restore_args+0x0/0x30
[<ffffffff8106f170>] ? kthread+0x0/0xc0
[<ffffffff81003c20>] ? kernel_thread_helper+0x0/0x10
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: Possible netfilter-related memory corruption in 2.6.37
From: Patrick McHardy @ 2011-02-18 18:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jan Engelhardt, Avi Kivity, netfilter-devel, Marcelo Tosatti,
nicolas prochazka, KVM list, netdev
In-Reply-To: <4D595DBD.3090005@trash.net>
[-- Attachment #1: Type: text/plain, Size: 391 bytes --]
Am 14.02.2011 17:52, schrieb Patrick McHardy:
> Am 14.02.2011 17:48, schrieb Eric Dumazet:
>> I am not sure, but I guess nf_reinject() needs a fix too ;)
>
> I agree. That one looks uglier though, I guess we'll have to
> iterate through all hooks to note the previous one.
How about this? Unfortunately I don't think we can avoid
iterating through all hooks without violating RCU rules.
[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 1009 bytes --]
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 74aebed..834bb07 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -235,6 +235,7 @@ int nf_queue(struct sk_buff *skb,
void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
{
struct sk_buff *skb = entry->skb;
+ struct nf_hook_ops *i, *prev;
struct list_head *elem = &entry->elem->list;
const struct nf_afinfo *afinfo;
@@ -244,8 +245,21 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
/* Continue traversal iff userspace said ok... */
if (verdict == NF_REPEAT) {
- elem = elem->prev;
- verdict = NF_ACCEPT;
+ prev = NULL;
+ list_for_each_entry_rcu(i, &nf_hooks[entry->pf][entry->hook],
+ list) {
+ if (&i->list == elem)
+ break;
+ prev = i;
+ }
+
+ if (prev == NULL ||
+ &i->list == &nf_hooks[entry->pf][entry->hook])
+ verdict = NF_DROP;
+ else {
+ elem = &prev->list;
+ verdict = NF_ACCEPT;
+ }
}
if (verdict == NF_ACCEPT) {
^ permalink raw reply related
* Re: [patch net-next-2.6] net: convert bonding to use rx_handler
From: Jiri Pirko @ 2011-02-18 18:47 UTC (permalink / raw)
To: Eric Dumazet
Cc: Patrick McHardy, netdev, davem, shemminger, fubar,
nicolas.2p.debian, andy
In-Reply-To: <1298045670.6201.73.camel@edumazet-laptop>
Fri, Feb 18, 2011 at 05:14:30PM CET, eric.dumazet@gmail.com wrote:
>Le vendredi 18 février 2011 à 16:50 +0100, Patrick McHardy a écrit :
>> On 18.02.2011 15:58, Jiri Pirko wrote:
>> > Fri, Feb 18, 2011 at 03:46:45PM CET, kaber@trash.net wrote:
>> >> Am 18.02.2011 15:27, schrieb Eric Dumazet:
>> >>> Le vendredi 18 février 2011 à 15:14 +0100, Jiri Pirko a écrit :
>> >>>
>> >>>> Do not know how to do it better. As for percpu variable, not only
>> >>>> origdev would have to be remembered but also probably skb pointer to
>> >>>> know if it's the first run on the skb or not. Can't really figure out a
>> >>>> better solution. Can you?
>> >>>
>> >>> I'll try and let you know.
>> >>
>> >> Why not simply do a lookup on skb->iif?
>> >
>> > Well I was trying to avoid iterating over list of devices for each
>> > incoming frame.
>> >
>>
>> Well, there are a couple of holes on 64 bit, perhaps you can rearrange
>> things and eliminate either iif or input_dev without increasing size
>> since they appear to be redundant.
>
>Jiri
>
>I dont understand why netif_rx() is needed in your patch.
I used netif_rx() because bridge and macvlan does that too. I did not see
a reason to not to do the same.
>
>Can we stack 10 bond devices or so ???
>
>If we avoid this stage and call the real thing (netif_receive_skb()),
>then we dont need adding a field in each skb, since it can be carried by
>a global variable (per cpu of course)
>
I'm probably missing something. How do netif_receive_skb() and
netif_rx() differ in this point of view, since both are calling:
"ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail);"
?
Still I see a problem with the percpu global variable. We would have to
store skb pointer there as well and in each __netif_receive_skb() call it
would have to be checked if it's different from the current one.
In that case store new skb and orig_Dev.
Leaving aside that global variables are evil in general, I still think
this is not nicer solution then to add skb->input_dev (although I
understand your arguments).
>bond_handle_frame() being called from __netif_receive_skb() I believe it
>can use netif_receive_skb() instead of netif_rx().
>
>Same remark for vlan_on_bond_hook()
>
>
>
^ 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