* 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
* Re: [PATCH 3/4] macvlan: implement bridge, VEPA and private mode
From: Patrick McHardy @ 2009-11-24 10:42 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-4-git-send-email-arnd@arndb.de>
Arnd Bergmann wrote:
> This allows each macvlan slave device to be in one
> of three modes, depending on the use case:
>
> MACVLAN_PRIVATE:
> The device never communicates with any other device
> on the same upper_dev. This even includes frames
> coming back from a reflective relay, where supported
> by the adjacent bridge.
>
> MACVLAN_VEPA:
> The new Virtual Ethernet Port Aggregator (VEPA) mode,
> we assume that the adjacent bridge returns all frames
> where both source and destination are local to the
> macvlan port, i.e. the bridge is set up as a reflective
> relay.
> Broadcast frames coming in from the upper_dev get
> flooded to all macvlan interfaces in VEPA mode.
> We never deliver any frames locally.
>
> MACVLAN_BRIDGE:
> We provide the behavior of a simple bridge between
> different macvlan interfaces on the same port. Frames
> from one interface to another one get delivered directly
> and are not sent out externally. Broadcast frames get
> flooded to all other bridge ports and to the external
> interface, but when they come back from a reflective
> relay, we don't deliver them again.
> Since we know all the MAC addresses, the macvlan bridge
> mode does not require learning or STP like the bridge
> module does.
This looks pretty nice. Some stylistic nitpicking below :)
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index a0dea23..b840b3a 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -29,9 +29,16 @@
> #include <linux/if_link.h>
> #include <linux/if_macvlan.h>
> #include <net/rtnetlink.h>
> +#include <net/xfrm.h>
Do we really need this?
> @@ -129,11 +137,14 @@ static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
> }
>
> static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
> - const struct ethhdr *eth)
> + const struct ethhdr *eth, int local)
bool local?
> {
> if (!skb)
> return NET_RX_DROP;
>
> + if (local)
> + return dev_forward_skb(dev, skb);
> +
> skb->dev = dev;
> if (!compare_ether_addr_64bits(eth->h_dest,
> dev->broadcast))
> @@ -145,7 +156,9 @@ static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
> }
>
> static void macvlan_broadcast(struct sk_buff *skb,
> - const struct macvlan_port *port)
> + const struct macvlan_port *port,
> + struct net_device *src,
> + enum macvlan_mode mode)
> {
> const struct ethhdr *eth = eth_hdr(skb);
> const struct macvlan_dev *vlan;
> @@ -159,8 +172,12 @@ static void macvlan_broadcast(struct sk_buff *skb,
>
> for (i = 0; i < MACVLAN_HASH_SIZE; i++) {
> hlist_for_each_entry_rcu(vlan, n, &port->vlan_hash[i], hlist) {
> + if ((vlan->dev == src) || !(vlan->mode & mode))
Please remove those unnecessary parentheses around the
device comparison.
> @@ -173,6 +190,7 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
> const struct ethhdr *eth = eth_hdr(skb);
> const struct macvlan_port *port;
> const struct macvlan_dev *vlan;
> + const struct macvlan_dev *src;
> struct net_device *dev;
>
> port = rcu_dereference(skb->dev->macvlan_port);
> @@ -180,7 +198,20 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
> return skb;
>
> if (is_multicast_ether_addr(eth->h_dest)) {
> - macvlan_broadcast(skb, port);
> + src = macvlan_hash_lookup(port, eth->h_source);
> + if (!src)
> + /* frame comes from an external address */
> + macvlan_broadcast(skb, port, NULL, MACVLAN_MODE_PRIVATE
> + | MACVLAN_MODE_VEPA | MACVLAN_MODE_BRIDGE);
The '|' should go on the first line.
> + else if (src->mode == MACVLAN_MODE_VEPA)
> + /* flood to everyone except source */
> + macvlan_broadcast(skb, port, src->dev,
> + MACVLAN_MODE_VEPA | MACVLAN_MODE_BRIDGE);
> + else if (src->mode == MACVLAN_MODE_BRIDGE)
> + /* flood only to VEPA ports, bridge ports
> + already saw the frame */
Multi-line comments should begin every line with '*'.
> + macvlan_broadcast(skb, port, src->dev,
> + MACVLAN_MODE_VEPA);
Please align the mode values (in all cases above) to the arguments
on the line above.
> return skb;
> }
>
> @@ -203,18 +234,46 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
> return NULL;
> }
>
> +static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + const struct macvlan_dev *vlan = netdev_priv(dev);
> + const struct macvlan_port *port = vlan->port;
> + const struct macvlan_dev *dest;
> +
> + if (vlan->mode == MACVLAN_MODE_BRIDGE) {
> + const struct ethhdr *eth = (void *)skb->data;
eth_hdr()?
> +
> + /* send to other bridge ports directly */
> + if (is_multicast_ether_addr(eth->h_dest)) {
> + macvlan_broadcast(skb, port, dev, MACVLAN_MODE_BRIDGE);
> + goto xmit_world;
> + }
> +
> + dest = macvlan_hash_lookup(port, eth->h_dest);
> + if (dest && dest->mode == MACVLAN_MODE_BRIDGE) {
> + int length = skb->len + ETH_HLEN;
unsigned int for length values please.
> + int ret = dev_forward_skb(dest->dev, skb);
> + macvlan_count_rx(dest, length,
> + likely(ret == NET_RX_SUCCESS), 0);
> +
> + return NET_XMIT_SUCCESS;
> + }
> + }
> +
> +xmit_world:
> + skb->dev = vlan->lowerdev;
> + return dev_queue_xmit(skb);
> +}
^ permalink raw reply
* Re: [PATCH] igb: fix misinterpreted return value of pci_enable_msix
From: Stefan Assmann @ 2009-11-24 10:51 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev, Andy Gospodarek, alexander.h.duyck, davem
In-Reply-To: <20091124104049.GB21600@Chamillionaire.breakpoint.cc>
On 24.11.2009 11:40, Florian Westphal wrote:
> 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.
Hi Florian,
thanks for pointing this out. I've jumped to conclusions too fast.
There's a problem with igb MSI-X in RHEL5 in the kdump case and this
fixed it for me. But you're right, it's not the proper solution. I'll
report back when I have more information.
Stefan
--
Stefan Assmann | Red Hat GmbH
Software Engineer | Otto-Hahn-Strasse 20, 85609 Dornach
| HR: Amtsgericht Muenchen HRB 153243
| GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com | Michael Cunningham, Charles Cachera
^ permalink raw reply
* Re: [PATCH 4/4] macvlan: export macvlan mode through netlink
From: Patrick McHardy @ 2009-11-24 10:53 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-5-git-send-email-arnd@arndb.de>
Arnd Bergmann wrote:
> @@ -600,6 +594,18 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
> if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
> return -EADDRNOTAVAIL;
> }
> +
> + if (data && data[IFLA_MACVLAN_MODE]) {
> + u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> + switch (mode) {
> + case MACVLAN_MODE_PRIVATE:
> + case MACVLAN_MODE_VEPA:
> + case MACVLAN_MODE_BRIDGE:
> + break;
> + default:
> + return -EINVAL;
EINVAL is quite unspecific. In this case I think EOPNOTSUPP would
be fine and provide more information.
> + }
> + }
> return 0;
> }
> @@ -664,6 +670,13 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,
> vlan->dev = dev;
> vlan->port = port;
>
> + vlan->mode = MACVLAN_MODE_VEPA;
> + if (data && data[IFLA_MACVLAN_MODE]) {
> + u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> +
> + vlan->mode = mode;
This looks a bit strange, like cut-and-paste without reformatting :)
I'd suggest to simply use "vlan->mode = nla_get_u32(...)".
> + }
> +
> err = register_netdevice(dev);
> if (err < 0)
> return err;
> @@ -685,6 +698,39 @@ static void macvlan_dellink(struct net_device *dev, struct list_head *head)
> macvlan_port_destroy(port->dev);
> }
>
> +static int macvlan_changelink(struct net_device *dev,
> + struct nlattr *tb[], struct nlattr *data[])
> +{
> + struct macvlan_dev *vlan = netdev_priv(dev);
> + if (data && data[IFLA_MACVLAN_MODE]) {
> + u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> + vlan->mode = mode;
Same here.
> + }
> +
> + return 0;
> +}
^ permalink raw reply
* Re: macvlan: fix gso_max_size setting
From: Patrick McHardy @ 2009-11-24 10:57 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, netdev
In-Reply-To: <20091124010817.GB13660@gondor.apana.org.au>
Herbert Xu wrote:
> On Mon, Nov 23, 2009 at 11:33:05PM +0100, Patrick McHardy wrote:
>> Perhaps we should simply fall back to software in that case.
>> Compile tested only.
>
> Sure.
>
> Since ixgbe appears to be the only driver in our entire tree
> that has this limitation, I think it might make more sense to
> move this special-case check into it and get it to do software
> GSO just like tg3 does for a few other cases that the hardware
> can't handle.
Sure, that would also be fine. I'll leave that for the Intel
guys however :)
^ permalink raw reply
* [patch]reset_resume for cdc-ether
From: Oliver Neukum @ 2009-11-24 10:59 UTC (permalink / raw)
To: David Brownell, netdev, USB list, David S. Miller
Hi,
this is quite trivial but might be convenient to many users.
Regards
Oliver
Signed-off-by: Oliver Neukum <oliver@neukum.org>
--
commit 31deb426ccea8faf91f4373c1f12c98bb0d82c73
Author: Oliver Neukum <oliver@neukum.org>
Date: Tue Nov 24 08:30:41 2009 +0100
cdc-ether:imlement reset_resume()
Normal resume can do double duty as reset_resume() for this driver
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index ed55024..11f73dd 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -617,6 +617,7 @@ static struct usb_driver cdc_driver = {
.disconnect = usbnet_disconnect,
.suspend = usbnet_suspend,
.resume = usbnet_resume,
+ .reset_resume = usbnet_resume,
.supports_autosuspend = 1,
};
^ permalink raw reply related
* Re: [PATCH v2] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Thomas Gleixner @ 2009-11-24 11:07 UTC (permalink / raw)
To: Peter P Waskiewicz Jr
Cc: linux-kernel, arjan, mingo, yong.zhang0, davem, netdev
In-Reply-To: <20091124093518.3909.16435.stgit@ppwaskie-hc2.jf.intel.com>
On Tue, 24 Nov 2009, Peter P Waskiewicz Jr wrote:
> 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.
And what does the kernel do with this information and why are we not
using the existing device/numa_node information ?
> +extern int irq_set_node_affinity(unsigned int irq,
> + const struct cpumask *cpumask);
A node can be described with a single integer, right ?
> +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;
Yikes. Why should user space be allowed to write to that file ? And
the whole business is what for ? Storing that value in the irq_desc
data structure for use space to read out again ?
Cool design. We provide storage space for user space applications in
the kernel now ?
See also my earlier reply in the thread. This patch is just adding
code and memory bloat while not solving anything at all.
Again, this is going nowhere else than into /dev/null.
Thanks,
tglx
^ permalink raw reply
* [ tc ] RTNETLINK answers: Invalid argument
From: thomas yang @ 2009-11-24 11:12 UTC (permalink / raw)
To: netdev
# tc filter add dev eth1 parent 1: protocol ip prio 10 u32 match ip
src 192.168.1.0/24 flowid 1:16 action mirred egress mirror dev wlan0
Action 3 device wlan0 ifindex 4
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
# uname -a
Linux localhost.localdomain 2.6.30.8-64.fc11.i586 #1 SMP Fri Sep 25
04:30:19 EDT 2009 i686 i686 i386 GNU/Linux
OS : Fedora 11
^ permalink raw reply
* Re: large packet loss take2 2.6.31.x
From: Jarek Poplawski @ 2009-11-24 11:19 UTC (permalink / raw)
To: Caleb Cushing
Cc: e1000-devel, netdev, Frans Pop, Jesse Brandeburg, linux-kernel,
Andi Kleen, Jeff Kirsher
In-Reply-To: <81bfc67a0911232217n41b9ac02w3b7770b789e5d209@mail.gmail.com>
On Tue, Nov 24, 2009 at 01:17:09AM -0500, Caleb Cushing wrote:
> > Btw, currently I don't consider this dropping means there has to be
> > a bug. It could be otherwise - a feature... e.g. when a new kernel
> > can transmit faster (then dropping in some other, slower place can
> > happen).
>
> um... where would it be dropping that we wouldn't have a bug? I mean
> sure faster is great... but if it makes my network not work right...
E.g. if it were dropped because of a queue overflow (but it doesn't
seem to be the case, at least at your box) or because of memory
problems while handling a lot of traffic.
>
> I've added all (I think) information you've asked for to the bug
> http://bugzilla.kernel.org/show_bug.cgi?id=13835 except for ethtool
> and netstat on the router side. ethtool complains about not having
> driver or capability (maybe because it's a 2.4 kernel?) and the
> version of netstat doesn't support -s. I disabled everything that I
> can think of that would send/receive packets before doing the test
> client side, except dhcp/dns windows box's were probably sending some
> broadcasts too. but the traffic should be pretty low. I did remember
> to set the txqueuelen didn't seem to make a difference
Alas it's not all information I asked. E.g. "netstat -s before faulty
kernel" and "netstat -s after faulty kernel" seem to be the same file:
netstat_after.slave4.log.gz. Anyway, since there are problems with
getting stats from the router we still can't compare them, or check
for the dropped stats. (Btw, could you check for /proc/net/softnet_stat
yet?)
So, it might be the kernel problem you reported, but there is not
enough data to prove it. Then my proposal is to try to repeat this
problem in more "testing friendly" conditions - preferably against
some other, more up-to-date linux box, if possible?
> only error in dmesg I see is
>
> e1000e 0000:00:19.0: pci_enable_pcie_error_reporting failed 0xfffffffb
I added e1000e maintainers to CC to have a look at this warning.
Jarek P.
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
^ permalink raw reply
* [PATCH net-next-2.6] ixgbe: Fix TX stats accounting
From: Eric Dumazet @ 2009-11-24 11:37 UTC (permalink / raw)
To: Peter P Waskiewicz Jr, Jeff Kirsher
Cc: robert@herjulf.net, Jesper Dangaard Brouer, Linux Netdev List,
David S. Miller
In-Reply-To: <1259057164.2631.36.camel@ppwaskie-mobl2>
Several cpus can update netdev->stats.tx_bytes & netdev->stats.tx_packets
in parallel. In this case, TX stats are under estimated and false sharing
takes place.
After a pktgen session sending exactly 200000000 packets :
# ifconfig fiber0 | grep TX
TX packets:198501982 errors:0 dropped:0 overruns:0 carrier:0
Multi queue devices should instead use txq->tx_bytes & txq->tx_packets
in their xmit() method (appropriate txq lock already held by caller, no
cache line miss), or use appropriate locking.
After patch, same pktgen session gives :
# ifconfig fiber0 | grep TX
TX packets:200000000 errors:0 dropped:0 overruns:0 carrier:0
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
drivers/net/ixgbe/ixgbe_main.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index ebcec30..1cea120 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -425,8 +425,6 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
tx_ring->total_packets += total_packets;
tx_ring->stats.packets += total_packets;
tx_ring->stats.bytes += total_bytes;
- netdev->stats.tx_bytes += total_bytes;
- netdev->stats.tx_packets += total_packets;
return (count < tx_ring->work_limit);
}
@@ -5249,6 +5247,7 @@ static netdev_tx_t ixgbe_xmit_frame(struct sk_buff *skb,
{
struct ixgbe_adapter *adapter = netdev_priv(netdev);
struct ixgbe_ring *tx_ring;
+ struct netdev_queue *txq;
unsigned int first;
unsigned int tx_flags = 0;
u8 hdr_len = 0;
@@ -5345,6 +5344,9 @@ static netdev_tx_t ixgbe_xmit_frame(struct sk_buff *skb,
tx_ring->atr_count = 0;
}
}
+ txq = netdev_get_tx_queue(netdev, r_idx);
+ txq->tx_bytes += skb->len;
+ txq->tx_packets++;
ixgbe_tx_queue(adapter, tx_ring, tx_flags, count, skb->len,
hdr_len);
ixgbe_maybe_stop_tx(netdev, tx_ring, DESC_NEEDED);
@@ -5359,19 +5361,6 @@ static netdev_tx_t ixgbe_xmit_frame(struct sk_buff *skb,
}
/**
- * ixgbe_get_stats - Get System Network Statistics
- * @netdev: network interface device structure
- *
- * Returns the address of the device statistics structure.
- * The statistics are actually updated from the timer callback.
- **/
-static struct net_device_stats *ixgbe_get_stats(struct net_device *netdev)
-{
- /* only return the current stats */
- return &netdev->stats;
-}
-
-/**
* ixgbe_set_mac - Change the Ethernet Address of the NIC
* @netdev: network interface device structure
* @p: pointer to an address structure
@@ -5501,7 +5490,6 @@ static const struct net_device_ops ixgbe_netdev_ops = {
.ndo_stop = ixgbe_close,
.ndo_start_xmit = ixgbe_xmit_frame,
.ndo_select_queue = ixgbe_select_queue,
- .ndo_get_stats = ixgbe_get_stats,
.ndo_set_rx_mode = ixgbe_set_rx_mode,
.ndo_set_multicast_list = ixgbe_set_rx_mode,
.ndo_validate_addr = eth_validate_addr,
^ permalink raw reply related
* Re: [PATCH 1/1] Defer skb allocation for both mergeable buffers and big packets in virtio_net
From: Michael S. Tsirkin @ 2009-11-24 11:37 UTC (permalink / raw)
To: Rusty Russell
Cc: Shirley Ma, Eric Dumazet, Avi Kivity, netdev, kvm, linux-kernel,
Hollis Blanchard
In-Reply-To: <200911240854.24054.rusty@rustcorp.com.au>
On Tue, Nov 24, 2009 at 08:54:23AM +1030, Rusty Russell wrote:
> On Tue, 24 Nov 2009 02:37:01 am Shirley Ma wrote:
> > > > + skb = (struct sk_buff *)buf;
> > > This cast is unnecessary, but a comment would be nice:
> >
> > Without this cast there is a compile warning.
>
> Hi Shirley,
>
> Looks like buf is a void *, so no cast should be necessary. But I could
> be reading the patch wrong.
>
> > > However, I question whether making it 16 byte is the right thing: the
> > > ethernet header is 14 bytes long, so don't we want 8 bytes of padding?
> >
> > Because in QEMU it requires 10 bytes header in a separately, so one page
> > is used to share between virtio_net_hdr header which is 10 bytes head
> > and rest of data. So I put 6 bytes offset here between two buffers. I
> > didn't look at the reason why a seperate buf is used for virtio_net_hdr
> > in QEMU.
>
> It's a qemu bug. It insists the header be an element in the scatterlist by
> itself. Unfortunately we have to accommodate it.
We do? Let's just fix this?
All we have to do is replace memcpy with proper iovec walk, correct?
Something like the followng (untested) patch? It's probably not too
late to put this in the next qemu release...
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 2f147e5..06c5148 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -434,26 +434,59 @@ static int iov_fill(struct iovec *iov, int iovcnt, const void *buf, int count)
return offset;
}
+static int iov_skip(struct iovec *iov, int iovcnt, int count)
+{
+ int offset, i;
+
+ offset = i = 0;
+ while (offset < count && i < iovcnt) {
+ int len = MIN(iov[i].iov_len, count - offset);
+ iov[i].iov_base += len;
+ iov[i].iov_len -= len;
+ offset += len;
+ i++;
+ }
+
+ return offset;
+}
+
+static int iov_copy(struct iovec *to, struct iovec *from, int iovcnt, int count)
+{
+ int offset, i;
+
+ offset = i = 0;
+ while (offset < count && i < iovcnt) {
+ int len = MIN(from[i].iov_len, count - offset);
+ to[i].iov_base = from[i].iov_base;
+ to[i].iov_len = from[i].iov_len;
+ offset += len;
+ i++;
+ }
+
+ return i;
+}
+
static int receive_header(VirtIONet *n, struct iovec *iov, int iovcnt,
const void *buf, size_t size, size_t hdr_len)
{
- struct virtio_net_hdr *hdr = (struct virtio_net_hdr *)iov[0].iov_base;
+ struct virtio_net_hdr hdr = {};
int offset = 0;
- hdr->flags = 0;
- hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
+ hdr.flags = 0;
+ hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE;
if (n->has_vnet_hdr) {
- memcpy(hdr, buf, sizeof(*hdr));
- offset = sizeof(*hdr);
- work_around_broken_dhclient(hdr, buf + offset, size - offset);
+ memcpy(&hdr, buf, sizeof hdr);
+ offset = sizeof hdr;
+ work_around_broken_dhclient(&hdr, buf + offset, size - offset);
}
+ iov_fill(iov, iovcnt, &hdr, sizeof hdr);
+
/* We only ever receive a struct virtio_net_hdr from the tapfd,
* but we may be passing along a larger header to the guest.
*/
- iov[0].iov_base += hdr_len;
- iov[0].iov_len -= hdr_len;
+ iov_skip(iov, iovcnt, hdr_len);
return offset;
}
@@ -514,7 +547,8 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
static ssize_t virtio_net_receive(VLANClientState *vc, const uint8_t *buf, size_t size)
{
VirtIONet *n = vc->opaque;
- struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
+ struct iovec mhdr[VIRTQUEUE_MAX_SIZE];
+ int mhdrcnt = 0;
size_t hdr_len, offset, i;
if (!virtio_net_can_receive(n->vc))
@@ -552,16 +586,13 @@ static ssize_t virtio_net_receive(VLANClientState *vc, const uint8_t *buf, size_
exit(1);
}
- if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != hdr_len) {
- fprintf(stderr, "virtio-net header not in first element\n");
- exit(1);
- }
-
memcpy(&sg, &elem.in_sg[0], sizeof(sg[0]) * elem.in_num);
if (i == 0) {
- if (n->mergeable_rx_bufs)
- mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base;
+ if (n->mergeable_rx_bufs) {
+ mhdrcnt = iov_copy(mhdr, sg, elem.in_num,
+ sizeof(struct virtio_net_hdr_mrg_rxbuf));
+ }
offset += receive_header(n, sg, elem.in_num,
buf + offset, size - offset, hdr_len);
@@ -579,8 +610,12 @@ static ssize_t virtio_net_receive(VLANClientState *vc, const uint8_t *buf, size_
offset += len;
}
- if (mhdr)
- mhdr->num_buffers = i;
+ if (mhdrcnt) {
+ uint16_t num = i;
+ iov_skip(mhdr, mhdrcnt,
+ offsetof(struct virtio_net_hdr_mrg_rxbuf, num_buffers));
+ iov_fill(mhdr, mhdrcnt, &num, sizeof num);
+ }
virtqueue_flush(n->rx_vq, i);
virtio_notify(&n->vdev, n->rx_vq);
@@ -627,20 +662,19 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
sizeof(struct virtio_net_hdr_mrg_rxbuf) :
sizeof(struct virtio_net_hdr);
- if (out_num < 1 || out_sg->iov_len != hdr_len) {
- fprintf(stderr, "virtio-net header not in first element\n");
+ if (out_num < 1) {
+ fprintf(stderr, "virtio-net: no output element\n");
exit(1);
}
/* ignore the header if GSO is not supported */
if (!n->has_vnet_hdr) {
- out_num--;
- out_sg++;
+ iov_skip(out_sg, out_num, hdr_len);
len += hdr_len;
} else if (n->mergeable_rx_bufs) {
/* tapfd expects a struct virtio_net_hdr */
hdr_len -= sizeof(struct virtio_net_hdr);
- out_sg->iov_len -= hdr_len;
+ iov_skip(out_sg, out_num, hdr_len);
len += hdr_len;
}
^ permalink raw reply related
* Re: [PATCH 3/4] macvlan: implement bridge, VEPA and private mode
From: Arnd Bergmann @ 2009-11-24 12:45 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: <4B0BB89F.7030605@trash.net>
On Tuesday 24 November 2009, Patrick McHardy wrote:
> Arnd Bergmann wrote:
> > @@ -29,9 +29,16 @@
> > #include <linux/if_link.h>
> > #include <linux/if_macvlan.h>
> > #include <net/rtnetlink.h>
> > +#include <net/xfrm.h>
>
> Do we really need this?
Not any more. I originally did secpath_reset here, but that is now moved
to dev_forward_skb() in net/core/dev.c so I'll remove the include here.
> > @@ -129,11 +137,14 @@ static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
> > }
> >
> > static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
> > - const struct ethhdr *eth)
> > + const struct ethhdr *eth, int local)
>
> bool local?
Already changed as a reply to Eric's comments
> > @@ -159,8 +172,12 @@ static void macvlan_broadcast(struct sk_buff *skb,
> >
> > for (i = 0; i < MACVLAN_HASH_SIZE; i++) {
> > hlist_for_each_entry_rcu(vlan, n, &port->vlan_hash[i], hlist) {
> > + if ((vlan->dev == src) || !(vlan->mode & mode))
>
> Please remove those unnecessary parentheses around the
> device comparison.
Ok. I usually prefer to have some extra parentheses in places that not
everyone finds obvious but these are pretty clear.
>
> The '|' should go on the first line.
>
> Multi-line comments should begin every line with '*'.
>
> Please align the mode values (in all cases above) to the arguments
> on the line above.
ok
> > +static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > + const struct macvlan_dev *vlan = netdev_priv(dev);
> > + const struct macvlan_port *port = vlan->port;
> > + const struct macvlan_dev *dest;
> > +
> > + if (vlan->mode == MACVLAN_MODE_BRIDGE) {
> > + const struct ethhdr *eth = (void *)skb->data;
>
> eth_hdr()?
This is done before calling eth_type_trans and skb->mac_header
appears to be uninitialized then, which gave me an instant kernel
panic here.
> > + dest = macvlan_hash_lookup(port, eth->h_dest);
> > + if (dest && dest->mode == MACVLAN_MODE_BRIDGE) {
> > + int length = skb->len + ETH_HLEN;
>
> unsigned int for length values please.
ok
Thanks for the detailed review,
Arnd <><
^ permalink raw reply
* Re: [PATCH 4/4] macvlan: export macvlan mode through netlink
From: Arnd Bergmann @ 2009-11-24 12:57 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: <4B0BBB2E.8020502@trash.net>
On Tuesday 24 November 2009, Patrick McHardy wrote:
> Arnd Bergmann wrote:
> > @@ -600,6 +594,18 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
> > if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
> > return -EADDRNOTAVAIL;
> > }
> > +
> > + if (data && data[IFLA_MACVLAN_MODE]) {
> > + u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> > + switch (mode) {
> > + case MACVLAN_MODE_PRIVATE:
> > + case MACVLAN_MODE_VEPA:
> > + case MACVLAN_MODE_BRIDGE:
> > + break;
> > + default:
> > + return -EINVAL;
>
> EINVAL is quite unspecific. In this case I think EOPNOTSUPP would
> be fine and provide more information.
ok
> > + }
> > + }
> > return 0;
> > }
> > @@ -664,6 +670,13 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,
> > vlan->dev = dev;
> > vlan->port = port;
> >
> > + vlan->mode = MACVLAN_MODE_VEPA;
> > + if (data && data[IFLA_MACVLAN_MODE]) {
> > + u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> > +
> > + vlan->mode = mode;
>
> This looks a bit strange, like cut-and-paste without reformatting :)
> I'd suggest to simply use "vlan->mode = nla_get_u32(...)".
Yep.
Thanks for the review. Combined changes below.
Arnd <><
---
drivers/net/macvlan.c | 47 +++++++++++++++++++++++------------------------
1 files changed, 23 insertions(+), 24 deletions(-)
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -118,15 +118,16 @@ 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,
- bool success, bool multicast)
+static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
+ unsigned int len, bool success,
+ bool multicast)
{
struct macvlan_rx_stats *rx_stats;
rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
if (likely(success)) {
rx_stats->rx_packets++;;
- rx_stats->rx_bytes += length;
+ rx_stats->rx_bytes += len;
if (multicast)
rx_stats->multicast++;
} else {
@@ -170,7 +171,7 @@ static void macvlan_broadcast(struct sk_buff *skb,
for (i = 0; i < MACVLAN_HASH_SIZE; i++) {
hlist_for_each_entry_rcu(vlan, n, &port->vlan_hash[i], hlist) {
- if ((vlan->dev == src) || !(vlan->mode & mode))
+ if (vlan->dev == src || !(vlan->mode & mode))
continue;
nskb = skb_clone(skb, GFP_ATOMIC);
@@ -190,7 +191,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;
+ unsigned int len;
port = rcu_dereference(skb->dev->macvlan_port);
if (port == NULL)
@@ -200,17 +201,22 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
src = macvlan_hash_lookup(port, eth->h_source);
if (!src)
/* frame comes from an external address */
- macvlan_broadcast(skb, port, NULL, MACVLAN_MODE_PRIVATE
- | MACVLAN_MODE_VEPA | MACVLAN_MODE_BRIDGE);
+ macvlan_broadcast(skb, port, NULL,
+ MACVLAN_MODE_PRIVATE |
+ MACVLAN_MODE_VEPA |
+ MACVLAN_MODE_BRIDGE);
else if (src->mode == MACVLAN_MODE_VEPA)
/* flood to everyone except source */
macvlan_broadcast(skb, port, src->dev,
- MACVLAN_MODE_VEPA | MACVLAN_MODE_BRIDGE);
+ MACVLAN_MODE_VEPA |
+ MACVLAN_MODE_BRIDGE);
else if (src->mode == MACVLAN_MODE_BRIDGE)
- /* flood only to VEPA ports, bridge ports
- already saw the frame */
+ /*
+ * flood only to VEPA ports, bridge ports
+ * already saw the frame on the way out.
+ */
macvlan_broadcast(skb, port, src->dev,
- MACVLAN_MODE_VEPA);
+ MACVLAN_MODE_VEPA);
return skb;
}
@@ -253,7 +259,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
dest = macvlan_hash_lookup(port, eth->h_dest);
if (dest && dest->mode == MACVLAN_MODE_BRIDGE) {
- int length = skb->len + ETH_HLEN;
+ unsigned int length = skb->len + ETH_HLEN;
int ret = dev_forward_skb(dest->dev, skb);
macvlan_count_rx(dest, length,
ret == NET_RX_SUCCESS, 0);
@@ -604,8 +610,7 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
}
if (data && data[IFLA_MACVLAN_MODE]) {
- u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
- switch (mode) {
+ switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
case MACVLAN_MODE_PRIVATE:
case MACVLAN_MODE_VEPA:
case MACVLAN_MODE_BRIDGE:
@@ -679,11 +684,8 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,
vlan->port = port;
vlan->mode = MACVLAN_MODE_VEPA;
- if (data && data[IFLA_MACVLAN_MODE]) {
- u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
-
- vlan->mode = mode;
- }
+ if (data && data[IFLA_MACVLAN_MODE])
+ vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
err = register_netdevice(dev);
if (err < 0)
@@ -710,11 +712,8 @@ static int macvlan_changelink(struct net_device *dev,
struct nlattr *tb[], struct nlattr *data[])
{
struct macvlan_dev *vlan = netdev_priv(dev);
- if (data && data[IFLA_MACVLAN_MODE]) {
- u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
- vlan->mode = mode;
- }
-
+ if (data && data[IFLA_MACVLAN_MODE])
+ vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
return 0;
}
^ permalink raw reply
* Re: [PATCH 1/4] veth: move loopback logic to common location
From: Arnd Bergmann @ 2009-11-24 13:13 UTC (permalink / raw)
To: Patrick McHardy
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: <4B0BB818.6090509@trash.net>
On Tuesday 24 November 2009, Patrick McHardy wrote:
> 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.
Ok, I won't bother with it for now then.
Arnd <><
^ permalink raw reply
* Re: [PATCH net-next-2.6] ixgbe: Fix TX stats accounting
From: Eric Dumazet @ 2009-11-24 13:23 UTC (permalink / raw)
To: Peter P Waskiewicz Jr, Jeff Kirsher
Cc: robert@herjulf.net, Jesper Dangaard Brouer, Linux Netdev List,
David S. Miller
In-Reply-To: <4B0BC56D.1000909@gmail.com>
Here is an updated version, because ixgbe_get_ethtool_stats()
needs to call dev_get_stats() or "ethtool -S" wont give
correct tx_bytes/tx_packets values.
[PATCH net-next-2.6] ixgbe: Fix TX stats accounting
Several cpus can update netdev->stats.tx_bytes & netdev->stats.tx_packets
in parallel. In this case, TX stats are under estimated and false sharing
takes place.
After a pktgen session sending exactly 200000000 packets :
# ifconfig fiber0 | grep TX
TX packets:198501982 errors:0 dropped:0 overruns:0 carrier:0
Multi queue devices should instead use txq->tx_bytes & txq->tx_packets
in their xmit() method (appropriate txq lock already held by caller, no
cache line miss), or use appropriate locking.
After patch, same pktgen session gives :
# ifconfig fiber0 | grep TX
TX packets:200000000 errors:0 dropped:0 overruns:0 carrier:0
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
drivers/net/ixgbe/ixgbe_ethtool.c | 1 +
drivers/net/ixgbe/ixgbe_main.c | 20 ++++----------------
2 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethtool.c b/drivers/net/ixgbe/ixgbe_ethtool.c
index 74f04e1..7b7f8f6 100644
--- a/drivers/net/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ixgbe/ixgbe_ethtool.c
@@ -944,6 +944,7 @@ static void ixgbe_get_ethtool_stats(struct net_device *netdev,
char *p = NULL;
ixgbe_update_stats(adapter);
+ dev_get_stats(netdev);
for (i = 0; i < IXGBE_GLOBAL_STATS_LEN; i++) {
switch (ixgbe_gstrings_stats[i].type) {
case NETDEV_STATS:
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index ebcec30..1cea120 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -425,8 +425,6 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
tx_ring->total_packets += total_packets;
tx_ring->stats.packets += total_packets;
tx_ring->stats.bytes += total_bytes;
- netdev->stats.tx_bytes += total_bytes;
- netdev->stats.tx_packets += total_packets;
return (count < tx_ring->work_limit);
}
@@ -5249,6 +5247,7 @@ static netdev_tx_t ixgbe_xmit_frame(struct sk_buff *skb,
{
struct ixgbe_adapter *adapter = netdev_priv(netdev);
struct ixgbe_ring *tx_ring;
+ struct netdev_queue *txq;
unsigned int first;
unsigned int tx_flags = 0;
u8 hdr_len = 0;
@@ -5345,6 +5344,9 @@ static netdev_tx_t ixgbe_xmit_frame(struct sk_buff *skb,
tx_ring->atr_count = 0;
}
}
+ txq = netdev_get_tx_queue(netdev, r_idx);
+ txq->tx_bytes += skb->len;
+ txq->tx_packets++;
ixgbe_tx_queue(adapter, tx_ring, tx_flags, count, skb->len,
hdr_len);
ixgbe_maybe_stop_tx(netdev, tx_ring, DESC_NEEDED);
@@ -5359,19 +5361,6 @@ static netdev_tx_t ixgbe_xmit_frame(struct sk_buff *skb,
}
/**
- * ixgbe_get_stats - Get System Network Statistics
- * @netdev: network interface device structure
- *
- * Returns the address of the device statistics structure.
- * The statistics are actually updated from the timer callback.
- **/
-static struct net_device_stats *ixgbe_get_stats(struct net_device *netdev)
-{
- /* only return the current stats */
- return &netdev->stats;
-}
-
-/**
* ixgbe_set_mac - Change the Ethernet Address of the NIC
* @netdev: network interface device structure
* @p: pointer to an address structure
@@ -5501,7 +5490,6 @@ static const struct net_device_ops ixgbe_netdev_ops = {
.ndo_stop = ixgbe_close,
.ndo_start_xmit = ixgbe_xmit_frame,
.ndo_select_queue = ixgbe_select_queue,
- .ndo_get_stats = ixgbe_get_stats,
.ndo_set_rx_mode = ixgbe_set_rx_mode,
.ndo_set_multicast_list = ixgbe_set_rx_mode,
.ndo_validate_addr = eth_validate_addr,
^ permalink raw reply related
* Re: large packet loss take2 2.6.31.x
From: Jarek Poplawski @ 2009-11-24 13:46 UTC (permalink / raw)
To: Caleb Cushing
Cc: Frans Pop, Andi Kleen, linux-kernel, netdev, Jeff Kirsher,
Jesse Brandeburg, e1000-devel
In-Reply-To: <20091124111946.GA7883@ff.dom.local>
On Tue, Nov 24, 2009 at 11:19:46AM +0000, Jarek Poplawski wrote:
...
> Alas it's not all information I asked. E.g. "netstat -s before faulty
> kernel" and "netstat -s after faulty kernel" seem to be the same file:
> netstat_after.slave4.log.gz.
On the other hand, there is a lot of tcp retransmits there:
Tcp:
17 active connections openings
0 passive connection openings
14 failed connection attempts
0 connection resets received
0 connections established
45 segments received
49 segments send out
19 segments retransmited
0 bad segments received.
19 resets sent
So it might point at the driver yet. It would be interesting to see
more of this: could you repeat "netstat -s" and "ethtool -S eth0"
after rebooting with both kernels and doing a few minutes of similar
tcp activities (against the router or some other "good" site). Btw,
please remind us the exact kernel versions. If you can, try 2.6.32-rc8
instead of 2.6.31.
Jarek P.
^ permalink raw reply
* Re: [PATCH 4/4] macvlan: export macvlan mode through netlink
From: Patrick McHardy @ 2009-11-24 13:47 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: <200911241357.46690.arnd@arndb.de>
Arnd Bergmann wrote:
> Thanks for the review. Combined changes below.
Looks good, thanks.
>
> Arnd <><
> ---
>
> drivers/net/macvlan.c | 47 +++++++++++++++++++++++------------------------
> 1 files changed, 23 insertions(+), 24 deletions(-)
>
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -118,15 +118,16 @@ 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,
> - bool success, bool multicast)
> +static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
> + unsigned int len, bool success,
> + bool multicast)
> {
> struct macvlan_rx_stats *rx_stats;
>
> rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
> if (likely(success)) {
> rx_stats->rx_packets++;;
> - rx_stats->rx_bytes += length;
> + rx_stats->rx_bytes += len;
> if (multicast)
> rx_stats->multicast++;
> } else {
> @@ -170,7 +171,7 @@ static void macvlan_broadcast(struct sk_buff *skb,
>
> for (i = 0; i < MACVLAN_HASH_SIZE; i++) {
> hlist_for_each_entry_rcu(vlan, n, &port->vlan_hash[i], hlist) {
> - if ((vlan->dev == src) || !(vlan->mode & mode))
> + if (vlan->dev == src || !(vlan->mode & mode))
> continue;
>
> nskb = skb_clone(skb, GFP_ATOMIC);
> @@ -190,7 +191,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;
> + unsigned int len;
>
> port = rcu_dereference(skb->dev->macvlan_port);
> if (port == NULL)
> @@ -200,17 +201,22 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
> src = macvlan_hash_lookup(port, eth->h_source);
> if (!src)
> /* frame comes from an external address */
> - macvlan_broadcast(skb, port, NULL, MACVLAN_MODE_PRIVATE
> - | MACVLAN_MODE_VEPA | MACVLAN_MODE_BRIDGE);
> + macvlan_broadcast(skb, port, NULL,
> + MACVLAN_MODE_PRIVATE |
> + MACVLAN_MODE_VEPA |
> + MACVLAN_MODE_BRIDGE);
> else if (src->mode == MACVLAN_MODE_VEPA)
> /* flood to everyone except source */
> macvlan_broadcast(skb, port, src->dev,
> - MACVLAN_MODE_VEPA | MACVLAN_MODE_BRIDGE);
> + MACVLAN_MODE_VEPA |
> + MACVLAN_MODE_BRIDGE);
> else if (src->mode == MACVLAN_MODE_BRIDGE)
> - /* flood only to VEPA ports, bridge ports
> - already saw the frame */
> + /*
> + * flood only to VEPA ports, bridge ports
> + * already saw the frame on the way out.
> + */
> macvlan_broadcast(skb, port, src->dev,
> - MACVLAN_MODE_VEPA);
> + MACVLAN_MODE_VEPA);
> return skb;
> }
>
> @@ -253,7 +259,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
>
> dest = macvlan_hash_lookup(port, eth->h_dest);
> if (dest && dest->mode == MACVLAN_MODE_BRIDGE) {
> - int length = skb->len + ETH_HLEN;
> + unsigned int length = skb->len + ETH_HLEN;
> int ret = dev_forward_skb(dest->dev, skb);
> macvlan_count_rx(dest, length,
> ret == NET_RX_SUCCESS, 0);
> @@ -604,8 +610,7 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
> }
>
> if (data && data[IFLA_MACVLAN_MODE]) {
> - u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> - switch (mode) {
> + switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
> case MACVLAN_MODE_PRIVATE:
> case MACVLAN_MODE_VEPA:
> case MACVLAN_MODE_BRIDGE:
> @@ -679,11 +684,8 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,
> vlan->port = port;
>
> vlan->mode = MACVLAN_MODE_VEPA;
> - if (data && data[IFLA_MACVLAN_MODE]) {
> - u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> -
> - vlan->mode = mode;
> - }
> + if (data && data[IFLA_MACVLAN_MODE])
> + vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
>
> err = register_netdevice(dev);
> if (err < 0)
> @@ -710,11 +712,8 @@ static int macvlan_changelink(struct net_device *dev,
> struct nlattr *tb[], struct nlattr *data[])
> {
> struct macvlan_dev *vlan = netdev_priv(dev);
> - if (data && data[IFLA_MACVLAN_MODE]) {
> - u32 mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> - vlan->mode = mode;
> - }
> -
> + if (data && data[IFLA_MACVLAN_MODE])
> + vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> return 0;
> }
>
>
^ permalink raw reply
* RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters
From: Ben Hutchings @ 2009-11-24 14:18 UTC (permalink / raw)
To: Williams, Mitch A
Cc: Kirsher, Jeffrey T, davem@davemloft.net, shemminger@vyatta.com,
netdev@vger.kernel.org, gospo@redhat.com
In-Reply-To: <EA929A9653AAE14F841771FB1DE5A1365FA6A9617F@rrsmsx501.amr.corp.intel.com>
On Mon, 2009-11-23 at 12:52 -0700, Williams, Mitch A wrote:
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Monday, November 23, 2009 5:23 AM
> [snip]
>
> >
> >How does this interact with use of multiple queues within a single
> >function? Are the specified queue numbers really interpreted as RX
> >queue indices or as function numbers?
> >
> >Ben.
>
> Yeah, that is ambiguous. Would it be better if we changed the name of the parameter to 'vf' instead of 'queue' to make it explicit?
>
> This would give us:
> $ ip link set eth1 vf 1 mac <blah>
>
> The issue of which VF goes with which PF device can be deduced in userspace via sysfs.
>
> If we want to make this apply to non SR-IOV queues, then we'll add a new parameter later.
>
> Works for you?
OK, this sounds reasonable.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 1/1] Defer skb allocation for both mergeable buffers and big packets in virtio_net
From: Anthony Liguori @ 2009-11-24 14:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, Shirley Ma, Eric Dumazet, Avi Kivity, netdev, kvm,
linux-kernel, Hollis Blanchard
In-Reply-To: <20091124113754.GB2405@redhat.com>
Michael S. Tsirkin wrote:
> On Tue, Nov 24, 2009 at 08:54:23AM +1030, Rusty Russell wrote:
>
>> On Tue, 24 Nov 2009 02:37:01 am Shirley Ma wrote:
>>
>>>>> + skb = (struct sk_buff *)buf;
>>>>>
>>>> This cast is unnecessary, but a comment would be nice:
>>>>
>>> Without this cast there is a compile warning.
>>>
>> Hi Shirley,
>>
>> Looks like buf is a void *, so no cast should be necessary. But I could
>> be reading the patch wrong.
>>
>>
>>>> However, I question whether making it 16 byte is the right thing: the
>>>> ethernet header is 14 bytes long, so don't we want 8 bytes of padding?
>>>>
>>> Because in QEMU it requires 10 bytes header in a separately, so one page
>>> is used to share between virtio_net_hdr header which is 10 bytes head
>>> and rest of data. So I put 6 bytes offset here between two buffers. I
>>> didn't look at the reason why a seperate buf is used for virtio_net_hdr
>>> in QEMU.
>>>
>> It's a qemu bug. It insists the header be an element in the scatterlist by
>> itself. Unfortunately we have to accommodate it.
>>
>
> We do? Let's just fix this?
>
So does lguest. It's been that way since the beginning. Fixing this
would result in breaking older guests.
We really need to introduce a feature bit if we want to change this.
Regards,
Anthony Liguori
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Arjan van de Ven @ 2009-11-24 14:42 UTC (permalink / raw)
To: Peter Zijlstra
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: <1259051986.4531.1057.camel@laptop>
Peter Zijlstra wrote:
>>> 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?
>
and that is exported to userspace.. how?
... that has nothing to do with an irq, and also falls flat for a driver that
supports multiple irqs, and assigns one to each numa node.
^ permalink raw reply
* Re: [PATCH] irq: Add node_affinity CPU masks for smarter irqbalance hints
From: Arjan van de Ven @ 2009-11-24 14:43 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Peter P Waskiewicz Jr, 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>
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.
the kernel does no such thing, nor has code to do so.
> Are you talking about some daft userspace thing that
> writes into the irq smp_affinity to effect irq balancing?
thanks ;)
^ permalink raw reply
* Re: [patch]USB autosuspend for cdc-ether
From: Steve.Glendinning @ 2009-11-24 15:11 UTC (permalink / raw)
To: Oliver Neukum
Cc: David S. Miller, David Brownell, linux-usb, netdev, stern,
Torgny Johansson
In-Reply-To: <200911231621.04499.oliver@neukum.org>
Hi Oliver,
Interesting patch, I'm going to try and add support for this
to smsc95xx.
A few things I noticed:
> @@ -929,7 +961,11 @@ kevent (struct work_struct *work)
> int retval = 0;
>
> clear_bit (EVENT_LINK_RESET, &dev->flags);
> + status = usb_autopm_get_interface(dev->intf);
> + if (status < 0)
> + goto skip_reset;
> if(info->link_reset && (retval = info->link_reset(dev)) < 0) {
> +skip_reset:
> devinfo(dev, "link reset failed (%d) usbnet usb-%s-%s, %s",
> retval,
> dev->udev->bus->bus_name, dev->udev->devpath,
on EVENT_LINK_RESET you call usb_autopm_get_interface with no matching
usb_autopm_put_interface, so you leak 1 reference count and the device
never autosuspends.
> @@ -958,7 +994,7 @@ static void tx_complete (struct urb *urb)
>
> switch (urb->status) {
> case -EPIPE:
> - usbnet_defer_kevent (dev, EVENT_TX_HALT);
> + usbnet_defer_kevent (dev, EVENT_TX_HALT);
> break;
>
> /* software-driven interface shutdown */
The only change here is the addition of trailing whitespace (which both
checkpatch.pl and git complain about). You should run your patch through
scripts/checkpatch.pl and fix these up.
--
Steve
^ permalink raw reply
* RE: [E1000-devel] large packet loss take2 2.6.31.x
From: Allan, Bruce W @ 2009-11-24 15:57 UTC (permalink / raw)
To: Jarek Poplawski, Caleb Cushing
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Frans Pop, Brandeburg, Jesse, linux-kernel@vger.kernel.org,
Andi Kleen, Kirsher, Jeffrey T
In-Reply-To: <20091124111946.GA7883@ff.dom.local>
>-----Original Message-----
>From: Jarek Poplawski [mailto:jarkao2@gmail.com]
>Sent: Tuesday, November 24, 2009 3:20 AM
>To: Caleb Cushing
>Cc: e1000-devel@lists.sourceforge.net; netdev@vger.kernel.org; Frans Pop;
>Brandeburg, Jesse; linux-kernel@vger.kernel.org; Andi Kleen; Kirsher,
>Jeffrey T
>Subject: Re: [E1000-devel] large packet loss take2 2.6.31.x
>
>On Tue, Nov 24, 2009 at 01:17:09AM -0500, Caleb Cushing wrote:
>> > Btw, currently I don't consider this dropping means there has to be
>> > a bug. It could be otherwise - a feature... e.g. when a new kernel
>> > can transmit faster (then dropping in some other, slower place can
>> > happen).
>>
>> um... where would it be dropping that we wouldn't have a bug? I mean
>> sure faster is great... but if it makes my network not work right...
>
>E.g. if it were dropped because of a queue overflow (but it doesn't
>seem to be the case, at least at your box) or because of memory
>problems while handling a lot of traffic.
>
>>
>> I've added all (I think) information you've asked for to the bug
>> http://bugzilla.kernel.org/show_bug.cgi?id=13835 except for ethtool
>> and netstat on the router side. ethtool complains about not having
>> driver or capability (maybe because it's a 2.4 kernel?) and the
>> version of netstat doesn't support -s. I disabled everything that I
>> can think of that would send/receive packets before doing the test
>> client side, except dhcp/dns windows box's were probably sending some
>> broadcasts too. but the traffic should be pretty low. I did remember
>> to set the txqueuelen didn't seem to make a difference
>
>Alas it's not all information I asked. E.g. "netstat -s before faulty
>kernel" and "netstat -s after faulty kernel" seem to be the same file:
>netstat_after.slave4.log.gz. Anyway, since there are problems with
>getting stats from the router we still can't compare them, or check
>for the dropped stats. (Btw, could you check for /proc/net/softnet_stat
>yet?)
>
>So, it might be the kernel problem you reported, but there is not
>enough data to prove it. Then my proposal is to try to repeat this
>problem in more "testing friendly" conditions - preferably against
>some other, more up-to-date linux box, if possible?
>
>> only error in dmesg I see is
>>
>> e1000e 0000:00:19.0: pci_enable_pcie_error_reporting failed 0xfffffffb
>
>I added e1000e maintainers to CC to have a look at this warning.
>
>Jarek P.
The "pci_enable_pcie_error_reporting failed" message is a non-fatal warning that has recently been removed.
^ permalink raw reply
* Re: Diagnostic Monitoring Interface Monitoring (DOM) PATCH 4/5 for net-next-2.6
From: robert @ 2009-11-24 18:12 UTC (permalink / raw)
To: Joe Perches; +Cc: Robert Olsson, David Miller, netdev
In-Reply-To: <1259017659.16503.138.camel@Joe-Laptop.home>
Joe Perches writes:
> Can you run ./scripts/checkpatch.pl on your patches please?
Yes there were many objections... but my linux quantum is for over for now
> > + if((res = read_phy_diag(hw, 0x1, DOM_A2_TEMP_SLOPE, &pd->temp_slope)))
> > + goto out;
> [etc...]
> > + if((res = read_phy_diag(hw, 0x1, DOM_A2_TX_I_OFFSET, &pd->tx_bias_offset)))
> > + goto out;
>
> perhaps this is clearer as:
>
> if (read_phy_diag(hw, 0x1, reg, loc) ||
> read_phy_diag(hw, 0x1, reg2, loc2) ||
> ...
> read_phy_diag(hw, 0x1, regN, locN))
> goto out;
Feel free to attack it...
Cheers
--ro
^ permalink raw reply
* Re: [PATCH 1/1] Defer skb allocation for both mergeable buffers and big packets in virtio_net
From: Michael S. Tsirkin @ 2009-11-24 16:04 UTC (permalink / raw)
To: Anthony Liguori
Cc: Rusty Russell, Shirley Ma, Eric Dumazet, Avi Kivity, netdev, kvm,
linux-kernel, Hollis Blanchard
In-Reply-To: <4B0BEF70.5070104@codemonkey.ws>
On Tue, Nov 24, 2009 at 08:36:32AM -0600, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> On Tue, Nov 24, 2009 at 08:54:23AM +1030, Rusty Russell wrote:
>>
>>> On Tue, 24 Nov 2009 02:37:01 am Shirley Ma wrote:
>>>
>>>>>> + skb = (struct sk_buff *)buf;
>>>>>>
>>>>> This cast is unnecessary, but a comment would be nice:
>>>>>
>>>> Without this cast there is a compile warning.
>>> Hi Shirley,
>>>
>>> Looks like buf is a void *, so no cast should be necessary. But I could
>>> be reading the patch wrong.
>>>
>>>
>>>>> However, I question whether making it 16 byte is the right thing: the
>>>>> ethernet header is 14 bytes long, so don't we want 8 bytes of padding?
>>>>>
>>>> Because in QEMU it requires 10 bytes header in a separately, so one page
>>>> is used to share between virtio_net_hdr header which is 10 bytes head
>>>> and rest of data. So I put 6 bytes offset here between two buffers. I
>>>> didn't look at the reason why a seperate buf is used for virtio_net_hdr
>>>> in QEMU.
>>>>
>>> It's a qemu bug. It insists the header be an element in the scatterlist by
>>> itself. Unfortunately we have to accommodate it.
>>>
>>
>> We do? Let's just fix this?
>>
>
> So does lguest. It's been that way since the beginning. Fixing this
> would result in breaking older guests.
The patch you are replying to fixes this in a way that does not break older guests.
> We really need to introduce a feature bit if we want to change this.
--
MST
^ 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