* Re: icmp redirects problem
From: Jarek Poplawski @ 2009-11-24 7:58 UTC (permalink / raw)
To: Alex Samad; +Cc: netdev
In-Reply-To: <20091124001230.GC14245@samad.com.au>
On Tue, Nov 24, 2009 at 11:12:30AM +1100, Alex Samad wrote:
> On Mon, Nov 23, 2009 at 10:58:38PM +0100, Jarek Poplawski wrote:
> > Alex Samad wrote, On 11/23/2009 05:31 AM:
> >
>
> [snip]
>
> > >
> > > laptop gets its ip from dchp server that make 192.168.11.1 the default
> > > gateway and its 192.168.11.1 that sends out the icmp redirect.
> >
> > Btw, it seems you should fix your routing (by adding sydrt01's eth0
> > the second ip or advertising 192.168.11.10 more) to avoid those
> > redirects.
>
> sorry I am lost on this statement, I can't add 192.168.11.10 to sydrt01
> as it is not physically connected to the 192.168.10.0/24 any more, which
> is why I had added the route on sydrt01 and which is why it send
> icmp_rediercts.
>
> I have updated the route table on each static machine, but the problem
> is on the machines that get their ip via dhcp - I haven't looked at
> pushing out route information via dhcp - I am not sure that it would
> work in a mixed windows / linux environment.
>
> what do you mean by advertising 192.168.11.10 more ?
I meant just what you've described, but wasn't sure of your config.
>
> >
> > >
> > > I had a quick look at the kernel tree for 2.6.31 (which is what I am
> > > using).
> >
> > ...
> >
> > > Line 680
> > > secure_redirects - BOOLEAN
> > > 681 Accept ICMP redirect messages only for gateways,
> > > 682 listed in default gateway list.
> > > 683 secure_redirects for the interface will be enabled if at
> > > least one of
> > > 684 conf/{all,interface}/secure_redirects is set to TRUE,
> > > 685 it will be disabled otherwise
> > > 686 default TRUE
> >
> > Very helpful links. So, as you wrote "the documentation seems to suggest"
> > something, and IMHO even if it doesn't, it's needlessly too concise
> > considering your "lost time", and I'd suggest you sending a patch to fix
> > this. (It seems it could "touch" shared_media, as well.)
>
> Which is wrong the code or the documentation and which part the test or
> the reliance on the shared_media or on the redirects flags
The code looks consistent to me. The documentation isn't wrong either,
until it only "seems to suggest", but it might be better, if it
metioned just what you tested: both things depend on accept_redirects.
Jarek P.
^ permalink raw reply
* Re: [PATCH 2/4] macvlan: cleanup rx statistics
From: Eric Dumazet @ 2009-11-24 8:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, netdev, David Miller, Stephen Hemminger, Herbert Xu,
Patrick Mullaney, Eric W. Biederman, Edge Virtual Bridging,
Anna Fischer, bridge, virtualization, Jens Osterkamp,
Gerhard Stenzel, Patrick McHardy, Mark Smith
In-Reply-To: <1259024166-28158-3-git-send-email-arnd@arndb.de>
Arnd Bergmann a écrit :
> We have very similar code for rx statistics in
> two places in the macvlan driver, with a third
> one being added in the next patch.
>
> Consolidate them into one function to improve
> overall readability of the driver.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/net/macvlan.c | 63 +++++++++++++++++++++++++-----------------------
> 1 files changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index ae2b5c7..a0dea23 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -116,42 +116,53 @@ static int macvlan_addr_busy(const struct macvlan_port *port,
> return 0;
> }
>
> +static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
> + int success, int multicast)
success and multicast should be declared as bool
> +{
> + struct macvlan_rx_stats *rx_stats;
> +
> + rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
> + rx_stats->rx_packets += success != 0;
> + rx_stats->rx_bytes += success ? length : 0;
> + rx_stats->multicast += success && multicast;
> + rx_stats->rx_errors += !success;
> +}
> +
I find following more readable, it probably generates more branches,
but avoid dirtying rx_errors if it is in another cache line.
if (likely(success)) {
rx_stats->rx_packets++;
rx_stats->rx_bytes += length;
if (multicast)
rx_stats->multicast++;
} else {
rx_stats->rx_errors++;
}
> - rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
> skb = skb_share_check(skb, GFP_ATOMIC);
> - if (skb == NULL) {
> - rx_stats->rx_errors++;
> - return NULL;
> - }
> -
> - rx_stats->rx_bytes += skb->len + ETH_HLEN;
> - rx_stats->rx_packets++;
> + macvlan_count_rx(vlan, skb->len + ETH_HLEN, likely(skb != NULL), 0);
its not _likely_ that skb != NULL, its a fact :)
-> macvlan_count_rx(vlan, skb->len + ETH_HLEN, true, false);
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Peter Zijlstra @ 2009-11-24 8:38 UTC (permalink / raw)
To: Waskiewicz Jr, Peter P
Cc: Yong Zhang, linux-kernel@vger.kernel.org,
arjan@linux.jf.intel.com, davem@davemloft.net,
netdev@vger.kernel.org, Thomas Gleixner
In-Reply-To: <Pine.WNT.4.64.0911231529240.10056@ppwaskie-MOBL2.amr.corp.intel.com>
On Mon, 2009-11-23 at 15:32 -0800, Waskiewicz Jr, Peter P wrote:
> Unfortunately, a driver can't. The irq_set_affinity() function isn't
> exported. I proposed a patch on netdev to export it, and then to tie down
> an interrupt using IRQF_NOBALANCING, so irqbalance won't touch it. That
> was rejected, since the driver is enforcing policy of the interrupt
> balancing, not irqbalance.
Why would a patch touching the irq subsystem go to netdev?
What is wrong with exporting irq_set_affinity(), and wtf do you need
IRQF_NOBALANCING for?
> I and Jesse Brandeburg had a meeting with Arjan about this. What we came
> up with was this interface, so drivers can set what they'd like to see, if
> irqbalance decides to honor it. That way interrupt affinity policies are
> set only by irqbalance, but this interface gives us a mechanism to hint to
> irqbalance what we'd like it to do.
If all you want is to expose policy to userspace then you don't need any
of this, simply expose the NICs home node through a sysfs device thingy
(I was under the impression its already there somewhere, but I can't
ever find anything in /sys).
No need what so ever to poke at the IRQ subsystem.
> Also, if you use the /proc interface to change smp_affinity on an
> interrupt without any of these changes, irqbalance will override it on its
> next poll interval. This also is not desirable.
This all sounds backwards.. we've got a perfectly functional interface
for affinity -- which people object to being used for some reason. So
you add another interface on top, and that is ok?
All the while not CC'ing the IRQ folks,.. brilliant approach.
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Peter P Waskiewicz Jr @ 2009-11-24 8:39 UTC (permalink / raw)
To: Yong Zhang
Cc: linux-kernel@vger.kernel.org, arjan@linux.jf.intel.com,
davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <2674af740911232117l275e933yaca3f1b8c1207bce@mail.gmail.com>
On Mon, 2009-11-23 at 22:17 -0700, Yong Zhang wrote:
> [snip]
> >>
> >> 1) I think you should consider CONFIG_CPUMASK_OFFSTACK which will affect
> >> node_affinity.
> >> 2) It seems like this patch can't work with SPARSE_IRQ.
> >
> > This mechanism isn't going to be used by any internal kernel mechanism
> > for determining interrupt placement or operation. It's purely something
> > that either a driver can modify, or external script (through /proc),
> > that irqbalance will make use of. If irqbalance isn't running, or the
> > current version of irqbalance doesn't support reading node_affinity,
> > then it won't affect the system's operation.
> >
> > If irqbalance does support it, it'll read whatever the supplied mask is,
> > and then will try and balance interrupts within that mask. It will bail
> > if the mask is invalid, or won't apply to the running system, just like
> > how putting a bogus mask into smp_affinity is ignored.
> >
> > If there's something I'm missing beyond this with the two suggestions
> > you've made (I looked into those two parameters and tried to draw
> > conclusions), please let me know.
>
> My two suggestions are both about your adding node_affinity. Before you can
> use this element, you must initialise it firstly. You can refer how
> irq_desc::affinity
> is used in function alloc_desc_masks().
> include/linux/irq.h:
> static inline bool alloc_desc_masks(struct irq_desc *desc, int node,
> bool boot)
> {
> gfp_t gfp = GFP_ATOMIC;
>
> if (boot)
> gfp = GFP_NOWAIT;
>
> #ifdef CONFIG_CPUMASK_OFFSTACK
> if (!alloc_cpumask_var_node(&desc->affinity, gfp, node))
> return false;
>
> #ifdef CONFIG_GENERIC_PENDING_IRQ
> if (!alloc_cpumask_var_node(&desc->pending_mask, gfp, node)) {
> free_cpumask_var(desc->affinity);
> return false;
> }
> #endif
> #endif
> return true;
> }
>
Ah, ok. I see what you were referring to now. Let me respin the patch
and send a second version.
Thanks Yong,
-PJ Waskiewicz
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Peter Zijlstra @ 2009-11-24 8:39 UTC (permalink / raw)
To: Arjan van de Ven
Cc: Peter P Waskiewicz Jr, Yong Zhang, linux-kernel@vger.kernel.org,
arjan@linux.jf.intel.com, davem@davemloft.net,
netdev@vger.kernel.org
In-Reply-To: <4B0B782A.4030901@linux.intel.com>
On Mon, 2009-11-23 at 22:07 -0800, Arjan van de Ven wrote:
> Peter Zijlstra wrote:
> > On Mon, 2009-11-23 at 01:36 -0800, Peter P Waskiewicz Jr wrote:
> >
> >> This mechanism isn't going to be used by any internal kernel mechanism
> >> for determining interrupt placement or operation. It's purely something
> >> that either a driver can modify, or external script (through /proc),
> >> that irqbalance will make use of. If irqbalance isn't running, or the
> >> current version of irqbalance doesn't support reading node_affinity,
> >> then it won't affect the system's operation.
> >>
> >> If irqbalance does support it, it'll read whatever the supplied mask is,
> >> and then will try and balance interrupts within that mask. It will bail
> >> if the mask is invalid, or won't apply to the running system, just like
> >> how putting a bogus mask into smp_affinity is ignored.
> >>
> >> If there's something I'm missing beyond this with the two suggestions
> >> you've made (I looked into those two parameters and tried to draw
> >> conclusions), please let me know.
> >
> > I don't see the point in adding it, if the driver wants to set a node
> > cpu mask it can already do that using the regular smp affinity settings.
> >
> > Same for userspace.
>
> the problem is that there is no way currently that the driver can communicate
> "I allocated all my metadata on THIS numa node". irqbalance and sysadmins need
> that to not make really stupid decisions.....
And what exactly is struct device::numa_node good for then?
^ permalink raw reply
* Re: [PATCH 2/4] macvlan: cleanup rx statistics
From: Arnd Bergmann @ 2009-11-24 8:45 UTC (permalink / raw)
To: virtualization
Cc: Eric Dumazet, Herbert Xu, Anna Fischer, netdev, bridge,
linux-kernel, Mark Smith, Gerhard Stenzel, Eric W. Biederman,
Jens Osterkamp, Patrick Mullaney, Stephen Hemminger,
Edge Virtual Bridging, David Miller
In-Reply-To: <4B0B9639.4070607@gmail.com>
On Tuesday 24 November 2009 08:15:53 Eric Dumazet wrote:
> Arnd Bergmann a écrit :
> >
> > +static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
> > + int success, int multicast)
>
> success and multicast should be declared as bool
ok
> > +{
> > + struct macvlan_rx_stats *rx_stats;
> > +
> > + rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
> > + rx_stats->rx_packets += success != 0;
> > + rx_stats->rx_bytes += success ? length : 0;
> > + rx_stats->multicast += success && multicast;
> > + rx_stats->rx_errors += !success;
> > +}
> > +
>
> I find following more readable, it probably generates more branches,
> but avoid dirtying rx_errors if it is in another cache line.
>
> if (likely(success)) {
> rx_stats->rx_packets++;
> rx_stats->rx_bytes += length;
> if (multicast)
> rx_stats->multicast++;
> } else {
> rx_stats->rx_errors++;
> }
Given that the structure only has four members and alloc_percpu requests
cache aligned data, it is rather likely to be in the same cache line.
I'll have a look at what gcc generates on x86-64 for both versions
and use the version you suggested unless it looks significantly more
expensive.
Since we're into micro-optimization territory, do you think it should
be marked inline or not?
> > - rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
> > skb = skb_share_check(skb, GFP_ATOMIC);
> > - if (skb == NULL) {
> > - rx_stats->rx_errors++;
> > - return NULL;
> > - }
> > -
> > - rx_stats->rx_bytes += skb->len + ETH_HLEN;
> > - rx_stats->rx_packets++;
> > + macvlan_count_rx(vlan, skb->len + ETH_HLEN, likely(skb != NULL), 0);
>
> its not _likely_ that skb != NULL, its a fact :)
>
> -> macvlan_count_rx(vlan, skb->len + ETH_HLEN, true, false);
I don't understand. Note how I removed the check for NULL above and
the skb pointer may be the result of a failing skb_clone().
Looking at this again, I actually introduced a bug by calling netif_rx
on a possibly NULL skb, I'll fix that.
Thanks!
Arnd <><
^ permalink raw reply
* Re: ixgbe question
From: Badalian Vyacheslav @ 2009-11-24 8:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Waskiewicz Jr, Peter P, Linux Netdev List
In-Reply-To: <4B0B8F52.3010005@gmail.com>
Eric Dumazet пишет:
> Waskiewicz Jr, Peter P a écrit :
>> Ok, I was confused earlier. I thought you were saying that all packets
>> were headed into a single Rx queue. This is different.
>>
>> Do you know what version of irqbalance you're running, or if it's running
>> at all? We've seen issues with irqbalance where it won't recognize the
>> ethernet device if the driver has been reloaded. In that case, it won't
>> balance the interrupts at all. If the default affinity was set to one
>> CPU, then well, you're screwed.
>>
>> My suggestion in this case is after you reload ixgbe and start your tests,
>> see if it all goes to one CPU. If it does, then restart irqbalance
>> (service irqbalance restart - or just kill it and restart by hand). Then
>> start running your test, and in 10 seconds you should see the interrupts
>> move and spread out.
>>
>> Let me know if this helps,
>
> Sure it helps !
>
> I tried without irqbalance and with irqbalance (Ubuntu 9.10 ships irqbalance 0.55-4)
> I can see irqbalance setting smp_affinities to 5555 or AAAA with no direct effect.
>
> I do receive 16 different irqs, but all serviced on one cpu.
>
> Only way to have irqs on different cpus is to manualy force irq affinities to be exclusive
> (one bit set in the mask, not several ones), and that is not optimal for moderate loads.
>
> echo 1 >`echo /proc/irq/*/fiber1-TxRx-0/../smp_affinity`
> echo 1 >`echo /proc/irq/*/fiber1-TxRx-1/../smp_affinity`
> echo 4 >`echo /proc/irq/*/fiber1-TxRx-2/../smp_affinity`
> echo 4 >`echo /proc/irq/*/fiber1-TxRx-3/../smp_affinity`
> echo 10 >`echo /proc/irq/*/fiber1-TxRx-4/../smp_affinity`
> echo 10 >`echo /proc/irq/*/fiber1-TxRx-5/../smp_affinity`
> echo 40 >`echo /proc/irq/*/fiber1-TxRx-6/../smp_affinity`
> echo 40 >`echo /proc/irq/*/fiber1-TxRx-7/../smp_affinity`
> echo 100 >`echo /proc/irq/*/fiber1-TxRx-8/../smp_affinity`
> echo 100 >`echo /proc/irq/*/fiber1-TxRx-9/../smp_affinity`
> echo 400 >`echo /proc/irq/*/fiber1-TxRx-10/../smp_affinity`
> echo 400 >`echo /proc/irq/*/fiber1-TxRx-11/../smp_affinity`
> echo 1000 >`echo /proc/irq/*/fiber1-TxRx-12/../smp_affinity`
> echo 1000 >`echo /proc/irq/*/fiber1-TxRx-13/../smp_affinity`
> echo 4000 >`echo /proc/irq/*/fiber1-TxRx-14/../smp_affinity`
> echo 4000 >`echo /proc/irq/*/fiber1-TxRx-15/../smp_affinity`
>
>
> One other problem is that after reload of ixgbe driver, link is 95% of the time
> at 1 Gbps speed, and I could not find an easy way to force it being 10 Gbps
>
> I run following script many times and stop it when 10 Gbps speed if reached.
>
> ethtool -A fiber0 rx off tx off
> ip link set fiber0 down
> ip link set fiber1 down
> sleep 2
> ethtool fiber0
> ethtool -s fiber0 speed 10000
> ethtool -s fiber1 speed 10000
> ethtool -r fiber0 &
> ethtool -r fiber1 &
> ethtool fiber0
> ip link set fiber1 up &
> ip link set fiber0 up &
> ethtool fiber0
>
> [ 33.625689] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 2.0.44-k2
> [ 33.625692] ixgbe: Copyright (c) 1999-2009 Intel Corporation.
> [ 33.625741] ixgbe 0000:07:00.0: PCI INT A -> GSI 32 (level, low) -> IRQ 32
> [ 33.625760] ixgbe 0000:07:00.0: setting latency timer to 64
> [ 33.735579] ixgbe 0000:07:00.0: irq 100 for MSI/MSI-X
> [ 33.735583] ixgbe 0000:07:00.0: irq 101 for MSI/MSI-X
> [ 33.735585] ixgbe 0000:07:00.0: irq 102 for MSI/MSI-X
> [ 33.735587] ixgbe 0000:07:00.0: irq 103 for MSI/MSI-X
> [ 33.735589] ixgbe 0000:07:00.0: irq 104 for MSI/MSI-X
> [ 33.735591] ixgbe 0000:07:00.0: irq 105 for MSI/MSI-X
> [ 33.735593] ixgbe 0000:07:00.0: irq 106 for MSI/MSI-X
> [ 33.735595] ixgbe 0000:07:00.0: irq 107 for MSI/MSI-X
> [ 33.735597] ixgbe 0000:07:00.0: irq 108 for MSI/MSI-X
> [ 33.735599] ixgbe 0000:07:00.0: irq 109 for MSI/MSI-X
> [ 33.735602] ixgbe 0000:07:00.0: irq 110 for MSI/MSI-X
> [ 33.735604] ixgbe 0000:07:00.0: irq 111 for MSI/MSI-X
> [ 33.735606] ixgbe 0000:07:00.0: irq 112 for MSI/MSI-X
> [ 33.735608] ixgbe 0000:07:00.0: irq 113 for MSI/MSI-X
> [ 33.735610] ixgbe 0000:07:00.0: irq 114 for MSI/MSI-X
> [ 33.735612] ixgbe 0000:07:00.0: irq 115 for MSI/MSI-X
> [ 33.735614] ixgbe 0000:07:00.0: irq 116 for MSI/MSI-X
> [ 33.735633] ixgbe: 0000:07:00.0: ixgbe_init_interrupt_scheme: Multiqueue Enabled: Rx Queue count = 16, Tx Queue count = 16
> [ 33.735638] ixgbe 0000:07:00.0: (PCI Express:5.0Gb/s:Width x8) 00:1b:21:4a:fe:54
> [ 33.735722] ixgbe 0000:07:00.0: MAC: 2, PHY: 11, SFP+: 5, PBA No: e66562-003
> [ 33.738111] ixgbe 0000:07:00.0: Intel(R) 10 Gigabit Network Connection
> [ 33.738135] ixgbe 0000:07:00.1: PCI INT B -> GSI 42 (level, low) -> IRQ 42
> [ 33.738151] ixgbe 0000:07:00.1: setting latency timer to 64
> [ 33.853526] ixgbe 0000:07:00.1: irq 117 for MSI/MSI-X
> [ 33.853529] ixgbe 0000:07:00.1: irq 118 for MSI/MSI-X
> [ 33.853532] ixgbe 0000:07:00.1: irq 119 for MSI/MSI-X
> [ 33.853534] ixgbe 0000:07:00.1: irq 120 for MSI/MSI-X
> [ 33.853536] ixgbe 0000:07:00.1: irq 121 for MSI/MSI-X
> [ 33.853538] ixgbe 0000:07:00.1: irq 122 for MSI/MSI-X
> [ 33.853540] ixgbe 0000:07:00.1: irq 123 for MSI/MSI-X
> [ 33.853542] ixgbe 0000:07:00.1: irq 124 for MSI/MSI-X
> [ 33.853544] ixgbe 0000:07:00.1: irq 125 for MSI/MSI-X
> [ 33.853546] ixgbe 0000:07:00.1: irq 126 for MSI/MSI-X
> [ 33.853548] ixgbe 0000:07:00.1: irq 127 for MSI/MSI-X
> [ 33.853550] ixgbe 0000:07:00.1: irq 128 for MSI/MSI-X
> [ 33.853552] ixgbe 0000:07:00.1: irq 129 for MSI/MSI-X
> [ 33.853554] ixgbe 0000:07:00.1: irq 130 for MSI/MSI-X
> [ 33.853556] ixgbe 0000:07:00.1: irq 131 for MSI/MSI-X
> [ 33.853558] ixgbe 0000:07:00.1: irq 132 for MSI/MSI-X
> [ 33.853560] ixgbe 0000:07:00.1: irq 133 for MSI/MSI-X
> [ 33.853580] ixgbe: 0000:07:00.1: ixgbe_init_interrupt_scheme: Multiqueue Enabled: Rx Queue count = 16, Tx Queue count = 16
> [ 33.853585] ixgbe 0000:07:00.1: (PCI Express:5.0Gb/s:Width x8) 00:1b:21:4a:fe:55
> [ 33.853669] ixgbe 0000:07:00.1: MAC: 2, PHY: 11, SFP+: 5, PBA No: e66562-003
> [ 33.855956] ixgbe 0000:07:00.1: Intel(R) 10 Gigabit Network Connection
>
> [ 85.208233] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: RX/TX
> [ 85.237453] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: RX/TX
> [ 96.080713] ixgbe: fiber1 NIC Link is Down
> [ 102.094610] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: None
> [ 102.119572] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: None
> [ 142.524691] ixgbe: fiber1 NIC Link is Down
> [ 148.421332] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: None
> [ 148.449465] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: None
> [ 160.728643] ixgbe: fiber1 NIC Link is Down
> [ 172.832301] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: None
> [ 173.659038] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: None
> [ 184.554501] ixgbe: fiber0 NIC Link is Down
> [ 185.376273] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: None
> [ 186.493598] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: None
> [ 190.564383] ixgbe: fiber0 NIC Link is Down
> [ 191.391149] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: None
> [ 192.484492] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: None
> [ 192.545424] ixgbe: fiber1 NIC Link is Down
> [ 205.858197] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: None
> [ 206.684940] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: None
> [ 211.991875] ixgbe: fiber1 NIC Link is Down
> [ 220.833478] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: None
> [ 220.833630] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: None
> [ 229.804853] ixgbe: fiber1 NIC Link is Down
> [ 248.395672] ixgbe: fiber0 NIC Link is Up 1 Gbps, Flow Control: None
> [ 249.222408] ixgbe: fiber1 NIC Link is Up 1 Gbps, Flow Control: None
> [ 484.631598] ixgbe: fiber1 NIC Link is Down
> [ 490.138931] ixgbe: fiber1 NIC Link is Up 10 Gbps, Flow Control: None
> [ 490.167880] ixgbe: fiber0 NIC Link is Up 10 Gbps, Flow Control: None
> --
> 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
>
>
May be its Flow Director?
Multuqueue in this network card work only if you set 1 queue to 1 cpu core in smp_affinity :(
In README:
Intel(R) Ethernet Flow Director
-------------------------------
Supports advanced filters that direct receive packets by their flows to
different queues. Enables tight control on routing a flow in the platform.
Matches flows and CPU cores for flow affinity. Supports multiple parameters
for flexible flow classification and load balancing.
Flow director is enabled only if the kernel is multiple TX queue capable.
An included script (set_irq_affinity.sh) automates setting the IRQ to CPU
affinity.
You can verify that the driver is using Flow Director by looking at the counter
in ethtool: fdir_miss and fdir_match.
The following three parameters impact Flow Director.
FdirMode
--------
Valid Range: 0-2 (0=off, 1=ATR, 2=Perfect filter mode)
Default Value: 1
Flow Director filtering modes.
FdirPballoc
-----------
Valid Range: 0-2 (0=64k, 1=128k, 2=256k)
Default Value: 0
Flow Director allocated packet buffer size.
AtrSampleRate
--------------
Valid Range: 1-100
Default Value: 20
Software ATR Tx packet sample rate. For example, when set to 20, every 20th
packet, looks to see if the packet will create a new flow.
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Peter P Waskiewicz Jr @ 2009-11-24 8:59 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Yong Zhang, linux-kernel@vger.kernel.org,
arjan@linux.jf.intel.com, davem@davemloft.net,
netdev@vger.kernel.org, Thomas Gleixner
In-Reply-To: <1259051902.4531.1053.camel@laptop>
On Tue, 2009-11-24 at 01:38 -0700, Peter Zijlstra wrote:
> On Mon, 2009-11-23 at 15:32 -0800, Waskiewicz Jr, Peter P wrote:
>
> > Unfortunately, a driver can't. The irq_set_affinity() function isn't
> > exported. I proposed a patch on netdev to export it, and then to tie down
> > an interrupt using IRQF_NOBALANCING, so irqbalance won't touch it. That
> > was rejected, since the driver is enforcing policy of the interrupt
> > balancing, not irqbalance.
>
> Why would a patch touching the irq subsystem go to netdev?
The only change to the IRQ subsystem was:
EXPORT_SYMBOL(irq_set_affinity);
The majority of the changeset was for the ixgbe driver.
> What is wrong with exporting irq_set_affinity(), and wtf do you need
> IRQF_NOBALANCING for?
>
Again, the pushback I received was with allowing anything other than
irqbalance to dictate interrupt affinity policy.
And if I set interrupt affinity from the driver or from /proc,
irqbalance will happily rebalance the interrupt elsewhere. The
IRQF_NOBALANCING flag will prevent irqbalance from being able to move
the interrupt.
> > I and Jesse Brandeburg had a meeting with Arjan about this. What we came
> > up with was this interface, so drivers can set what they'd like to see, if
> > irqbalance decides to honor it. That way interrupt affinity policies are
> > set only by irqbalance, but this interface gives us a mechanism to hint to
> > irqbalance what we'd like it to do.
>
> If all you want is to expose policy to userspace then you don't need any
> of this, simply expose the NICs home node through a sysfs device thingy
> (I was under the impression its already there somewhere, but I can't
> ever find anything in /sys).
>
> No need what so ever to poke at the IRQ subsystem.
The point is we need something common that the kernel side (whether a
driver or /proc can modify) that irqbalance can use.
> > Also, if you use the /proc interface to change smp_affinity on an
> > interrupt without any of these changes, irqbalance will override it on its
> > next poll interval. This also is not desirable.
>
> This all sounds backwards.. we've got a perfectly functional interface
> for affinity -- which people object to being used for some reason. So
> you add another interface on top, and that is ok?
>
But it's not functional. If I set the affinity in smp_affinity, then
irqbalance will override it 10 seconds later.
> All the while not CC'ing the IRQ folks,.. brilliant approach.
If I knew who I should CC, I'd be happy to add them. Can you provide
email addresses please?
Cheers,
-PJ Waskiewicz
^ permalink raw reply
* Re: ixgbe question
From: Peter P Waskiewicz Jr @ 2009-11-24 9:07 UTC (permalink / raw)
To: Eric Dumazet
Cc: robert@herjulf.net, Jesper Dangaard Brouer, Linux Netdev List
In-Reply-To: <4B0B8F52.3010005@gmail.com>
On Tue, 2009-11-24 at 00:46 -0700, Eric Dumazet wrote:
> Waskiewicz Jr, Peter P a écrit :
> > Ok, I was confused earlier. I thought you were saying that all packets
> > were headed into a single Rx queue. This is different.
> >
> > Do you know what version of irqbalance you're running, or if it's running
> > at all? We've seen issues with irqbalance where it won't recognize the
> > ethernet device if the driver has been reloaded. In that case, it won't
> > balance the interrupts at all. If the default affinity was set to one
> > CPU, then well, you're screwed.
> >
> > My suggestion in this case is after you reload ixgbe and start your tests,
> > see if it all goes to one CPU. If it does, then restart irqbalance
> > (service irqbalance restart - or just kill it and restart by hand). Then
> > start running your test, and in 10 seconds you should see the interrupts
> > move and spread out.
> >
> > Let me know if this helps,
>
> Sure it helps !
>
> I tried without irqbalance and with irqbalance (Ubuntu 9.10 ships irqbalance 0.55-4)
> I can see irqbalance setting smp_affinities to 5555 or AAAA with no direct effect.
>
> I do receive 16 different irqs, but all serviced on one cpu.
>
> Only way to have irqs on different cpus is to manualy force irq affinities to be exclusive
> (one bit set in the mask, not several ones), and that is not optimal for moderate loads.
>
> echo 1 >`echo /proc/irq/*/fiber1-TxRx-0/../smp_affinity`
> echo 1 >`echo /proc/irq/*/fiber1-TxRx-1/../smp_affinity`
> echo 4 >`echo /proc/irq/*/fiber1-TxRx-2/../smp_affinity`
> echo 4 >`echo /proc/irq/*/fiber1-TxRx-3/../smp_affinity`
> echo 10 >`echo /proc/irq/*/fiber1-TxRx-4/../smp_affinity`
> echo 10 >`echo /proc/irq/*/fiber1-TxRx-5/../smp_affinity`
> echo 40 >`echo /proc/irq/*/fiber1-TxRx-6/../smp_affinity`
> echo 40 >`echo /proc/irq/*/fiber1-TxRx-7/../smp_affinity`
> echo 100 >`echo /proc/irq/*/fiber1-TxRx-8/../smp_affinity`
> echo 100 >`echo /proc/irq/*/fiber1-TxRx-9/../smp_affinity`
> echo 400 >`echo /proc/irq/*/fiber1-TxRx-10/../smp_affinity`
> echo 400 >`echo /proc/irq/*/fiber1-TxRx-11/../smp_affinity`
> echo 1000 >`echo /proc/irq/*/fiber1-TxRx-12/../smp_affinity`
> echo 1000 >`echo /proc/irq/*/fiber1-TxRx-13/../smp_affinity`
> echo 4000 >`echo /proc/irq/*/fiber1-TxRx-14/../smp_affinity`
> echo 4000 >`echo /proc/irq/*/fiber1-TxRx-15/../smp_affinity`
>
>
> One other problem is that after reload of ixgbe driver, link is 95% of the time
> at 1 Gbps speed, and I could not find an easy way to force it being 10 Gbps
>
You might have this elsewhere, but it sounds like you're connecting back
to back with another 82599 NIC. Our optics in that NIC are dual-rate,
and the software mechanism that tries to "autoneg" link speed gets out
of sync easily in back-to-back setups.
If it's really annoying, and you're willing to run with a local patch to
disable the autotry mechanism, try this:
diff --git a/drivers/net/ixgbe/ixgbe_main.c
b/drivers/net/ixgbe/ixgbe_main.c
index a5036f7..62c0915 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -4670,6 +4670,10 @@ static void ixgbe_multispeed_fiber_task(struct
work_struct *work)
autoneg = hw->phy.autoneg_advertised;
if ((!autoneg) && (hw->mac.ops.get_link_capabilities))
hw->mac.ops.get_link_capabilities(hw, &autoneg,
&negotiation);
+
+ /* force 10G only */
+ autoneg = IXGBE_LINK_SPEED_10GB_FULL;
+
if (hw->mac.ops.setup_link)
hw->mac.ops.setup_link(hw, autoneg, negotiation, true);
adapter->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
Cheers,
-PJ
^ permalink raw reply related
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Peter Zijlstra @ 2009-11-24 9:08 UTC (permalink / raw)
To: Peter P Waskiewicz Jr
Cc: Yong Zhang, linux-kernel@vger.kernel.org,
arjan@linux.jf.intel.com, davem@davemloft.net,
netdev@vger.kernel.org, Thomas Gleixner
In-Reply-To: <1259053156.2631.21.camel@ppwaskie-mobl2>
On Tue, 2009-11-24 at 00:59 -0800, Peter P Waskiewicz Jr wrote:
> > This all sounds backwards.. we've got a perfectly functional interface
> > for affinity -- which people object to being used for some reason. So
> > you add another interface on top, and that is ok?
> >
>
> But it's not functional. If I set the affinity in smp_affinity, then
> irqbalance will override it 10 seconds later.
And here I was thinking the kernel round-robins IRQ delivery on the mask
specified there. Are you talking about some daft userspace thing that
writes into the irq smp_affinity to effect irq balancing?
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Peter P Waskiewicz Jr @ 2009-11-24 9:15 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Yong Zhang, linux-kernel@vger.kernel.org,
arjan@linux.jf.intel.com, davem@davemloft.net,
netdev@vger.kernel.org, Thomas Gleixner
In-Reply-To: <1259053736.4531.1097.camel@laptop>
On Tue, 2009-11-24 at 02:08 -0700, Peter Zijlstra wrote:
> On Tue, 2009-11-24 at 00:59 -0800, Peter P Waskiewicz Jr wrote:
> > > This all sounds backwards.. we've got a perfectly functional interface
> > > for affinity -- which people object to being used for some reason. So
> > > you add another interface on top, and that is ok?
> > >
> >
> > But it's not functional. If I set the affinity in smp_affinity, then
> > irqbalance will override it 10 seconds later.
>
> And here I was thinking the kernel round-robins IRQ delivery on the mask
> specified there. Are you talking about some daft userspace thing that
> writes into the irq smp_affinity to effect irq balancing?
>
Yep. That's exactly what irqbalance does.
Cheers,
-PJ
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Peter Zijlstra @ 2009-11-24 9:15 UTC (permalink / raw)
To: Peter P Waskiewicz Jr
Cc: Yong Zhang, linux-kernel@vger.kernel.org,
arjan@linux.jf.intel.com, davem@davemloft.net,
netdev@vger.kernel.org, Thomas Gleixner
In-Reply-To: <1259053156.2631.21.camel@ppwaskie-mobl2>
On Tue, 2009-11-24 at 00:59 -0800, Peter P Waskiewicz Jr wrote:
>
> > All the while not CC'ing the IRQ folks,.. brilliant approach.
>
> If I knew who I should CC, I'd be happy to add them. Can you provide
> email addresses please?
Since most people can't seen to read a simple MAINTAINERS file, some
other people wrote a script to read it for you:
# scripts/get_maintainer.pl -f kernel/irq/manage.c
Ingo Molnar <mingo@elte.hu>
Thomas Gleixner <tglx@linutronix.de>
linux-kernel@vger.kernel.org
Another option is to do something like:
# git log kernel/irq/manage.c | grep Author | head -30 | awk
'{ t[$0]++; } END { for (i in t) { print t[i] " " i; }}' | sort -rn
10 Author: Thomas Gleixner <tglx@linutronix.de>
9 Author: Ingo Molnar <mingo@elte.hu>
3 Author: Magnus Damm <damm@igel.co.jp>
2 Author: Linus Torvalds <torvalds@linux-foundation.org>
...
^ permalink raw reply
* Re: [PATCH 2/4] macvlan: cleanup rx statistics
From: Arnd Bergmann @ 2009-11-24 9:28 UTC (permalink / raw)
To: virtualization
Cc: Eric Dumazet, Herbert Xu, Anna Fischer, netdev, bridge,
linux-kernel, Mark Smith, Gerhard Stenzel, Eric W. Biederman,
Jens Osterkamp, Patrick Mullaney, Stephen Hemminger,
Edge Virtual Bridging, David Miller
In-Reply-To: <200911240845.14454.arnd@arndb.de>
On Tuesday 24 November 2009 08:45:14 Arnd Bergmann wrote:
> On Tuesday 24 November 2009 08:15:53 Eric Dumazet wrote:
> > Arnd Bergmann a écrit :
> > I find following more readable, it probably generates more branches,
> > but avoid dirtying rx_errors if it is in another cache line.
> >
> > if (likely(success)) {
> > rx_stats->rx_packets++;
> > rx_stats->rx_bytes += length;
> > if (multicast)
> > rx_stats->multicast++;
> > } else {
> > rx_stats->rx_errors++;
> > }
>
> Given that the structure only has four members and alloc_percpu requests
> cache aligned data, it is rather likely to be in the same cache line.
>
> I'll have a look at what gcc generates on x86-64 for both versions
> and use the version you suggested unless it looks significantly more
> expensive.
Ok, that's what I got for trying to be clever. My version did not avoid
any branches, just created more code. I'll fold this update into my
patches then:
---
macvlan: cleanups
Use bool instead of int for flags, don't misoptimize rx counters,
avoid accessing a NULL skb.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 3db96b9..ff5f0b0 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -119,19 +119,23 @@ static int macvlan_addr_busy(const struct macvlan_port *port,
}
static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
- int success, int multicast)
+ bool success, bool multicast)
{
struct macvlan_rx_stats *rx_stats;
rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
- rx_stats->rx_packets += success != 0;
- rx_stats->rx_bytes += success ? length : 0;
- rx_stats->multicast += success && multicast;
- rx_stats->rx_errors += !success;
+ if (likely(success)) {
+ rx_stats->rx_packets++;;
+ rx_stats->rx_bytes += length;
+ if (multicast)
+ rx_stats->multicast++;
+ } else {
+ rx_stats->rx_errors++;
+ }
}
static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
- const struct ethhdr *eth, int local)
+ const struct ethhdr *eth, bool local)
{
if (!skb)
return NET_RX_DROP;
@@ -173,7 +177,7 @@ static void macvlan_broadcast(struct sk_buff *skb,
err = macvlan_broadcast_one(nskb, vlan->dev, eth,
mode == MACVLAN_MODE_BRIDGE);
macvlan_count_rx(vlan, skb->len + ETH_HLEN,
- likely(err == NET_RX_SUCCESS), 1);
+ err == NET_RX_SUCCESS, 1);
}
}
}
@@ -186,6 +190,7 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
const struct macvlan_dev *vlan;
const struct macvlan_dev *src;
struct net_device *dev;
+ int len;
port = rcu_dereference(skb->dev->macvlan_port);
if (port == NULL)
@@ -218,8 +223,11 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
kfree_skb(skb);
return NULL;
}
+ len = skb->len + ETH_HLEN;
skb = skb_share_check(skb, GFP_ATOMIC);
- macvlan_count_rx(vlan, skb->len + ETH_HLEN, likely(skb != NULL), 0);
+ macvlan_count_rx(vlan, len, skb != NULL, 0);
+ if (!skb)
+ return NULL;
skb->dev = dev;
skb->pkt_type = PACKET_HOST;
@@ -248,7 +256,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
int length = skb->len + ETH_HLEN;
int ret = dev_forward_skb(dest->dev, skb);
macvlan_count_rx(dest, length,
- likely(ret == NET_RX_SUCCESS), 0);
+ ret == NET_RX_SUCCESS, 0);
return NET_XMIT_SUCCESS;
}
^ permalink raw reply related
* [PATCH v2] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Peter P Waskiewicz Jr @ 2009-11-24 9:35 UTC (permalink / raw)
To: linux-kernel, arjan; +Cc: mingo, tglx, yong.zhang0, davem, netdev
This patchset adds a new CPU mask for SMP systems to the irq_desc
struct. It also exposes an API for underlying device drivers to
assist irqbalance in making smarter decisions when balancing, especially
in a NUMA environment. For example, an ethernet driver with MSI-X may
wish to limit the CPUs that an interrupt can be balanced within to
stay on a single NUMA node. Current irqbalance operation can move the
interrupt off the node, resulting in cross-node memory accesses and
locks.
The API is a get/set API within the kernel, along with a /proc entry
for the interrupt.
Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
---
include/linux/interrupt.h | 8 ++++++
include/linux/irq.h | 8 ++++++
kernel/irq/manage.c | 32 +++++++++++++++++++++++++
kernel/irq/proc.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 105 insertions(+), 0 deletions(-)
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..9fd08aa 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -208,6 +208,8 @@ extern cpumask_var_t irq_default_affinity;
extern int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask);
extern int irq_can_set_affinity(unsigned int irq);
extern int irq_select_affinity(unsigned int irq);
+extern int irq_set_node_affinity(unsigned int irq,
+ const struct cpumask *cpumask);
#else /* CONFIG_SMP */
@@ -223,6 +225,12 @@ static inline int irq_can_set_affinity(unsigned int irq)
static inline int irq_select_affinity(unsigned int irq) { return 0; }
+static inline int irq_set_node_affinity(unsigned int irq,
+ const struct cpumask *m)
+{
+ return -EINVAL;
+}
+
#endif /* CONFIG_SMP && CONFIG_GENERIC_HARDIRQS */
#ifdef CONFIG_GENERIC_HARDIRQS
diff --git a/include/linux/irq.h b/include/linux/irq.h
index ae9653d..819cda0 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -166,6 +166,7 @@ struct irq_2_iommu;
* @lock: locking for SMP
* @affinity: IRQ affinity on SMP
* @node: node index useful for balancing
+ * @node_affinity: irq mask hints for irqbalance
* @pending_mask: pending rebalanced interrupts
* @threads_active: number of irqaction threads currently running
* @wait_for_threads: wait queue for sync_irq to wait for threaded handlers
@@ -196,6 +197,7 @@ struct irq_desc {
#ifdef CONFIG_SMP
cpumask_var_t affinity;
unsigned int node;
+ cpumask_var_t node_affinity;
#ifdef CONFIG_GENERIC_PENDING_IRQ
cpumask_var_t pending_mask;
#endif
@@ -445,9 +447,15 @@ static inline bool alloc_desc_masks(struct irq_desc *desc, int node,
if (!alloc_cpumask_var_node(&desc->affinity, gfp, node))
return false;
+ if (!alloc_cpumask_var_node(&desc->node_affinity, gfp, node)) {
+ free_cpumask_var(desc->affinity);
+ return false;
+ }
+
#ifdef CONFIG_GENERIC_PENDING_IRQ
if (!alloc_cpumask_var_node(&desc->pending_mask, gfp, node)) {
free_cpumask_var(desc->affinity);
+ free_cpumask_var(desc->node_affinity);
return false;
}
#endif
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 7305b29..9e80783 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -138,6 +138,38 @@ int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
return 0;
}
+/**
+ * irq_set_node_affinity - Set the CPU mask this interrupt can run on
+ * @irq: Interrupt to modify
+ * @cpumask: CPU mask to assign to the interrupt
+ *
+ */
+int irq_set_node_affinity(unsigned int irq, const struct cpumask *cpumask)
+{
+ struct irq_desc *desc = irq_to_desc(irq);
+ unsigned long flags;
+
+ spin_lock_irqsave(&desc->lock, flags);
+ cpumask_copy(desc->node_affinity, cpumask);
+ spin_unlock_irqrestore(&desc->lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL(irq_set_node_affinity);
+
+/**
+ * irq_get_node_affinity - Get the CPU mask this interrupt can run on
+ * @irq: Interrupt to get information
+ *
+ */
+struct cpumask *irq_get_node_affinity(unsigned int irq)
+{
+ struct irq_desc *desc = irq_to_desc(irq);
+
+ return desc->node_affinity;
+}
+EXPORT_SYMBOL(irq_get_node_affinity);
+
#ifndef CONFIG_AUTO_IRQ_AFFINITY
/*
* Generic version of the affinity autoselector.
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 0832145..192e3fb 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -31,6 +31,16 @@ static int irq_affinity_proc_show(struct seq_file *m, void *v)
return 0;
}
+static int irq_node_affinity_proc_show(struct seq_file *m, void *v)
+{
+ struct irq_desc *desc = irq_to_desc((long)m->private);
+ const struct cpumask *mask = desc->node_affinity;
+
+ seq_cpumask(m, mask);
+ seq_putc(m, '\n');
+ return 0;
+}
+
#ifndef is_affinity_mask_valid
#define is_affinity_mask_valid(val) 1
#endif
@@ -78,11 +88,46 @@ free_cpumask:
return err;
}
+static ssize_t irq_node_affinity_proc_write(struct file *file,
+ const char __user *buffer, size_t count, loff_t *pos)
+{
+ unsigned int irq = (int)(long)PDE(file->f_path.dentry->d_inode)->data;
+ cpumask_var_t new_value;
+ int err;
+
+ if (no_irq_affinity || irq_balancing_disabled(irq))
+ return -EIO;
+
+ if (!alloc_cpumask_var(&new_value, GFP_KERNEL))
+ return -ENOMEM;
+
+ err = cpumask_parse_user(buffer, count, new_value);
+ if (err)
+ goto free_cpumask;
+
+ if (!is_affinity_mask_valid(new_value)) {
+ err = -EINVAL;
+ goto free_cpumask;
+ }
+
+ irq_set_node_affinity(irq, new_value);
+ err = count;
+
+free_cpumask:
+ free_cpumask_var(new_value);
+ return err;
+}
+
static int irq_affinity_proc_open(struct inode *inode, struct file *file)
{
return single_open(file, irq_affinity_proc_show, PDE(inode)->data);
}
+static int irq_node_affinity_proc_open(struct inode *inode, struct file *f)
+{
+ return single_open(f, irq_node_affinity_proc_show, PDE(inode)->data);
+}
+
static const struct file_operations irq_affinity_proc_fops = {
.open = irq_affinity_proc_open,
.read = seq_read,
@@ -91,6 +136,14 @@ static const struct file_operations irq_affinity_proc_fops = {
.write = irq_affinity_proc_write,
};
+static const struct file_operations irq_node_affinity_proc_fops = {
+ .open = irq_node_affinity_proc_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+ .write = irq_node_affinity_proc_write,
+};
+
static int default_affinity_show(struct seq_file *m, void *v)
{
seq_cpumask(m, irq_default_affinity);
@@ -230,6 +283,10 @@ void register_irq_proc(unsigned int irq, struct irq_desc *desc)
/* create /proc/irq/<irq>/smp_affinity */
proc_create_data("smp_affinity", 0600, desc->dir,
&irq_affinity_proc_fops, (void *)(long)irq);
+
+ /* create /proc/irq/<irq>/node_affinity */
+ proc_create_data("node_affinity", 0600, desc->dir,
+ &irq_node_affinity_proc_fops, (void *)(long)irq);
#endif
proc_create_data("spurious", 0444, desc->dir,
^ permalink raw reply related
* Re: [PATCH 1/4] veth: move loopback logic to common location
From: Patrick McHardy @ 2009-11-24 9:51 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Eric Dumazet, linux-kernel, netdev, David Miller,
Stephen Hemminger, Herbert Xu, Patrick Mullaney,
Eric W. Biederman, Edge Virtual Bridging, Anna Fischer, bridge,
virtualization, Jens Osterkamp, Gerhard Stenzel, Mark Smith
In-Reply-To: <1259024166-28158-2-git-send-email-arnd@arndb.de>
Arnd Bergmann wrote:
> +int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
> +{
> + skb_orphan(skb);
> +
> + if (!(dev->flags & IFF_UP))
> + return NET_RX_DROP;
> +
> + if (skb->len > (dev->mtu + dev->hard_header_len))
> + return NET_RX_DROP;
> +
> + skb_dst_drop(skb);
> + skb->tstamp.tv64 = 0;
> + skb->pkt_type = PACKET_HOST;
> + skb->protocol = eth_type_trans(skb, dev);
> + skb->mark = 0;
skb->mark clearing should stay private to veth since its usually
supposed to stay intact. The only exception is packets crossing
namespaces, where they should appear like a freshly received skbs.
> + secpath_reset(skb);
> + nf_reset(skb);
> + return netif_rx(skb);
> +}
> +EXPORT_SYMBOL_GPL(dev_forward_skb);
^ permalink raw reply
* Re: ixgbe question
From: Eric Dumazet @ 2009-11-24 9:55 UTC (permalink / raw)
To: Peter P Waskiewicz Jr
Cc: robert@herjulf.net, Jesper Dangaard Brouer, Linux Netdev List
In-Reply-To: <1259053673.2631.30.camel@ppwaskie-mobl2>
Peter P Waskiewicz Jr a écrit :
> You might have this elsewhere, but it sounds like you're connecting back
> to back with another 82599 NIC. Our optics in that NIC are dual-rate,
> and the software mechanism that tries to "autoneg" link speed gets out
> of sync easily in back-to-back setups.
>
> If it's really annoying, and you're willing to run with a local patch to
> disable the autotry mechanism, try this:
>
> diff --git a/drivers/net/ixgbe/ixgbe_main.c
> b/drivers/net/ixgbe/ixgbe_main.c
> index a5036f7..62c0915 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -4670,6 +4670,10 @@ static void ixgbe_multispeed_fiber_task(struct
> work_struct *work)
> autoneg = hw->phy.autoneg_advertised;
> if ((!autoneg) && (hw->mac.ops.get_link_capabilities))
> hw->mac.ops.get_link_capabilities(hw, &autoneg,
> &negotiation);
> +
> + /* force 10G only */
> + autoneg = IXGBE_LINK_SPEED_10GB_FULL;
> +
> if (hw->mac.ops.setup_link)
> hw->mac.ops.setup_link(hw, autoneg, negotiation, true);
> adapter->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
Thanks ! This did the trick :)
If I am not mistaken, number of TX queues should be capped by number of possible cpus ?
Its currently a fixed 128 value, allocating 128*128 = 16384 bytes,
and polluting "tc -s -d class show dev fiber0" output.
[PATCH net-next-2.6] ixgbe: Do not allocate too many netdev txqueues
Instead of allocating 128 struct netdev_queue per device, use the minimum
value between 128 and number of possible cpus, to reduce ram usage and
"tc -s -d class show dev ..." output
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index ebcec30..ec2508d 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5582,7 +5583,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
pci_set_master(pdev);
pci_save_state(pdev);
- netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), MAX_TX_QUEUES);
+ netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter),
+ min_t(unsigned int,
+ MAX_TX_QUEUES,
+ num_possible_cpus()));
if (!netdev) {
err = -ENOMEM;
goto err_alloc_etherdev;
^ permalink raw reply related
* Re: [PATCH 1/4] veth: move loopback logic to common location
From: Arnd Bergmann @ 2009-11-24 10:02 UTC (permalink / raw)
To: Patrick McHardy
Cc: Eric Dumazet, linux-kernel, netdev, David Miller,
Stephen Hemminger, Herbert Xu, Patrick Mullaney,
Eric W. Biederman, Edge Virtual Bridging, Anna Fischer, bridge,
virtualization, Jens Osterkamp, Gerhard Stenzel, Mark Smith
In-Reply-To: <4B0BAC97.6010000@trash.net>
On Tuesday 24 November 2009 09:51:19 Patrick McHardy wrote:
> > + skb_dst_drop(skb);
> > + skb->tstamp.tv64 = 0;
> > + skb->pkt_type = PACKET_HOST;
> > + skb->protocol = eth_type_trans(skb, dev);
> > + skb->mark = 0;
>
> skb->mark clearing should stay private to veth since its usually
> supposed to stay intact. The only exception is packets crossing
> namespaces, where they should appear like a freshly received skbs.
But isn't that what we want in macvlan as well when we're
forwarding from one downstream interface to another?
I did all my testing with macvlan interfaces in separate namespaces
communicating with each other, so I'd assume that we should always
clear skb->mark and skb->dst in this function. Maybe I should make
the documentation clearer?
---
net: clarify documentation of dev_forward_skb
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1433,6 +1433,10 @@ static inline void net_timestamp(struct sk_buff *skb)
* dev_forward_skb can be used for injecting an skb from the
* start_xmit function of one device into the receive queue
* of another device.
+ *
+ * The receiving device may be in another namespace, so
+ * we have to clear all information in the skb that could
+ * impact namespace isolation.
*/
int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
{
^ permalink raw reply
* Re: ixgbe question
From: Peter P Waskiewicz Jr @ 2009-11-24 10:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: robert@herjulf.net, Jesper Dangaard Brouer, Linux Netdev List
In-Reply-To: <4B0BADA6.7080602@gmail.com>
On Tue, 2009-11-24 at 02:55 -0700, Eric Dumazet wrote:
> Peter P Waskiewicz Jr a écrit :
>
> > You might have this elsewhere, but it sounds like you're connecting back
> > to back with another 82599 NIC. Our optics in that NIC are dual-rate,
> > and the software mechanism that tries to "autoneg" link speed gets out
> > of sync easily in back-to-back setups.
> >
> > If it's really annoying, and you're willing to run with a local patch to
> > disable the autotry mechanism, try this:
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_main.c
> > b/drivers/net/ixgbe/ixgbe_main.c
> > index a5036f7..62c0915 100644
> > --- a/drivers/net/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ixgbe/ixgbe_main.c
> > @@ -4670,6 +4670,10 @@ static void ixgbe_multispeed_fiber_task(struct
> > work_struct *work)
> > autoneg = hw->phy.autoneg_advertised;
> > if ((!autoneg) && (hw->mac.ops.get_link_capabilities))
> > hw->mac.ops.get_link_capabilities(hw, &autoneg,
> > &negotiation);
> > +
> > + /* force 10G only */
> > + autoneg = IXGBE_LINK_SPEED_10GB_FULL;
> > +
> > if (hw->mac.ops.setup_link)
> > hw->mac.ops.setup_link(hw, autoneg, negotiation, true);
> > adapter->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
>
> Thanks ! This did the trick :)
>
> If I am not mistaken, number of TX queues should be capped by number of possible cpus ?
>
> Its currently a fixed 128 value, allocating 128*128 = 16384 bytes,
> and polluting "tc -s -d class show dev fiber0" output.
>
Yes, this is a stupid issue we haven't gotten around to fixing yet.
This looks fine to me. Thanks for putting it together.
> [PATCH net-next-2.6] ixgbe: Do not allocate too many netdev txqueues
>
> Instead of allocating 128 struct netdev_queue per device, use the minimum
> value between 128 and number of possible cpus, to reduce ram usage and
> "tc -s -d class show dev ..." output
>
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index ebcec30..ec2508d 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -5582,7 +5583,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
> pci_set_master(pdev);
> pci_save_state(pdev);
>
> - netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), MAX_TX_QUEUES);
> + netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter),
> + min_t(unsigned int,
> + MAX_TX_QUEUES,
> + num_possible_cpus()));
> if (!netdev) {
> err = -ENOMEM;
> goto err_alloc_etherdev;
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Thomas Gleixner @ 2009-11-24 10:07 UTC (permalink / raw)
To: Peter P Waskiewicz Jr
Cc: Peter Zijlstra, Yong Zhang, linux-kernel@vger.kernel.org,
arjan@linux.jf.intel.com, davem@davemloft.net,
netdev@vger.kernel.org, Jesse Barnes
In-Reply-To: <1259053156.2631.21.camel@ppwaskie-mobl2>
On Tue, 24 Nov 2009, Peter P Waskiewicz Jr wrote:
> On Tue, 2009-11-24 at 01:38 -0700, Peter Zijlstra wrote:
> > On Mon, 2009-11-23 at 15:32 -0800, Waskiewicz Jr, Peter P wrote:
> >
> > > Unfortunately, a driver can't. The irq_set_affinity() function isn't
> > > exported. I proposed a patch on netdev to export it, and then to tie down
> > > an interrupt using IRQF_NOBALANCING, so irqbalance won't touch it. That
> > > was rejected, since the driver is enforcing policy of the interrupt
> > > balancing, not irqbalance.
> >
> > Why would a patch touching the irq subsystem go to netdev?
>
> The only change to the IRQ subsystem was:
>
> EXPORT_SYMBOL(irq_set_affinity);
Which is still touching the generic irq subsystem and needs the ack of
the relevant maintainer. If there is a need to expose such an
interface to drivers then the maintainer wants to know exactly why and
needs to be part of the discussion of alternative solutions. Otherwise
you waste time on implementing stuff like the current patch which is
definitely not going anywhere near the irq subsystem.
> > If all you want is to expose policy to userspace then you don't need any
> > of this, simply expose the NICs home node through a sysfs device thingy
> > (I was under the impression its already there somewhere, but I can't
> > ever find anything in /sys).
> >
> > No need what so ever to poke at the IRQ subsystem.
>
> The point is we need something common that the kernel side (whether a
> driver or /proc can modify) that irqbalance can use.
/sys/class/net/ethX/device/numa_node
perhaps ?
> > > Also, if you use the /proc interface to change smp_affinity on an
> > > interrupt without any of these changes, irqbalance will override it on its
> > > next poll interval. This also is not desirable.
> >
> > This all sounds backwards.. we've got a perfectly functional interface
> > for affinity -- which people object to being used for some reason. So
> > you add another interface on top, and that is ok?
> >
>
> But it's not functional. If I set the affinity in smp_affinity, then
> irqbalance will override it 10 seconds later.
And to work around the brain wreckage of irqbalanced you want to
fiddle in the irq code instead of teaching irqbalanced to handle node
affinities ?
The only thing which is worth to investigate is whether the irq core
code should honour the dev->numa_node setting and restrict the
possible irq affinity settings to that node. If a device is tied to a
node it makes a certain amount of sense to do that.
But such a change would not need a new interface in the irq core and
definitely not a new cpumask_t member in the irq_desc structure to
store a node affinity which can be expressed with a simple
integer.
But this needs more thoughts and I want to know more about the
background and the reasoning for such a change.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 1/4] veth: move loopback logic to common location
From: Patrick McHardy @ 2009-11-24 10:17 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Eric Dumazet, linux-kernel, netdev, David Miller,
Stephen Hemminger, Herbert Xu, Patrick Mullaney,
Eric W. Biederman, Edge Virtual Bridging, Anna Fischer, bridge,
virtualization, Jens Osterkamp, Gerhard Stenzel, Mark Smith
In-Reply-To: <200911241002.20904.arnd@arndb.de>
Arnd Bergmann wrote:
> On Tuesday 24 November 2009 09:51:19 Patrick McHardy wrote:
>>> + skb_dst_drop(skb);
>>> + skb->tstamp.tv64 = 0;
>>> + skb->pkt_type = PACKET_HOST;
>>> + skb->protocol = eth_type_trans(skb, dev);
>>> + skb->mark = 0;
>> skb->mark clearing should stay private to veth since its usually
>> supposed to stay intact. The only exception is packets crossing
>> namespaces, where they should appear like a freshly received skbs.
>
> But isn't that what we want in macvlan as well when we're
> forwarding from one downstream interface to another?
In the TX direction you can use the mark for TC classification
on the underlying device.
> I did all my testing with macvlan interfaces in separate namespaces
> communicating with each other, so I'd assume that we should always
> clear skb->mark and skb->dst in this function.
Good point, in that case we probably should clear it as well. But
in the non-namespace case the TC classification currently works and
this is consistent with any other virtual device driver, so it
should continue to work.
^ permalink raw reply
* [PATCH] igb: fix misinterpreted return value of pci_enable_msix
From: Stefan Assmann @ 2009-11-24 10:31 UTC (permalink / raw)
To: netdev; +Cc: Andy Gospodarek, alexander.h.duyck, davem
From: Stefan Assmann <sassmann@redhat.com>
In the igb driver a return value of 0 of function pci_enable_msix is
interpreted as an error case. The correct behaviour is to check < 0 for error
values.
Signed-off-by: Stefan Assmann <sassmann@redhat.com>
---
drivers/net/igb/igb_main.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index bb1a6ee..745481d 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -694,7 +694,7 @@ static void igb_set_interrupt_capability(struct igb_adapter *adapter)
err = pci_enable_msix(adapter->pdev,
adapter->msix_entries,
numvecs);
- if (err == 0)
+ if (err < 0)
goto out;
igb_reset_interrupt_capability(adapter);
--
1.6.5.2
^ permalink raw reply related
* Re: [PATCH 1/4] veth: move loopback logic to common location
From: Arnd Bergmann @ 2009-11-24 10:34 UTC (permalink / raw)
To: virtualization
Cc: Patrick McHardy, Herbert Xu, Eric Dumazet, Anna Fischer, netdev,
bridge, linux-kernel, Mark Smith, Gerhard Stenzel,
Eric W. Biederman, Jens Osterkamp, Patrick Mullaney,
Stephen Hemminger, Edge Virtual Bridging, David Miller
In-Reply-To: <4B0BB2A7.5040707@trash.net>
On Tuesday 24 November 2009 10:17:11 Patrick McHardy wrote:
> Arnd Bergmann wrote:
> > On Tuesday 24 November 2009 09:51:19 Patrick McHardy wrote:
> >>> + skb_dst_drop(skb);
> >>> + skb->tstamp.tv64 = 0;
> >>> + skb->pkt_type = PACKET_HOST;
> >>> + skb->protocol = eth_type_trans(skb, dev);
> >>> + skb->mark = 0;
> >> skb->mark clearing should stay private to veth since its usually
> >> supposed to stay intact. The only exception is packets crossing
> >> namespaces, where they should appear like a freshly received skbs.
> >
> > But isn't that what we want in macvlan as well when we're
> > forwarding from one downstream interface to another?
>
> In the TX direction you can use the mark for TC classification
> on the underlying device.
I don't use dev_forward_skb for the case where the data is sent
to the underlying device, so the TC classification should stay
intact.
> > I did all my testing with macvlan interfaces in separate namespaces
> > communicating with each other, so I'd assume that we should always
> > clear skb->mark and skb->dst in this function.
>
> Good point, in that case we probably should clear it as well. But
> in the non-namespace case the TC classification currently works and
> this is consistent with any other virtual device driver, so it
> should continue to work.
Do you think we should be able to use TC to direct traffic between
macvlans on the same underlying device in bridge mode? It does sound
useful, but I'm not sure how to implement that or if you'd expect
it to work with the current code. If we support that, it should probably
also work with namespaces, by consuming the mark in the macvlan
and veth drivers.
Arnd <><
^ permalink raw reply
* Re: [PATCH 1/4] veth: move loopback logic to common location
From: Patrick McHardy @ 2009-11-24 10:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: virtualization, Herbert Xu, Eric Dumazet, Anna Fischer, netdev,
bridge, linux-kernel, Mark Smith, Gerhard Stenzel,
Eric W. Biederman, Jens Osterkamp, Patrick Mullaney,
Stephen Hemminger, Edge Virtual Bridging, David Miller
In-Reply-To: <200911241034.43961.arnd@arndb.de>
Arnd Bergmann wrote:
> On Tuesday 24 November 2009 10:17:11 Patrick McHardy wrote:
>> Arnd Bergmann wrote:
>>> On Tuesday 24 November 2009 09:51:19 Patrick McHardy wrote:
>>>>> + skb_dst_drop(skb);
>>>>> + skb->tstamp.tv64 = 0;
>>>>> + skb->pkt_type = PACKET_HOST;
>>>>> + skb->protocol = eth_type_trans(skb, dev);
>>>>> + skb->mark = 0;
>>>> skb->mark clearing should stay private to veth since its usually
>>>> supposed to stay intact. The only exception is packets crossing
>>>> namespaces, where they should appear like a freshly received skbs.
>>> But isn't that what we want in macvlan as well when we're
>>> forwarding from one downstream interface to another?
>> In the TX direction you can use the mark for TC classification
>> on the underlying device.
>
> I don't use dev_forward_skb for the case where the data is sent
> to the underlying device, so the TC classification should stay
> intact.
Right, I see. This looks fine.
>>> I did all my testing with macvlan interfaces in separate namespaces
>>> communicating with each other, so I'd assume that we should always
>>> clear skb->mark and skb->dst in this function.
>> Good point, in that case we probably should clear it as well. But
>> in the non-namespace case the TC classification currently works and
>> this is consistent with any other virtual device driver, so it
>> should continue to work.
>
> Do you think we should be able to use TC to direct traffic between
> macvlans on the same underlying device in bridge mode? It does sound
> useful, but I'm not sure how to implement that or if you'd expect
> it to work with the current code. If we support that, it should probably
> also work with namespaces, by consuming the mark in the macvlan
> and veth drivers.
I don't think its necessary, we bypass outgoing queuing anyways.
But if you'd want to add it, just keeping the skb->mark clearing
in veth should work from what I can tell.
^ permalink raw reply
* Re: [PATCH] igb: fix misinterpreted return value of pci_enable_msix
From: Florian Westphal @ 2009-11-24 10:40 UTC (permalink / raw)
To: Stefan Assmann; +Cc: netdev, Andy Gospodarek, alexander.h.duyck, davem
In-Reply-To: <4B0BB5FE.2000907@redhat.com>
Stefan Assmann <sassmann@redhat.com> wrote:
> From: Stefan Assmann <sassmann@redhat.com>
>
> In the igb driver a return value of 0 of function pci_enable_msix is
> interpreted as an error case. The correct behaviour is to check < 0 for error
> values.
>
> Signed-off-by: Stefan Assmann <sassmann@redhat.com>
> ---
> drivers/net/igb/igb_main.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
> index bb1a6ee..745481d 100644
> --- a/drivers/net/igb/igb_main.c
> +++ b/drivers/net/igb/igb_main.c
> @@ -694,7 +694,7 @@ static void igb_set_interrupt_capability(struct igb_adapter *adapter)
> err = pci_enable_msix(adapter->pdev,
> adapter->msix_entries,
> numvecs);
> - if (err == 0)
> + if (err < 0)
> goto out;
>
The existing code looks correct to me:
if (err == 0)
goto out;
igb_reset_interrupt_capability(adapter);
/* If we can't do MSI-X, try MSI * */
msi_only:
so the "goto out" appears to be the "success" case.
^ permalink raw reply
* Re: [PATCH 2/4] macvlan: cleanup rx statistics
From: Patrick McHardy @ 2009-11-24 10:41 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Eric Dumazet, linux-kernel, netdev, David Miller,
Stephen Hemminger, Herbert Xu, Patrick Mullaney,
Eric W. Biederman, Edge Virtual Bridging, Anna Fischer, bridge,
virtualization, Jens Osterkamp, Gerhard Stenzel, Mark Smith
In-Reply-To: <1259024166-28158-3-git-send-email-arnd@arndb.de>
Arnd Bergmann wrote:
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index ae2b5c7..a0dea23 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -116,42 +116,53 @@ static int macvlan_addr_busy(const struct macvlan_port *port,
> return 0;
> }
>
> +static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
Please use unsigned int for length values.
Regarding Eric's comments, I also think it would be more readable to use
if (success) {
...
} else {
...
}
> + int success, int multicast)
> +{
> + struct macvlan_rx_stats *rx_stats;
> +
> + rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
> + rx_stats->rx_packets += success != 0;
> + rx_stats->rx_bytes += success ? length : 0;
> + rx_stats->multicast += success && multicast;
> + rx_stats->rx_errors += !success;
> +}
> +
> +static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
> + const struct ethhdr *eth)
> +{
> + if (!skb)
> + return NET_RX_DROP;
> +
> + skb->dev = dev;
> + if (!compare_ether_addr_64bits(eth->h_dest,
> + dev->broadcast))
This would fit on one line without reducing readability.
> + skb->pkt_type = PACKET_BROADCAST;
> + else
> + skb->pkt_type = PACKET_MULTICAST;
> +
> + return netif_rx(skb);
> +}
^ 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