* [PATCH 8/8] Phonet: kill the ST-Ericsson pipe controller Kconfig
From: Rémi Denis-Courmont @ 2011-03-09 8:44 UTC (permalink / raw)
To: netdev
In-Reply-To: <201103091043.32391.remi.denis-courmont@nokia.com>
This is now a run-time choice so that a single kernel can support both
old and new generation ISI modems. Support for manually enabling the
pipe flow is removed as it did not work properly, does not fit well
with the socket API, and I am not aware of any use at the moment.
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
Documentation/networking/phonet.txt | 13 -------------
include/linux/phonet.h | 2 --
net/phonet/Kconfig | 12 ------------
net/phonet/pep.c | 25 -------------------------
4 files changed, 0 insertions(+), 52 deletions(-)
diff --git a/Documentation/networking/phonet.txt b/Documentation/networking/phonet.txt
index 3d12779..8100358 100644
--- a/Documentation/networking/phonet.txt
+++ b/Documentation/networking/phonet.txt
@@ -205,19 +205,6 @@ The pipe protocol provides two socket options at the SOL_PNPIPE level:
socket descriptors that are already connected or being connected.
-Phonet Pipe-controller Implementation
--------------------------------------
-
-Phonet Pipe-controller is enabled by selecting the CONFIG_PHONET_PIPECTRLR
-Kconfig option.
-
-The implementation adds socket options at SOL_PNPIPE level:
-
- PNPIPE_ENABLE accepts one integer value (int). If set to zero, the pipe
- is disabled. If the value is non-zero, the pipe is enabled. If the pipe
- is not (yet) connected, ENOTCONN is error is returned.
-
-
Authors
-------
diff --git a/include/linux/phonet.h b/include/linux/phonet.h
index 32a0965..6fb1384 100644
--- a/include/linux/phonet.h
+++ b/include/linux/phonet.h
@@ -37,8 +37,6 @@
#define PNPIPE_ENCAP 1
#define PNPIPE_IFINDEX 2
#define PNPIPE_HANDLE 3
-#define PNPIPE_ENABLE 4
-/* unused slot */
#define PNADDR_ANY 0
#define PNADDR_BROADCAST 0xFC
diff --git a/net/phonet/Kconfig b/net/phonet/Kconfig
index 0d9b8a2..6ec7d55 100644
--- a/net/phonet/Kconfig
+++ b/net/phonet/Kconfig
@@ -14,15 +14,3 @@ config PHONET
To compile this driver as a module, choose M here: the module
will be called phonet. If unsure, say N.
-
-config PHONET_PIPECTRLR
- bool "Phonet Pipe Controller (EXPERIMENTAL)"
- depends on PHONET && EXPERIMENTAL
- default N
- help
- The Pipe Controller implementation in Phonet stack to support Pipe
- data with Nokia Slim modems like WG2.5 used on ST-Ericsson U8500
- platform.
-
- This option is incompatible with older Nokia modems.
- Say N here unless you really know what you are doing.
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index 671effb..68e635f 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -167,15 +167,6 @@ static int pipe_handler_send_created_ind(struct sock *sk)
data, 4, GFP_ATOMIC);
}
-#ifdef CONFIG_PHONET_PIPECTRLR
-static int pipe_handler_enable_pipe(struct sock *sk, int enable)
-{
- u8 id = enable ? PNS_PEP_ENABLE_REQ : PNS_PEP_DISABLE_REQ;
-
- return pipe_handler_request(sk, id, PAD, NULL, 0);
-}
-#endif
-
static int pep_accept_conn(struct sock *sk, struct sk_buff *skb)
{
static const u8 data[20] = {
@@ -968,16 +959,6 @@ static int pep_setsockopt(struct sock *sk, int level, int optname,
}
goto out_norel;
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNPIPE_ENABLE:
- if ((1 << sk->sk_state) & ~(TCPF_SYN_RECV|TCPF_ESTABLISHED)) {
- err = -ENOTCONN;
- break;
- }
- err = pipe_handler_enable_pipe(sk, val);
- break;
-#endif
-
default:
err = -ENOPROTOOPT;
}
@@ -1013,12 +994,6 @@ static int pep_getsockopt(struct sock *sk, int level, int optname,
return -EINVAL;
break;
-#ifdef CONFIG_PHONET_PIPECTRLR
- case PNPIPE_ENABLE:
- val = sk->sk_state == TCP_ESTABLISHED;
- break;
-#endif
-
default:
return -ENOPROTOOPT;
}
--
1.7.1
^ permalink raw reply related
* Re: Bug inkvm_set_irq
From: Jean-Philippe Menil @ 2011-03-09 12:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20110308111320.GA8002@redhat.com>
Le 08/03/2011 12:13, Michael S. Tsirkin a écrit :
> On Fri, Mar 04, 2011 at 10:39:05AM +0100, Jean-Philippe Menil wrote:
>> Yes, it's a 2.6.37.2 kernel.
> OK, here's a debugging patch.
> Please run with slab debugging as previously until you see
> 'eventfd bug detected!' in dmesg or until there is a crash.
> It might be also useful to enable timestampts on printk with
> Symbol: PRINTK_TIME [=y]
> │ Type : boolean
> │ Prompt: Show timing information on printks
>
> once you see the error, please upload the
> full dmesg output somewhere to we can track what
> goes on.
>
> Hopefully there won't be an oops this time which
> should make it easier for you to test (no need to
> reboot).
>
>
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index c1f1e3c..3cb679b 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -32,6 +32,7 @@
> #include<linux/eventfd.h>
> #include<linux/kernel.h>
> #include<linux/slab.h>
> +#include<linux/nmi.h>
>
> #include "iodev.h"
>
> @@ -43,6 +44,8 @@
> * --------------------------------------------------------------------
> */
>
> +#define KVM_BAD_PTR ((void*)(long)(0x6b6b6b6b6b6b6b6bull))
> +
> struct _irqfd {
> struct kvm *kvm;
> struct eventfd_ctx *eventfd;
> @@ -61,6 +64,13 @@ irqfd_inject(struct work_struct *work)
> {
> struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
> struct kvm *kvm = irqfd->kvm;
> + if (kvm == KVM_BAD_PTR) {
> + printk(KERN_ERR "Eventfd bug detected!\n");
> + printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
> + work, irqfd, kvm, irqfd->gsi);
> + trigger_all_cpu_backtrace();
> + return;
> + }
>
> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
> @@ -75,6 +85,8 @@ irqfd_shutdown(struct work_struct *work)
> struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
> u64 cnt;
>
> + printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> + work, irqfd, irqfd->kvm, irqfd->gsi);
> /*
> * Synchronize with the wait-queue and unhook ourselves to prevent
> * further events.
> @@ -91,6 +103,8 @@ irqfd_shutdown(struct work_struct *work)
> * It is now safe to release the object's resources
> */
> eventfd_ctx_put(irqfd->eventfd);
> + printk(KERN_ERR "kfree at %s(work=%p,irqfd=%p)\n", __func__,
> + work, irqfd);
> kfree(irqfd);
> }
>
> @@ -111,6 +125,8 @@ static void
> irqfd_deactivate(struct _irqfd *irqfd)
> {
> BUG_ON(!irqfd_is_active(irqfd));
> + printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> + irqfd, irqfd->kvm, irqfd->gsi);
>
> list_del_init(&irqfd->list);
>
> @@ -178,6 +194,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
>
> irqfd->kvm = kvm;
> irqfd->gsi = gsi;
> + printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> + irqfd, kvm, gsi);
> INIT_LIST_HEAD(&irqfd->list);
> INIT_WORK(&irqfd->inject, irqfd_inject);
> INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
> @@ -264,6 +282,8 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
> struct _irqfd *irqfd, *tmp;
> struct eventfd_ctx *eventfd;
>
> + printk(KERN_ERR "%s(kvm=%p, gsi=%d)\n", __func__,
> + kvm, gsi);
> eventfd = eventfd_ctx_fdget(fd);
> if (IS_ERR(eventfd))
> return PTR_ERR(eventfd);
> @@ -305,6 +325,7 @@ void
> kvm_irqfd_release(struct kvm *kvm)
> {
> struct _irqfd *irqfd, *tmp;
> + printk(KERN_ERR "%s(kvm=%p)\n", __func__, kvm);
>
> spin_lock_irq(&kvm->irqfds.lock);
>
> --
> 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
Hi,
I boot the host with the patched kernel yesterday.
No crach until now, but two "Eventfd bug detected!" in the log at "Mar
9 02:04:31" and "Mar 9 02:15:17"
You can find part of the log at the following adress:
http://filex.univ-nantes.fr/get?k=jL4Fe7yfSMN57toAH7V
It a split file of the kern.log (1,4G), so if you need another part of
the log, let me know.
Thanks for all.
Regards.
--
Jean-Philippe Menil - Pôle réseau Service IRTS
DSI Université de Nantes
jean-philippe.menil@univ-nantes.fr
Tel : 02.53.48.49.27 - Fax : 02.53.48.49.09
^ permalink raw reply
* Re: [Patch V2] bonding: fix netpoll in active-backup mode
From: Cong Wang @ 2011-03-09 12:32 UTC (permalink / raw)
To: Neil Horman; +Cc: linux-kernel, Jay Vosburgh, netdev
In-Reply-To: <20110308134348.GB19072@hmsreliant.think-freely.org>
于 2011年03月08日 21:43, Neil Horman 写道:
> On Tue, Mar 08, 2011 at 05:58:56PM +0800, Amerigo Wang wrote:
>> V2: avoid calling slave_diable_netpoll() with write_lock_bh() held.
>>
>> netconsole doesn't work in active-backup mode, because we don't do anything
>> for nic failover in active-backup mode. We should disable netpoll on the
>> failing slave when it is detected down and enable netpoll when it becomes
>> the active slave.
>>
> You still haven't explained why it needs to be this way. what exactly is the
> shortcomming with leaving netpoll enabled on all slaves, regardless of state?
> It should be sufficient if, during a failover in a mode where the inactive slave
> should not tx data, that you simply clear the slave __LINK_STATE_START bit. That will
> prevent higher layers from sending any queued data without you needing to muck
> about with netpoll state.
>
Oh, actually I thought the struct net_device of the failing interface will be freed,
but that is not true...
So, yeah, I think just setup netpoll in bond_netpoll_setup() should be sufficient.
Thanks.
^ permalink raw reply
* Re: [Patch V2] bonding: fix netpoll in active-backup mode
From: Cong Wang @ 2011-03-09 12:34 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: linux-kernel, Neil Horman, Jay Vosburgh, netdev
In-Reply-To: <20110308212405.GX11864@gospo.rdu.redhat.com>
于 2011年03月09日 05:24, Andy Gospodarek 写道:
> On Tue, Mar 08, 2011 at 05:58:56PM +0800, Amerigo Wang wrote:
>> V2: avoid calling slave_diable_netpoll() with write_lock_bh() held.
>>
>> netconsole doesn't work in active-backup mode, because we don't do anything
>> for nic failover in active-backup mode. We should disable netpoll on the
>> failing slave when it is detected down and enable netpoll when it becomes
>> the active slave.
>>
>> Tested by ifdown the current active slave and ifup it again for several times,
>> netconsole works well.
>>
>> Signed-off-by: WANG Cong<amwang@redhat.com>
>> Cc: Neil Horman<nhorman@tuxdriver.com>
>>
>
> It seems like you are going to a lot of trouble to fix a bug where
> netpoll will not be setup on any interface that is down when enslaved.
> That seems to be the only path that would not have slave->np setup
> properly at enslavement.
>
> Did you ever try just this?
That was my first thought, but I was over-worried about the failing slave.
This way should work too. Mind to send it as a normal patch? :)
Thanks!
^ permalink raw reply
* Reply.
From: Barr. Tan Chee Lan @ 2011-03-09 12:34 UTC (permalink / raw)
Dear Friend,
I am Barrister Tan Chee Lan, I have contacted you to assist in distributing the
money left behind by my client before it is confiscated or declared unserviceabl
e by the bank where this deposit valued at Six Million, One Hundred Thousand Uni
ted States Dollars (US$6.1 Million). This bank has issued me a notice to contact
the next of kin, or the account will be confiscated.
Best regards,
Tan Chee Lan
Attorney at Law
^ permalink raw reply
* Re: Bug inkvm_set_irq
From: Michael S. Tsirkin @ 2011-03-09 13:00 UTC (permalink / raw)
To: Jean-Philippe Menil; +Cc: netdev, kvm, virtualization
In-Reply-To: <4D77727B.2000207@univ-nantes.fr>
On Wed, Mar 09, 2011 at 01:28:43PM +0100, Jean-Philippe Menil wrote:
> Le 08/03/2011 12:13, Michael S. Tsirkin a écrit :
> >On Fri, Mar 04, 2011 at 10:39:05AM +0100, Jean-Philippe Menil wrote:
> >>Yes, it's a 2.6.37.2 kernel.
> >OK, here's a debugging patch.
> >Please run with slab debugging as previously until you see
> >'eventfd bug detected!' in dmesg or until there is a crash.
> >It might be also useful to enable timestampts on printk with
> > Symbol: PRINTK_TIME [=y]
> > │ Type : boolean
> > │ Prompt: Show timing information on printks
> >
> >once you see the error, please upload the
> >full dmesg output somewhere to we can track what
> >goes on.
> >
> >Hopefully there won't be an oops this time which
> >should make it easier for you to test (no need to
> >reboot).
> >
> >
> >diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> >index c1f1e3c..3cb679b 100644
> >--- a/virt/kvm/eventfd.c
> >+++ b/virt/kvm/eventfd.c
> >@@ -32,6 +32,7 @@
> > #include<linux/eventfd.h>
> > #include<linux/kernel.h>
> > #include<linux/slab.h>
> >+#include<linux/nmi.h>
> >
> > #include "iodev.h"
> >
> >@@ -43,6 +44,8 @@
> > * --------------------------------------------------------------------
> > */
> >
> >+#define KVM_BAD_PTR ((void*)(long)(0x6b6b6b6b6b6b6b6bull))
> >+
> > struct _irqfd {
> > struct kvm *kvm;
> > struct eventfd_ctx *eventfd;
> >@@ -61,6 +64,13 @@ irqfd_inject(struct work_struct *work)
> > {
> > struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
> > struct kvm *kvm = irqfd->kvm;
> >+ if (kvm == KVM_BAD_PTR) {
> >+ printk(KERN_ERR "Eventfd bug detected!\n");
> >+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
> >+ work, irqfd, kvm, irqfd->gsi);
> >+ trigger_all_cpu_backtrace();
> >+ return;
> >+ }
> >
> > kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
> > kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
> >@@ -75,6 +85,8 @@ irqfd_shutdown(struct work_struct *work)
> > struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
> > u64 cnt;
> >
> >+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >+ work, irqfd, irqfd->kvm, irqfd->gsi);
> > /*
> > * Synchronize with the wait-queue and unhook ourselves to prevent
> > * further events.
> >@@ -91,6 +103,8 @@ irqfd_shutdown(struct work_struct *work)
> > * It is now safe to release the object's resources
> > */
> > eventfd_ctx_put(irqfd->eventfd);
> >+ printk(KERN_ERR "kfree at %s(work=%p,irqfd=%p)\n", __func__,
> >+ work, irqfd);
> > kfree(irqfd);
> > }
> >
> >@@ -111,6 +125,8 @@ static void
> > irqfd_deactivate(struct _irqfd *irqfd)
> > {
> > BUG_ON(!irqfd_is_active(irqfd));
> >+ printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >+ irqfd, irqfd->kvm, irqfd->gsi);
> >
> > list_del_init(&irqfd->list);
> >
> >@@ -178,6 +194,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
> >
> > irqfd->kvm = kvm;
> > irqfd->gsi = gsi;
> >+ printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >+ irqfd, kvm, gsi);
> > INIT_LIST_HEAD(&irqfd->list);
> > INIT_WORK(&irqfd->inject, irqfd_inject);
> > INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
> >@@ -264,6 +282,8 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
> > struct _irqfd *irqfd, *tmp;
> > struct eventfd_ctx *eventfd;
> >
> >+ printk(KERN_ERR "%s(kvm=%p, gsi=%d)\n", __func__,
> >+ kvm, gsi);
> > eventfd = eventfd_ctx_fdget(fd);
> > if (IS_ERR(eventfd))
> > return PTR_ERR(eventfd);
> >@@ -305,6 +325,7 @@ void
> > kvm_irqfd_release(struct kvm *kvm)
> > {
> > struct _irqfd *irqfd, *tmp;
> >+ printk(KERN_ERR "%s(kvm=%p)\n", __func__, kvm);
> >
> > spin_lock_irq(&kvm->irqfds.lock);
> >
> >--
> >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
> Hi,
>
> I boot the host with the patched kernel yesterday.
> No crach until now, but two "Eventfd bug detected!" in the log at
> "Mar 9 02:04:31" and "Mar 9 02:15:17"
> You can find part of the log at the following adress:
> http://filex.univ-nantes.fr/get?k=jL4Fe7yfSMN57toAH7V
>
> It a split file of the kern.log (1,4G), so if you need another part
> of the log, let me know.
>
> Thanks for all.
>
> Regards.
Downloading, it's big :)
What about some 1000 lines before and after Eventfd bug detected! line?
^ permalink raw reply
* Re: [PATCH 1/2] net: Allow no-cache copy from user on transmit
From: Michał Mirosław @ 2011-03-09 13:04 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev
In-Reply-To: <alpine.DEB.2.00.1103082053350.1433@pokey.mtv.corp.google.com>
2011/3/9 Tom Herbert <therbert@google.com>:
> This patch uses __copy_from_user_nocache (from skb_copy_to_page)
> on transmit to bypass data cache for a performance improvement.
> This functionality is configurable per device using ethtool, the
> device must also be doing TX csum offload to enable. It seems
> reasonable to set this when the device does not copy or
> otherwise touch the data.
>
> This patch was tested using 200 instances of netperf TCP_RR with
> 1400 byte request and one byte reply. Platform is 16 core AMD.
>
> No-cache copy disabled:
> 672703 tps, 97.13% client utilization
> 50/90/99% latency 244.31 484.205 1028.41
>
> No-cache copy enabled:
> 702113 tps, 96.16% client utilization,
> 50/90/99% latency 238.56 467.56 956.955
>
> This seems to provide a nice little performance improvement and is
> consistent in the tests I ran. Presumably, this would provide
> the greatest benfits in the presence of an application workload
> stressing the cache and a lot of transmit data happening. I don't
> yet see a downside to using this.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> include/linux/ethtool.h | 4 ++++
> include/linux/netdevice.h | 4 +++-
> include/net/sock.h | 5 +++++
> net/core/ethtool.c | 11 ++++++++++-
> 4 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index aac3e2e..2cf3cc9 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -833,6 +833,10 @@ struct ethtool_ops {
> #define ETHTOOL_GFEATURES 0x0000003a /* Get device offload settings */
> #define ETHTOOL_SFEATURES 0x0000003b /* Change device offload settings */
>
> +#define ETHTOOL_GNCCOPY 0x0000003c /* Get no-cache-copy enable
> + * (ethtool_value). */
> +#define ETHTOOL_SNCCOPY 0x0000003d /* Set no-cache-copy enable
> + * (ethtool_value). */
Please don't add new get/set ops for netdev features - G/SFEATURES
handle them all.
Proper way is to modify NETIF_F_ETHTOOL_BITS, netdev_fix_features()
and, if needed for now,
legacy get/set ops (they will go away, eventually).
> /* compatibility with older code */
> #define SPARC_ETH_GSET ETHTOOL_GSET
> #define SPARC_ETH_SSET ETHTOOL_SSET
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 71563e7..1b72f29 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -978,6 +978,7 @@ struct net_device {
> #define NETIF_F_NTUPLE (1 << 27) /* N-tuple filters supported */
> #define NETIF_F_RXHASH (1 << 28) /* Receive hashing offload */
> #define NETIF_F_RXCSUM (1 << 29) /* Receive checksumming offload */
> +#define NETIF_F_NOCACHE_COPY (1 << 30) /* Use no-cache copyfromuser */
>
> /* Segmentation offload features */
> #define NETIF_F_GSO_SHIFT 16
> @@ -1020,7 +1021,8 @@ struct net_device {
> NETIF_F_FRAGLIST)
>
> /* changeable features with no special hardware requirements */
> -#define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO)
> +#define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO | \
> + NETIF_F_NOCACHE_COPY)
>
> /* Interface index. Unique device identifier */
> int ifindex;
> diff --git a/include/net/sock.h b/include/net/sock.h
> index da0534d..55b1385 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1401,6 +1401,11 @@ static inline int skb_copy_to_page(struct sock *sk, char __user *from,
> if (err)
> return err;
> skb->csum = csum_block_add(skb->csum, csum, skb->len);
> + } else if (sk->sk_route_caps & NETIF_F_NOCACHE_COPY) {
> + if (!access_ok(VERIFY_READ, from, copy)
> + || __copy_from_user_nocache(page_address(page) + off,
> + from, copy))
> + return -EFAULT;
> } else if (copy_from_user(page_address(page) + off, from, copy))
> return -EFAULT;
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index c1a71bb..a8b2638 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -328,6 +328,7 @@ static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GS
> /* NETIF_F_NETNS_LOCAL */ "netns-local",
> /* NETIF_F_GRO */ "rx-gro",
> /* NETIF_F_LRO */ "rx-lro",
> + /* NETIF_F_NOCACHE_COPY */ "tx-nocache-copy",
>
> /* NETIF_F_TSO */ "tx-tcp-segmentation",
> /* NETIF_F_UFO */ "tx-udp-fragmentation",
You can't put the strings in the middle of this array. Positions in
the array are dependent on flag bit position.
[...]
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: Bug inkvm_set_irq
From: Jean-Philippe Menil @ 2011-03-09 13:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20110309130000.GA30297@redhat.com>
Le 09/03/2011 14:00, Michael S. Tsirkin a écrit :
> On Wed, Mar 09, 2011 at 01:28:43PM +0100, Jean-Philippe Menil wrote:
>> Le 08/03/2011 12:13, Michael S. Tsirkin a écrit :
>>> On Fri, Mar 04, 2011 at 10:39:05AM +0100, Jean-Philippe Menil wrote:
>>>> Yes, it's a 2.6.37.2 kernel.
>>> OK, here's a debugging patch.
>>> Please run with slab debugging as previously until you see
>>> 'eventfd bug detected!' in dmesg or until there is a crash.
>>> It might be also useful to enable timestampts on printk with
>>> Symbol: PRINTK_TIME [=y]
>>> │ Type : boolean
>>> │ Prompt: Show timing information on printks
>>>
>>> once you see the error, please upload the
>>> full dmesg output somewhere to we can track what
>>> goes on.
>>>
>>> Hopefully there won't be an oops this time which
>>> should make it easier for you to test (no need to
>>> reboot).
>>>
>>>
>>> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
>>> index c1f1e3c..3cb679b 100644
>>> --- a/virt/kvm/eventfd.c
>>> +++ b/virt/kvm/eventfd.c
>>> @@ -32,6 +32,7 @@
>>> #include<linux/eventfd.h>
>>> #include<linux/kernel.h>
>>> #include<linux/slab.h>
>>> +#include<linux/nmi.h>
>>>
>>> #include "iodev.h"
>>>
>>> @@ -43,6 +44,8 @@
>>> * --------------------------------------------------------------------
>>> */
>>>
>>> +#define KVM_BAD_PTR ((void*)(long)(0x6b6b6b6b6b6b6b6bull))
>>> +
>>> struct _irqfd {
>>> struct kvm *kvm;
>>> struct eventfd_ctx *eventfd;
>>> @@ -61,6 +64,13 @@ irqfd_inject(struct work_struct *work)
>>> {
>>> struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
>>> struct kvm *kvm = irqfd->kvm;
>>> + if (kvm == KVM_BAD_PTR) {
>>> + printk(KERN_ERR "Eventfd bug detected!\n");
>>> + printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
>>> + work, irqfd, kvm, irqfd->gsi);
>>> + trigger_all_cpu_backtrace();
>>> + return;
>>> + }
>>>
>>> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
>>> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
>>> @@ -75,6 +85,8 @@ irqfd_shutdown(struct work_struct *work)
>>> struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
>>> u64 cnt;
>>>
>>> + printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
>>> + work, irqfd, irqfd->kvm, irqfd->gsi);
>>> /*
>>> * Synchronize with the wait-queue and unhook ourselves to prevent
>>> * further events.
>>> @@ -91,6 +103,8 @@ irqfd_shutdown(struct work_struct *work)
>>> * It is now safe to release the object's resources
>>> */
>>> eventfd_ctx_put(irqfd->eventfd);
>>> + printk(KERN_ERR "kfree at %s(work=%p,irqfd=%p)\n", __func__,
>>> + work, irqfd);
>>> kfree(irqfd);
>>> }
>>>
>>> @@ -111,6 +125,8 @@ static void
>>> irqfd_deactivate(struct _irqfd *irqfd)
>>> {
>>> BUG_ON(!irqfd_is_active(irqfd));
>>> + printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
>>> + irqfd, irqfd->kvm, irqfd->gsi);
>>>
>>> list_del_init(&irqfd->list);
>>>
>>> @@ -178,6 +194,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
>>>
>>> irqfd->kvm = kvm;
>>> irqfd->gsi = gsi;
>>> + printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
>>> + irqfd, kvm, gsi);
>>> INIT_LIST_HEAD(&irqfd->list);
>>> INIT_WORK(&irqfd->inject, irqfd_inject);
>>> INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
>>> @@ -264,6 +282,8 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
>>> struct _irqfd *irqfd, *tmp;
>>> struct eventfd_ctx *eventfd;
>>>
>>> + printk(KERN_ERR "%s(kvm=%p, gsi=%d)\n", __func__,
>>> + kvm, gsi);
>>> eventfd = eventfd_ctx_fdget(fd);
>>> if (IS_ERR(eventfd))
>>> return PTR_ERR(eventfd);
>>> @@ -305,6 +325,7 @@ void
>>> kvm_irqfd_release(struct kvm *kvm)
>>> {
>>> struct _irqfd *irqfd, *tmp;
>>> + printk(KERN_ERR "%s(kvm=%p)\n", __func__, kvm);
>>>
>>> spin_lock_irq(&kvm->irqfds.lock);
>>>
>>> --
>>> 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
>> Hi,
>>
>> I boot the host with the patched kernel yesterday.
>> No crach until now, but two "Eventfd bug detected!" in the log at
>> "Mar 9 02:04:31" and "Mar 9 02:15:17"
>> You can find part of the log at the following adress:
>> http://filex.univ-nantes.fr/get?k=jL4Fe7yfSMN57toAH7V
>>
>> It a split file of the kern.log (1,4G), so if you need another part
>> of the log, let me know.
>>
>> Thanks for all.
>>
>> Regards.
> Downloading, it's big :)
> What about some 1000 lines before and after Eventfd bug detected! line?
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Yes, sorry about that.
I could have split my log into a smaller file.
I was a little afraid of not transmit enough informations, and i was a
bit wide.
I hope you can find usefull trace anyway.
Regards.
--
Jean-Philippe Menil - Pôle réseau Service IRTS
DSI Université de Nantes
jean-philippe.menil@univ-nantes.fr
Tel : 02.53.48.49.27 - Fax : 02.53.48.49.09
^ permalink raw reply
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Neil Horman @ 2011-03-09 13:33 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: Andy Gospodarek, Jiri Pirko, netdev, davem, shemminger, kaber,
fubar, eric.dumazet
In-Reply-To: <4D76A345.9040200@gmail.com>
On Tue, Mar 08, 2011 at 10:44:37PM +0100, Nicolas de Pesloüan wrote:
> Le 08/03/2011 14:42, Andy Gospodarek a écrit :
> >I'm pretty sure this patch will have the same catastrophic problem your
> >last one did. By cloning and setting skb2->dev = orig_dev you just
> >inserted a frame identical to the one we received right back into the
> >stack. It only took a few minutes for my box to melt as one frame on
> >the wire will cause an infinite number of frames to be received by the
> >stack.
>
> I agree with Andy. We still keep one reinject (netif_rx), which is
> probably better that two (__netif_receive_skb), but not enough.
>
> I really think we need a general framework for late delivery of
> final packets to packet handler registered somewhere in the
> rx_handler path.
>
> Jiri, is this patch the one you announced as "I have some kind nice
> solution in mind and I'm going to submit that as a patch later (too
> many patches are in the wind atm)" ?
I've not had time to flesh any of this out, but have you considered the use of
privately allocated net namespaces to define the receive order of frames when
doing multiple re-injections through netif_rx/netif_receive_skb. If you
modified the ptype list traversals so validate that dev_net(ptype->dev) ==
dev_net(skb->dev) before delivery, you could allocate private net namespaces in
your virutal master drivers (bridges/bonds/vlans) and place the slave devices in
those spaces, so that only the parent device could receive frames on them.
Of course thats going to have its own set of problems to deal with, but it might
be worth considering.
Neil
> --
> 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
>
>
^ permalink raw reply
* Re: Bug inkvm_set_irq
From: Michael S. Tsirkin @ 2011-03-09 13:59 UTC (permalink / raw)
To: Jean-Philippe Menil; +Cc: netdev, kvm, virtualization
In-Reply-To: <4D777CDA.3050500@univ-nantes.fr>
On Wed, Mar 09, 2011 at 02:12:58PM +0100, Jean-Philippe Menil wrote:
> Le 09/03/2011 14:00, Michael S. Tsirkin a écrit :
> >On Wed, Mar 09, 2011 at 01:28:43PM +0100, Jean-Philippe Menil wrote:
> >>Le 08/03/2011 12:13, Michael S. Tsirkin a écrit :
> >>>On Fri, Mar 04, 2011 at 10:39:05AM +0100, Jean-Philippe Menil wrote:
> >>>>Yes, it's a 2.6.37.2 kernel.
> >>>OK, here's a debugging patch.
> >>>Please run with slab debugging as previously until you see
> >>>'eventfd bug detected!' in dmesg or until there is a crash.
> >>>It might be also useful to enable timestampts on printk with
> >>> Symbol: PRINTK_TIME [=y]
> >>> │ Type : boolean
> >>> │ Prompt: Show timing information on printks
> >>>
> >>>once you see the error, please upload the
> >>>full dmesg output somewhere to we can track what
> >>>goes on.
> >>>
> >>>Hopefully there won't be an oops this time which
> >>>should make it easier for you to test (no need to
> >>>reboot).
> >>>
> >>>
> >>>diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> >>>index c1f1e3c..3cb679b 100644
> >>>--- a/virt/kvm/eventfd.c
> >>>+++ b/virt/kvm/eventfd.c
> >>>@@ -32,6 +32,7 @@
> >>> #include<linux/eventfd.h>
> >>> #include<linux/kernel.h>
> >>> #include<linux/slab.h>
> >>>+#include<linux/nmi.h>
> >>>
> >>> #include "iodev.h"
> >>>
> >>>@@ -43,6 +44,8 @@
> >>> * --------------------------------------------------------------------
> >>> */
> >>>
> >>>+#define KVM_BAD_PTR ((void*)(long)(0x6b6b6b6b6b6b6b6bull))
> >>>+
> >>> struct _irqfd {
> >>> struct kvm *kvm;
> >>> struct eventfd_ctx *eventfd;
> >>>@@ -61,6 +64,13 @@ irqfd_inject(struct work_struct *work)
> >>> {
> >>> struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
> >>> struct kvm *kvm = irqfd->kvm;
> >>>+ if (kvm == KVM_BAD_PTR) {
> >>>+ printk(KERN_ERR "Eventfd bug detected!\n");
> >>>+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
> >>>+ work, irqfd, kvm, irqfd->gsi);
> >>>+ trigger_all_cpu_backtrace();
> >>>+ return;
> >>>+ }
> >>>
> >>> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
> >>> kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
> >>>@@ -75,6 +85,8 @@ irqfd_shutdown(struct work_struct *work)
> >>> struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
> >>> u64 cnt;
> >>>
> >>>+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >>>+ work, irqfd, irqfd->kvm, irqfd->gsi);
> >>> /*
> >>> * Synchronize with the wait-queue and unhook ourselves to prevent
> >>> * further events.
> >>>@@ -91,6 +103,8 @@ irqfd_shutdown(struct work_struct *work)
> >>> * It is now safe to release the object's resources
> >>> */
> >>> eventfd_ctx_put(irqfd->eventfd);
> >>>+ printk(KERN_ERR "kfree at %s(work=%p,irqfd=%p)\n", __func__,
> >>>+ work, irqfd);
> >>> kfree(irqfd);
> >>> }
> >>>
> >>>@@ -111,6 +125,8 @@ static void
> >>> irqfd_deactivate(struct _irqfd *irqfd)
> >>> {
> >>> BUG_ON(!irqfd_is_active(irqfd));
> >>>+ printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >>>+ irqfd, irqfd->kvm, irqfd->gsi);
> >>>
> >>> list_del_init(&irqfd->list);
> >>>
> >>>@@ -178,6 +194,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
> >>>
> >>> irqfd->kvm = kvm;
> >>> irqfd->gsi = gsi;
> >>>+ printk(KERN_ERR "%s(irqfd=%p,kvm=%p, gsi=%d)\n", __func__,
> >>>+ irqfd, kvm, gsi);
> >>> INIT_LIST_HEAD(&irqfd->list);
> >>> INIT_WORK(&irqfd->inject, irqfd_inject);
> >>> INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
> >>>@@ -264,6 +282,8 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
> >>> struct _irqfd *irqfd, *tmp;
> >>> struct eventfd_ctx *eventfd;
> >>>
> >>>+ printk(KERN_ERR "%s(kvm=%p, gsi=%d)\n", __func__,
> >>>+ kvm, gsi);
> >>> eventfd = eventfd_ctx_fdget(fd);
> >>> if (IS_ERR(eventfd))
> >>> return PTR_ERR(eventfd);
> >>>@@ -305,6 +325,7 @@ void
> >>> kvm_irqfd_release(struct kvm *kvm)
> >>> {
> >>> struct _irqfd *irqfd, *tmp;
> >>>+ printk(KERN_ERR "%s(kvm=%p)\n", __func__, kvm);
> >>>
> >>> spin_lock_irq(&kvm->irqfds.lock);
> >>>
> >>>--
> >>>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
> >>Hi,
> >>
> >>I boot the host with the patched kernel yesterday.
> >>No crach until now, but two "Eventfd bug detected!" in the log at
> >>"Mar 9 02:04:31" and "Mar 9 02:15:17"
> >>You can find part of the log at the following adress:
> >>http://filex.univ-nantes.fr/get?k=jL4Fe7yfSMN57toAH7V
> >>
> >>It a split file of the kern.log (1,4G), so if you need another part
> >>of the log, let me know.
> >>
> >>Thanks for all.
> >>
> >>Regards.
> >Downloading, it's big :)
> >What about some 1000 lines before and after Eventfd bug detected! line?
> >--
> >To unsubscribe from this list: send the line "unsubscribe kvm" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
> Yes, sorry about that.
> I could have split my log into a smaller file.
>
> I was a little afraid of not transmit enough informations, and i was
> a bit wide.
> I hope you can find usefull trace anyway.
>
> Regards.
OK, use after free.
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.593681]
kvm_irqfd_assign(irqfd=ffff88045e8d6230,kvm=ffff88085151c000, gsi=26)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605359]
kvm_irqfd_deassign(kvm=ffff88085151c000, gsi=26)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605388]
irqfd_deactivate(irqfd=ffff88045e8d6230,kvm=ffff88085151c000, gsi=26)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605461]
irqfd_shutdown(work=ffff88045e8d62b0,irqfd=ffff88045e8d6230,kvm=ffff88085151c000,
gsi=26)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605509]
kfree at irqfd_shutdown(work=ffff88045e8d62b0,irqfd=ffff88045e8d6230)
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605588]
Eventfd bug detected!
Mar 9 02:15:17 ayrshire.u06.univ-nantes.prive kernel: [37461.605612]
irqfd_inject(work=ffff88045e8d6290,irqfd=ffff88045e8d6230,kvm=6b6b6b6b6b6b6b6b,gsi=1802201963)
and 1802201963 is also 6b6b6b6b.
I suspect that somehow, we get an event from eventfd even
though we did eventfd_ctx_remove_wait_queue.
Could you please try the following patch on top?
When you see Eventfd bug or Wakeup bug, paste some
last lines.
For the full log file - better compress with xz.
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 3cb679b..63aeba5 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -92,6 +92,8 @@ irqfd_shutdown(struct work_struct *work)
* further events.
*/
eventfd_ctx_remove_wait_queue(irqfd->eventfd, &irqfd->wait, &cnt);
+ printk(KERN_ERR "remove wq at %s(work=%p,irqfd=%p)\n", __func__,
+ work, irqfd);
/*
* We know no new events will be scheduled at this point, so block
@@ -142,6 +144,13 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
unsigned long flags = (unsigned long)key;
+ if (irqfd->kvm == KVM_BAD_PTR) {
+ printk(KERN_ERR "Wakeup bug detected! flags 0x%lx\n", flags);
+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d)\n", __func__,
+ &irqfd->inject, irqfd, irqfd->kvm, irqfd->gsi);
+ trigger_all_cpu_backtrace();
+ return 0;
+ }
if (flags & POLLIN)
/* An event has been signaled, inject an interrupt */
schedule_work(&irqfd->inject);
@@ -153,6 +162,9 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
spin_lock_irqsave(&kvm->irqfds.lock, flags);
+ printk(KERN_ERR "%s(work=%p,irqfd=%p,kvm=%p,gsi=%d,active=%d)\n", __func__,
+ &irqfd->inject, irqfd, irqfd->kvm, irqfd->gsi,
+ irqfd_is_active(irqfd));
/*
* We must check if someone deactivated the irqfd before
* we could acquire the irqfds.lock since the item is
--
MST
^ permalink raw reply related
* [PATCH] Davinci: Do not reset EMAC TX overruns counter on read
From: Thomas Lange @ 2011-03-09 14:41 UTC (permalink / raw)
To: netdev
Cc: davinci-linux-open-source@linux.davincidsp.com, davem, anantgole,
chaithrika
Don't reset tx_fifo_errors when reading out current EMAC stats.
(tx_fifo_errors shows up as TX overruns in netdev stats.)
Without this correction, the old counter value is lost every time
stats are read out.
Signed-off-by: Thomas Lange <thomas@corelatus.se>
---
drivers/net/davinci_emac.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index 2a628d1..922c8c5 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -1730,7 +1730,7 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
emac_read(EMAC_TXCARRIERSENSE);
emac_write(EMAC_TXCARRIERSENSE, stats_clear_mask);
- ndev->stats.tx_fifo_errors = emac_read(EMAC_TXUNDERRUN);
+ ndev->stats.tx_fifo_errors += emac_read(EMAC_TXUNDERRUN);
emac_write(EMAC_TXUNDERRUN, stats_clear_mask);
return &ndev->stats;
--
1.7.2.5
^ permalink raw reply related
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Nicolas de Pesloüan @ 2011-03-09 14:49 UTC (permalink / raw)
To: Jiri Pirko
Cc: Andy Gospodarek, netdev, davem, shemminger, kaber, fubar,
eric.dumazet
In-Reply-To: <20110309074547.GA2808@psychotron.redhat.com>
Le 09/03/2011 08:45, Jiri Pirko a écrit :
> Tue, Mar 08, 2011 at 10:44:37PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 08/03/2011 14:42, Andy Gospodarek a écrit :
>>> I'm pretty sure this patch will have the same catastrophic problem your
>>> last one did. By cloning and setting skb2->dev = orig_dev you just
>>> inserted a frame identical to the one we received right back into the
>>> stack. It only took a few minutes for my box to melt as one frame on
>>> the wire will cause an infinite number of frames to be received by the
>>> stack.
>>
>> I agree with Andy. We still keep one reinject (netif_rx), which is
>> probably better that two (__netif_receive_skb), but not enough.
>>
>> I really think we need a general framework for late delivery of final
>> packets to packet handler registered somewhere in the rx_handler
>> path.
>>
>> Jiri, is this patch the one you announced as "I have some kind nice
>> solution in mind and I'm going to submit that as a patch later (too
>> many patches are in the wind atm)" ?
>
>
> I did not had time to verify my thought yet but I think that the only
> think needed against my original patch (bonding: move processing of recv
> handlers into handle_frame()) is ro remove vlan_on_bond_hook, period.
>
> Because all incoming arps are seen by bond_handle_frame =>
> bond->recv_probe , even vlan ones - that would make eth0-bond0-bond0.5
> work and eth0-bond0-br0-bond0.5 as well. But again, need to verify this.
All incoming ARPs are seen by bond_handle_frame, even vlan ones, definitely true.
But as some of them *are* vlan ones, you will have to untag those "by hand" inside bond_handle_frame.
It might work, but I wouldn't support the idea, for two reasons :
- It would duplicate code, or at least, duplicate places where untagging happens.
- It would cause some vlan hacks to sit inside bond, which is not nice from a layering point of
view. You recently advocated against breaking layering and you are right. We should avoid putting X
related stuff into Y part of the network stack, as much as possible.
Again, I think we should try and find a generic way to have the final frame delivered to whoever
needs it. And this way should not be limited to "vlan untagging" nor to bonding.
Nicolas.
^ permalink raw reply
* Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state
From: Américo Wang @ 2011-03-09 15:09 UTC (permalink / raw)
To: Adam Majer
Cc: netdev, Stephen Hemminger, David S. Miller, Alexey Kuznetsov,
Pekka Savola (ipv6), James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, bridge, linux-kernel
In-Reply-To: <20110306051833.GA3098@mira.lan.galacticasoftware.com>
On Sat, Mar 05, 2011 at 11:18:33PM -0600, Adam Majer wrote:
>
>IPv6 address autoconfiguration relies on an UP interface to processes
>traffic normally. A bridge that is UP enters LEARNING state and this
>state "eats" any Router Advertising packets sent to the
>bridge. Issuing a NETDEV_CHANGE notification on the bridge interface
>when it changes state allows autoconfiguration code to retry
>querying router information.
>
...
>+static void br_port_change_notifier_handler(struct work_struct *work)
>+{
>+ struct net_bridge *br = container_of(work,
>+ struct net_bridge,
>+ change_notification_worker);
>+
>+ rtnl_lock();
>+ netdev_state_change(br->dev);
>+ rtnl_unlock();
>+}
>+
Do you really want user-space to get this notification too?
Why do you put it into a workqueue? Maybe it has to be called in
process-context?
Thanks.
^ permalink raw reply
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Jiri Pirko @ 2011-03-09 15:09 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: Andy Gospodarek, netdev, davem, shemminger, kaber, fubar,
eric.dumazet
In-Reply-To: <4D77938D.3080408@gmail.com>
Wed, Mar 09, 2011 at 03:49:49PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 09/03/2011 08:45, Jiri Pirko a écrit :
>>Tue, Mar 08, 2011 at 10:44:37PM CET, nicolas.2p.debian@gmail.com wrote:
>>>Le 08/03/2011 14:42, Andy Gospodarek a écrit :
>>>>I'm pretty sure this patch will have the same catastrophic problem your
>>>>last one did. By cloning and setting skb2->dev = orig_dev you just
>>>>inserted a frame identical to the one we received right back into the
>>>>stack. It only took a few minutes for my box to melt as one frame on
>>>>the wire will cause an infinite number of frames to be received by the
>>>>stack.
>>>
>>>I agree with Andy. We still keep one reinject (netif_rx), which is
>>>probably better that two (__netif_receive_skb), but not enough.
>>>
>>>I really think we need a general framework for late delivery of final
>>>packets to packet handler registered somewhere in the rx_handler
>>>path.
>>>
>>>Jiri, is this patch the one you announced as "I have some kind nice
>>>solution in mind and I'm going to submit that as a patch later (too
>>>many patches are in the wind atm)" ?
>>
>>
>>I did not had time to verify my thought yet but I think that the only
>>think needed against my original patch (bonding: move processing of recv
>>handlers into handle_frame()) is ro remove vlan_on_bond_hook, period.
>>
>>Because all incoming arps are seen by bond_handle_frame =>
>>bond->recv_probe , even vlan ones - that would make eth0-bond0-bond0.5
>>work and eth0-bond0-br0-bond0.5 as well. But again, need to verify this.
>
>All incoming ARPs are seen by bond_handle_frame, even vlan ones, definitely true.
>
>But as some of them *are* vlan ones, you will have to untag those "by hand" inside bond_handle_frame.
Hmm. For hw vlan accel this would not be needed. But for
non-hw-vlan-accel the frame is wrapped when going thru handle_frame.
And yes, in that case untagging would be necessary. Unless the vlan
untagging happening now in ptype_base handler is moved in rx path
somewhere before __netif_receive_skb. That would result in two things:
1) Bond would be able to scope vlan packets
2) The rx path for hw-vlan-accel and non-hw-vlan-accel would be the same
(should be desirable + one reinjection would be avoided for
non-hw-vlan-accel)
>
>It might work, but I wouldn't support the idea, for two reasons :
>
>- It would duplicate code, or at least, duplicate places where untagging happens.
>- It would cause some vlan hacks to sit inside bond, which is not
>nice from a layering point of view. You recently advocated against
>breaking layering and you are right. We should avoid putting X
>related stuff into Y part of the network stack, as much as possible.
>
>Again, I think we should try and find a generic way to have the final
>frame delivered to whoever needs it. And this way should not be
>limited to "vlan untagging" nor to bonding.
>
> Nicolas.
^ permalink raw reply
* Re: [patch net-next-2.6] net: reinject arps into bonding slave instead of master
From: Nicolas de Pesloüan @ 2011-03-09 15:28 UTC (permalink / raw)
To: Jiri Pirko
Cc: Andy Gospodarek, netdev, davem, shemminger, kaber, fubar,
eric.dumazet
In-Reply-To: <20110309150939.GA9013@psychotron.brq.redhat.com>
Le 09/03/2011 16:09, Jiri Pirko a écrit :
> Wed, Mar 09, 2011 at 03:49:49PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 09/03/2011 08:45, Jiri Pirko a écrit :
>>> Tue, Mar 08, 2011 at 10:44:37PM CET, nicolas.2p.debian@gmail.com wrote:
>>>> Le 08/03/2011 14:42, Andy Gospodarek a écrit :
>>>>> I'm pretty sure this patch will have the same catastrophic problem your
>>>>> last one did. By cloning and setting skb2->dev = orig_dev you just
>>>>> inserted a frame identical to the one we received right back into the
>>>>> stack. It only took a few minutes for my box to melt as one frame on
>>>>> the wire will cause an infinite number of frames to be received by the
>>>>> stack.
>>>>
>>>> I agree with Andy. We still keep one reinject (netif_rx), which is
>>>> probably better that two (__netif_receive_skb), but not enough.
>>>>
>>>> I really think we need a general framework for late delivery of final
>>>> packets to packet handler registered somewhere in the rx_handler
>>>> path.
>>>>
>>>> Jiri, is this patch the one you announced as "I have some kind nice
>>>> solution in mind and I'm going to submit that as a patch later (too
>>>> many patches are in the wind atm)" ?
>>>
>>>
>>> I did not had time to verify my thought yet but I think that the only
>>> think needed against my original patch (bonding: move processing of recv
>>> handlers into handle_frame()) is ro remove vlan_on_bond_hook, period.
>>>
>>> Because all incoming arps are seen by bond_handle_frame =>
>>> bond->recv_probe , even vlan ones - that would make eth0-bond0-bond0.5
>>> work and eth0-bond0-br0-bond0.5 as well. But again, need to verify this.
>>
>> All incoming ARPs are seen by bond_handle_frame, even vlan ones, definitely true.
>>
>> But as some of them *are* vlan ones, you will have to untag those "by hand" inside bond_handle_frame.
>
> Hmm. For hw vlan accel this would not be needed.
Agreed.
> But for non-hw-vlan-accel the frame is wrapped when going thru handle_frame. And yes, in that
> case untagging would be necessary. Unless the vlan untagging happening now in ptype_base handler
> is moved in rx path somewhere before __netif_receive_skb.
Can't it me moved not before, but inside __netif_receive_skb, as a rx_handler?
At the time we setup eth0.100, we can arrange for a vlan_untag rx_handler to be registered on eth0.
This is exactly the way it works now for macvlan. Should it be possible (and desirable) for vlan too?
This might re-open the discussion about the need for several rx_handlers per interface, but that is
another story.
Also, moving it before __netif_receive_skb would cause every protocol handlers to receive the frame
untagged. At least protocol sniffers registered at ptype_all may want to receive the tagged frame,
for diagnostic purpose.
That would result in two things:
>
> 1) Bond would be able to scope vlan packets
> 2) The rx path for hw-vlan-accel and non-hw-vlan-accel would be the same
> (should be desirable + one reinjection would be avoided for
> non-hw-vlan-accel)
Agreed, but moving it inside __netif_receive_skb should have the same effects. If we move
non-hw-accel-vlan to a rx_handler, the skb would be COW before being untagged only if shared. This
sounds good to me.
Nicolas.
^ permalink raw reply
* Re: Network performance with small packets
From: Shirley Ma @ 2011-03-09 15:42 UTC (permalink / raw)
To: habanero
Cc: Rusty Russell, Michael S. Tsirkin, Krishna Kumar2, David Miller,
kvm, netdev, steved, Tom Lendacky
In-Reply-To: <1299637278.13202.61.camel@localhost.localdomain>
On Tue, 2011-03-08 at 20:21 -0600, Andrew Theurer wrote:
> Tom L has started using Rusty's patches and found some interesting
> results, sent yesterday:
> http://marc.info/?l=kvm&m=129953710930124&w=2
Thanks. Very good experimental. I have been struggling with guest/vhost
optimization work for a while. I created different experimental patches,
performance results really depends on workloads.
Based on the discussions and findings, seems that to improve
virtio_net/vhost optimization work, we really need to collect more
statistics data on both virtio_net and vhost for both TX and RX.
A way to filter number of guest exits, I/O exits, irq injections in
guest networking stacks only would be helpful.
Thanks
Shirley
^ permalink raw reply
* Re: Network performance with small packets - continued
From: Shirley Ma @ 2011-03-09 15:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, Krishna Kumar2, David Miller, kvm, netdev, steved,
Tom Lendacky
In-Reply-To: <20110309071558.GA25757@redhat.com>
On Wed, 2011-03-09 at 09:15 +0200, Michael S. Tsirkin wrote:
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 82dba5a..ebe3337 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -514,11 +514,11 @@ static unsigned int free_old_xmit_skbs(struct
> virtnet_info *vi)
> struct sk_buff *skb;
> unsigned int len, tot_sgs = 0;
>
> - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> + if ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> pr_debug("Sent skb %p\n", skb);
> vi->dev->stats.tx_bytes += skb->len;
> vi->dev->stats.tx_packets++;
> - tot_sgs += skb_vnet_hdr(skb)->num_sg;
> + tot_sgs = 2+MAX_SKB_FRAGS;
> dev_kfree_skb_any(skb);
> }
> return tot_sgs;
Return value should be different based on indirect or direct buffers
here?
> @@ -576,9 +576,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> struct virtnet_info *vi = netdev_priv(dev);
> int capacity;
>
> - /* Free up any pending old buffers before queueing new ones.
> */
> - free_old_xmit_skbs(vi);
> -
> /* Try to transmit */
> capacity = xmit_skb(vi, skb);
>
> @@ -605,6 +602,10 @@ static netdev_tx_t start_xmit(struct sk_buff
> *skb, struct net_device *dev)
> skb_orphan(skb);
> nf_reset(skb);
>
> + /* Free up any old buffers so we can queue new ones. */
> + if (capacity < 2+MAX_SKB_FRAGS)
> + capacity += free_old_xmit_skbs(vi);
> +
> /* Apparently nice girls don't return TX_BUSY; stop the queue
> * before it gets out of hand. Naturally, this wastes
> entries. */
> if (capacity < 2+MAX_SKB_FRAGS) {
I tried a similar patch before, it didn't help much on TCP stream
performance. But I didn't try multiple stream TCP_RR.
Shirley
^ permalink raw reply
* Re: Network performance with small packets - continued
From: Tom Lendacky @ 2011-03-09 16:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Shirley Ma, Rusty Russell, Krishna Kumar2, David Miller, kvm,
netdev, steved
In-Reply-To: <20110309071558.GA25757@redhat.com>
On Wednesday, March 09, 2011 01:15:58 am Michael S. Tsirkin wrote:
> On Mon, Mar 07, 2011 at 04:31:41PM -0600, Tom Lendacky wrote:
> > We've been doing some more experimenting with the small packet network
> > performance problem in KVM. I have a different setup than what Steve D.
> > was using so I re-baselined things on the kvm.git kernel on both the
> > host and guest with a 10GbE adapter. I also made use of the
> > virtio-stats patch.
> >
> > The virtual machine has 2 vCPUs, 8GB of memory and two virtio network
> > adapters (the first connected to a 1GbE adapter and a LAN, the second
> > connected to a 10GbE adapter that is direct connected to another system
> > with the same 10GbE adapter) running the kvm.git kernel. The test was a
> > TCP_RR test with 100 connections from a baremetal client to the KVM
> > guest using a 256 byte message size in both directions.
> >
> > I used the uperf tool to do this after verifying the results against
> > netperf. Uperf allows the specification of the number of connections as
> > a parameter in an XML file as opposed to launching, in this case, 100
> > separate instances of netperf.
> >
> > Here is the baseline for baremetal using 2 physical CPUs:
> > Txn Rate: 206,389.59 Txn/Sec, Pkt Rate: 410,048 Pkts/Sec
> > TxCPU: 7.88% RxCPU: 99.41%
> >
> > To be sure to get consistent results with KVM I disabled the
> > hyperthreads, pinned the qemu-kvm process, vCPUs, vhost thread and
> > ethernet adapter interrupts (this resulted in runs that differed by only
> > about 2% from lowest to highest). The fact that pinning is required to
> > get consistent results is a different problem that we'll have to look
> > into later...
> >
> > Here is the KVM baseline (average of six runs):
> > Txn Rate: 87,070.34 Txn/Sec, Pkt Rate: 172,992 Pkts/Sec
> > Exits: 148,444.58 Exits/Sec
> > TxCPU: 2.40% RxCPU: 99.35%
> >
> > About 42% of baremetal.
>
> Can you add interrupt stats as well please?
Yes I can. Just the guest interrupts for the virtio device?
>
> > empty. So I coded a quick patch to delay freeing of the used Tx buffers
> > until more than half the ring was used (I did not test this under a
> > stream condition so I don't know if this would have a negative impact).
> > Here are the results
> >
> > from delaying the freeing of used Tx buffers (average of six runs):
> > Txn Rate: 90,886.19 Txn/Sec, Pkt Rate: 180,571 Pkts/Sec
> > Exits: 142,681.67 Exits/Sec
> > TxCPU: 2.78% RxCPU: 99.36%
> >
> > About a 4% increase over baseline and about 44% of baremetal.
>
> Hmm, I am not sure what you mean by delaying freeing.
In the start_xmit function of virtio_net.c the first thing done is to free any
used entries from the ring. I patched the code to track the number of used tx
ring entries and only free the used entries when they are greater than half
the capacity of the ring (similar to the way the rx ring is re-filled).
> I think we do have a problem that free_old_xmit_skbs
> tries to flush out the ring aggressively:
> it always polls until the ring is empty,
> so there could be bursts of activity where
> we spend a lot of time flushing the old entries
> before e.g. sending an ack, resulting in
> latency bursts.
>
> Generally we'll need some smarter logic,
> but with indirect at the moment we can just poll
> a single packet after we post a new one, and be done with it.
> Is your patch something like the patch below?
> Could you try mine as well please?
Yes, I'll try the patch and post the results.
>
> > This spread out the kick_notify but still resulted in alot of them. I
> > decided to build on the delayed Tx buffer freeing and code up an
> > "ethtool" like coalescing patch in order to delay the kick_notify until
> > there were at least 5 packets on the ring or 2000 usecs, whichever
> > occurred first. Here are the
> >
> > results of delaying the kick_notify (average of six runs):
> > Txn Rate: 107,106.36 Txn/Sec, Pkt Rate: 212,796 Pkts/Sec
> > Exits: 102,587.28 Exits/Sec
> > TxCPU: 3.03% RxCPU: 99.33%
> >
> > About a 23% increase over baseline and about 52% of baremetal.
> >
> > Running the perf command against the guest I noticed almost 19% of the
> > time being spent in _raw_spin_lock. Enabling lockstat in the guest
> > showed alot of contention in the "irq_desc_lock_class". Pinning the
> > virtio1-input interrupt to a single cpu in the guest and re-running the
> > last test resulted in
> >
> > tremendous gains (average of six runs):
> > Txn Rate: 153,696.59 Txn/Sec, Pkt Rate: 305,358 Pkgs/Sec
> > Exits: 62,603.37 Exits/Sec
> > TxCPU: 3.73% RxCPU: 98.52%
> >
> > About a 77% increase over baseline and about 74% of baremetal.
> >
> > Vhost is receiving a lot of notifications for packets that are to be
> > transmitted (over 60% of the packets generate a kick_notify). Also, it
> > looks like vhost is sending a lot of notifications for packets it has
> > received before the guest can get scheduled to disable notifications and
> > begin processing the packets
>
> Hmm, is this really what happens to you? The effect would be that guest
> gets an interrupt while notifications are disabled in guest, right? Could
> you add a counter and check this please?
The disabling of the interrupt/notifications is done by the guest. So the
guest has to get scheduled and handle the notification before it disables
them. The vhost_signal routine will keep injecting an interrupt until this
happens causing the contention in the guest. I'll try the patches you specify
below and post the results. They look like they should take care of this
issue.
>
> Another possible thing to try would be these old patches to publish used
> index from guest to make sure this double interrupt does not happen:
> [PATCHv2] virtio: put last seen used index into ring itself
> [PATCHv2] vhost-net: utilize PUBLISH_USED_IDX feature
>
> > resulting in some lock contention in the guest (and
> > high interrupt rates).
> >
> > Some thoughts for the transmit path... can vhost be enhanced to do some
> > adaptive polling so that the number of kick_notify events are reduced and
> > replaced by kick_no_notify events?
>
> Worth a try.
>
> > Comparing the transmit path to the receive path, the guest disables
> > notifications after the first kick and vhost re-enables notifications
> > after completing processing of the tx ring.
>
> Is this really what happens? I though the host disables notifications
> after the first kick.
Yup, sorry for the confusion. The kick is done by the guest and then vhost
disables notifications. Maybe a similar approach to the above patches of
checking the used index in the virtio_net driver could also help here?
>
> > Can a similar thing be done for the
> >
> > receive path? Once vhost sends the first notification for a received
> > packet it can disable notifications and let the guest re-enable
> > notifications when it has finished processing the receive ring. Also,
> > can the virtio-net driver do some adaptive polling (or does napi take
> > care of that for the guest)?
>
> Worth a try. I don't think napi does anything like this.
>
> > Running the same workload on the same configuration with a different
> > hypervisor results in performance that is almost equivalent to baremetal
> > without doing any pinning.
> >
> > Thanks,
> > Tom Lendacky
>
> There's no need to flush out all used buffers
> before we post more for transmit: with indirect,
> just a single one is enough. Without indirect we'll
> need more possibly, but just for testing this should
> be enough.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> Note: untested.
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 82dba5a..ebe3337 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -514,11 +514,11 @@ static unsigned int free_old_xmit_skbs(struct
> virtnet_info *vi) struct sk_buff *skb;
> unsigned int len, tot_sgs = 0;
>
> - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> + if ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> pr_debug("Sent skb %p\n", skb);
> vi->dev->stats.tx_bytes += skb->len;
> vi->dev->stats.tx_packets++;
> - tot_sgs += skb_vnet_hdr(skb)->num_sg;
> + tot_sgs = 2+MAX_SKB_FRAGS;
> dev_kfree_skb_any(skb);
> }
> return tot_sgs;
> @@ -576,9 +576,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> struct net_device *dev) struct virtnet_info *vi = netdev_priv(dev);
> int capacity;
>
> - /* Free up any pending old buffers before queueing new ones. */
> - free_old_xmit_skbs(vi);
> -
> /* Try to transmit */
> capacity = xmit_skb(vi, skb);
>
> @@ -605,6 +602,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> struct net_device *dev) skb_orphan(skb);
> nf_reset(skb);
>
> + /* Free up any old buffers so we can queue new ones. */
> + if (capacity < 2+MAX_SKB_FRAGS)
> + capacity += free_old_xmit_skbs(vi);
> +
> /* Apparently nice girls don't return TX_BUSY; stop the queue
> * before it gets out of hand. Naturally, this wastes entries. */
> if (capacity < 2+MAX_SKB_FRAGS) {
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Network performance with small packets - continued
From: Michael S. Tsirkin @ 2011-03-09 16:10 UTC (permalink / raw)
To: Shirley Ma
Cc: Rusty Russell, Krishna Kumar2, David Miller, kvm, netdev, steved,
Tom Lendacky
In-Reply-To: <1299685543.25664.97.camel@localhost.localdomain>
On Wed, Mar 09, 2011 at 07:45:43AM -0800, Shirley Ma wrote:
> On Wed, 2011-03-09 at 09:15 +0200, Michael S. Tsirkin wrote:
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 82dba5a..ebe3337 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -514,11 +514,11 @@ static unsigned int free_old_xmit_skbs(struct
> > virtnet_info *vi)
> > struct sk_buff *skb;
> > unsigned int len, tot_sgs = 0;
> >
> > - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> > + if ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> > pr_debug("Sent skb %p\n", skb);
> > vi->dev->stats.tx_bytes += skb->len;
> > vi->dev->stats.tx_packets++;
> > - tot_sgs += skb_vnet_hdr(skb)->num_sg;
> > + tot_sgs = 2+MAX_SKB_FRAGS;
> > dev_kfree_skb_any(skb);
> > }
> > return tot_sgs;
>
> Return value should be different based on indirect or direct buffers
> here?
Something like that. Or we can assume no indirect, worst-case.
But just for testing, I think it should work as an estimation.
> > @@ -576,9 +576,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> > struct virtnet_info *vi = netdev_priv(dev);
> > int capacity;
> >
> > - /* Free up any pending old buffers before queueing new ones.
> > */
> > - free_old_xmit_skbs(vi);
> > -
> > /* Try to transmit */
> > capacity = xmit_skb(vi, skb);
> >
> > @@ -605,6 +602,10 @@ static netdev_tx_t start_xmit(struct sk_buff
> > *skb, struct net_device *dev)
> > skb_orphan(skb);
> > nf_reset(skb);
> >
> > + /* Free up any old buffers so we can queue new ones. */
> > + if (capacity < 2+MAX_SKB_FRAGS)
> > + capacity += free_old_xmit_skbs(vi);
> > +
> > /* Apparently nice girls don't return TX_BUSY; stop the queue
> > * before it gets out of hand. Naturally, this wastes
> > entries. */
> > if (capacity < 2+MAX_SKB_FRAGS) {
>
> I tried a similar patch before, it didn't help much on TCP stream
> performance. But I didn't try multiple stream TCP_RR.
>
> Shirley
There's a bug in myh patch by the way. Pls try the following
instead (still untested).
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 82dba5a..4477b9a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -514,11 +514,11 @@ static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
struct sk_buff *skb;
unsigned int len, tot_sgs = 0;
- while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
+ if ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
pr_debug("Sent skb %p\n", skb);
vi->dev->stats.tx_bytes += skb->len;
vi->dev->stats.tx_packets++;
- tot_sgs += skb_vnet_hdr(skb)->num_sg;
+ tot_sgs = 2+MAX_SKB_FRAGS;
dev_kfree_skb_any(skb);
}
return tot_sgs;
@@ -576,7 +576,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
struct virtnet_info *vi = netdev_priv(dev);
int capacity;
- /* Free up any pending old buffers before queueing new ones. */
+ /* Free up any old buffers so we can queue new ones. */
free_old_xmit_skbs(vi);
/* Try to transmit */
@@ -605,6 +605,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
skb_orphan(skb);
nf_reset(skb);
+ /* Free up any old buffers so we can queue new ones. */
+ if (capacity < 2+MAX_SKB_FRAGS)
+ capacity += free_old_xmit_skbs(vi);
+
/* Apparently nice girls don't return TX_BUSY; stop the queue
* before it gets out of hand. Naturally, this wastes entries. */
if (capacity < 2+MAX_SKB_FRAGS) {
^ permalink raw reply related
* Re: Network performance with small packets - continued
From: Shirley Ma @ 2011-03-09 16:21 UTC (permalink / raw)
To: Tom Lendacky
Cc: Michael S. Tsirkin, Rusty Russell, Krishna Kumar2, David Miller,
kvm, netdev, steved
In-Reply-To: <201103091009.27932.tahm@linux.vnet.ibm.com>
On Wed, 2011-03-09 at 10:09 -0600, Tom Lendacky wrote:
> >
> > > This spread out the kick_notify but still resulted in alot of
> them. I
> > > decided to build on the delayed Tx buffer freeing and code up an
> > > "ethtool" like coalescing patch in order to delay the kick_notify
> until
> > > there were at least 5 packets on the ring or 2000 usecs, whichever
> > > occurred first. Here are the
> > >
> > > results of delaying the kick_notify (average of six runs):
> > > Txn Rate: 107,106.36 Txn/Sec, Pkt Rate: 212,796 Pkts/Sec
> > > Exits: 102,587.28 Exits/Sec
> > > TxCPU: 3.03% RxCPU: 99.33%
> > >
> > > About a 23% increase over baseline and about 52% of baremetal.
> > >
> > > Running the perf command against the guest I noticed almost 19% of
> the
> > > time being spent in _raw_spin_lock. Enabling lockstat in the
> guest
> > > showed alot of contention in the "irq_desc_lock_class". Pinning
> the
> > > virtio1-input interrupt to a single cpu in the guest and
> re-running the
> > > last test resulted in
> > >
> > > tremendous gains (average of six runs):
> > > Txn Rate: 153,696.59 Txn/Sec, Pkt Rate: 305,358 Pkgs/Sec
> > > Exits: 62,603.37 Exits/Sec
> > > TxCPU: 3.73% RxCPU: 98.52%
> > >
> > > About a 77% increase over baseline and about 74% of baremetal.
> > >
> > > Vhost is receiving a lot of notifications for packets that are to
> be
> > > transmitted (over 60% of the packets generate a kick_notify).
> Also, it
> > > looks like vhost is sending a lot of notifications for packets it
> has
> > > received before the guest can get scheduled to disable
> notifications and
> > > begin processing the packets
> >
> > Hmm, is this really what happens to you? The effect would be that
> guest
> > gets an interrupt while notifications are disabled in guest, right?
> Could
> > you add a counter and check this please?
>
> The disabling of the interrupt/notifications is done by the guest. So
> the
> guest has to get scheduled and handle the notification before it
> disables
> them. The vhost_signal routine will keep injecting an interrupt until
> this
> happens causing the contention in the guest. I'll try the patches you
> specify
> below and post the results. They look like they should take care of
> this
> issue.
In guest TX path, the guest interrupt should be disabled in the start
since it free_old_xmit_skbs in start_xmit call, it's not necessary to
receive any send completion interrupts to handle free old skbs. Then the
interrupt is only enabled when the netif queue is full. For multiple
streams TCP_RR test, we never hit netif queue full situation, the
cat /proc/interrupts/ send completion interrupts rate is 0, right?
Shirley
^ permalink raw reply
* Re: Network performance with small packets - continued
From: Shirley Ma @ 2011-03-09 16:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, Krishna Kumar2, David Miller, kvm, netdev, steved,
Tom Lendacky
In-Reply-To: <20110309161013.GA7165@redhat.com>
On Wed, 2011-03-09 at 18:10 +0200, Michael S. Tsirkin wrote:
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 82dba5a..4477b9a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -514,11 +514,11 @@ static unsigned int free_old_xmit_skbs(struct
> virtnet_info *vi)
> struct sk_buff *skb;
> unsigned int len, tot_sgs = 0;
>
> - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> + if ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> pr_debug("Sent skb %p\n", skb);
> vi->dev->stats.tx_bytes += skb->len;
> vi->dev->stats.tx_packets++;
> - tot_sgs += skb_vnet_hdr(skb)->num_sg;
> + tot_sgs = 2+MAX_SKB_FRAGS;
> dev_kfree_skb_any(skb);
> }
> return tot_sgs;
> @@ -576,7 +576,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> struct virtnet_info *vi = netdev_priv(dev);
> int capacity;
>
> - /* Free up any pending old buffers before queueing new ones.
> */
> + /* Free up any old buffers so we can queue new ones. */
> free_old_xmit_skbs(vi);
>
> /* Try to transmit */
> @@ -605,6 +605,10 @@ static netdev_tx_t start_xmit(struct sk_buff
> *skb, struct net_device *dev)
> skb_orphan(skb);
> nf_reset(skb);
>
> + /* Free up any old buffers so we can queue new ones. */
> + if (capacity < 2+MAX_SKB_FRAGS)
> + capacity += free_old_xmit_skbs(vi);
> +
> /* Apparently nice girls don't return TX_BUSY; stop the queue
> * before it gets out of hand. Naturally, this wastes
> entries. */
> if (capacity < 2+MAX_SKB_FRAGS) {
> --
I tried this one as well. It might improve TCP_RR performance but not
TCP_STREAM. :) Let's wait for Tom's TCP_RR resutls.
Thanks
Shirley
^ permalink raw reply
* Re: Network performance with small packets - continued
From: Michael S. Tsirkin @ 2011-03-09 16:28 UTC (permalink / raw)
To: Tom Lendacky
Cc: Shirley Ma, Rusty Russell, Krishna Kumar2, David Miller, kvm,
netdev, steved
In-Reply-To: <201103091009.27932.tahm@linux.vnet.ibm.com>
On Wed, Mar 09, 2011 at 10:09:26AM -0600, Tom Lendacky wrote:
> On Wednesday, March 09, 2011 01:15:58 am Michael S. Tsirkin wrote:
> > On Mon, Mar 07, 2011 at 04:31:41PM -0600, Tom Lendacky wrote:
> > > We've been doing some more experimenting with the small packet network
> > > performance problem in KVM. I have a different setup than what Steve D.
> > > was using so I re-baselined things on the kvm.git kernel on both the
> > > host and guest with a 10GbE adapter. I also made use of the
> > > virtio-stats patch.
> > >
> > > The virtual machine has 2 vCPUs, 8GB of memory and two virtio network
> > > adapters (the first connected to a 1GbE adapter and a LAN, the second
> > > connected to a 10GbE adapter that is direct connected to another system
> > > with the same 10GbE adapter) running the kvm.git kernel. The test was a
> > > TCP_RR test with 100 connections from a baremetal client to the KVM
> > > guest using a 256 byte message size in both directions.
One thing that might be happening is that we are out of
atomic memory poll in guest, so indirect allocations
start failing, and this is slow path.
Could you check this please?
> > > I used the uperf tool to do this after verifying the results against
> > > netperf. Uperf allows the specification of the number of connections as
> > > a parameter in an XML file as opposed to launching, in this case, 100
> > > separate instances of netperf.
> > >
> > > Here is the baseline for baremetal using 2 physical CPUs:
> > > Txn Rate: 206,389.59 Txn/Sec, Pkt Rate: 410,048 Pkts/Sec
> > > TxCPU: 7.88% RxCPU: 99.41%
> > >
> > > To be sure to get consistent results with KVM I disabled the
> > > hyperthreads, pinned the qemu-kvm process, vCPUs, vhost thread and
> > > ethernet adapter interrupts (this resulted in runs that differed by only
> > > about 2% from lowest to highest). The fact that pinning is required to
> > > get consistent results is a different problem that we'll have to look
> > > into later...
> > >
> > > Here is the KVM baseline (average of six runs):
> > > Txn Rate: 87,070.34 Txn/Sec, Pkt Rate: 172,992 Pkts/Sec
> > > Exits: 148,444.58 Exits/Sec
> > > TxCPU: 2.40% RxCPU: 99.35%
> > >
> > > About 42% of baremetal.
> >
> > Can you add interrupt stats as well please?
>
> Yes I can. Just the guest interrupts for the virtio device?
Guess so: tx and rx.
> >
> > > empty. So I coded a quick patch to delay freeing of the used Tx buffers
> > > until more than half the ring was used (I did not test this under a
> > > stream condition so I don't know if this would have a negative impact).
> > > Here are the results
> > >
> > > from delaying the freeing of used Tx buffers (average of six runs):
> > > Txn Rate: 90,886.19 Txn/Sec, Pkt Rate: 180,571 Pkts/Sec
> > > Exits: 142,681.67 Exits/Sec
> > > TxCPU: 2.78% RxCPU: 99.36%
> > >
> > > About a 4% increase over baseline and about 44% of baremetal.
> >
> > Hmm, I am not sure what you mean by delaying freeing.
>
> In the start_xmit function of virtio_net.c the first thing done is to free any
> used entries from the ring. I patched the code to track the number of used tx
> ring entries and only free the used entries when they are greater than half
> the capacity of the ring (similar to the way the rx ring is re-filled).
We don't even need than: just max skb frag + 2.
Also don't need to free them all: just enough to get
place for max skb frag + 2 entries.
> > I think we do have a problem that free_old_xmit_skbs
> > tries to flush out the ring aggressively:
> > it always polls until the ring is empty,
> > so there could be bursts of activity where
> > we spend a lot of time flushing the old entries
> > before e.g. sending an ack, resulting in
> > latency bursts.
> >
> > Generally we'll need some smarter logic,
> > but with indirect at the moment we can just poll
> > a single packet after we post a new one, and be done with it.
> > Is your patch something like the patch below?
> > Could you try mine as well please?
>
> Yes, I'll try the patch and post the results.
>
> >
> > > This spread out the kick_notify but still resulted in alot of them. I
> > > decided to build on the delayed Tx buffer freeing and code up an
> > > "ethtool" like coalescing patch in order to delay the kick_notify until
> > > there were at least 5 packets on the ring or 2000 usecs, whichever
> > > occurred first. Here are the
> > >
> > > results of delaying the kick_notify (average of six runs):
> > > Txn Rate: 107,106.36 Txn/Sec, Pkt Rate: 212,796 Pkts/Sec
> > > Exits: 102,587.28 Exits/Sec
> > > TxCPU: 3.03% RxCPU: 99.33%
> > >
> > > About a 23% increase over baseline and about 52% of baremetal.
> > >
> > > Running the perf command against the guest I noticed almost 19% of the
> > > time being spent in _raw_spin_lock. Enabling lockstat in the guest
> > > showed alot of contention in the "irq_desc_lock_class". Pinning the
> > > virtio1-input interrupt to a single cpu in the guest and re-running the
> > > last test resulted in
> > >
> > > tremendous gains (average of six runs):
> > > Txn Rate: 153,696.59 Txn/Sec, Pkt Rate: 305,358 Pkgs/Sec
> > > Exits: 62,603.37 Exits/Sec
> > > TxCPU: 3.73% RxCPU: 98.52%
> > >
> > > About a 77% increase over baseline and about 74% of baremetal.
> > >
> > > Vhost is receiving a lot of notifications for packets that are to be
> > > transmitted (over 60% of the packets generate a kick_notify). Also, it
> > > looks like vhost is sending a lot of notifications for packets it has
> > > received before the guest can get scheduled to disable notifications and
> > > begin processing the packets
> >
> > Hmm, is this really what happens to you? The effect would be that guest
> > gets an interrupt while notifications are disabled in guest, right? Could
> > you add a counter and check this please?
>
> The disabling of the interrupt/notifications is done by the guest. So the
> guest has to get scheduled and handle the notification before it disables
> them. The vhost_signal routine will keep injecting an interrupt until this
> happens causing the contention in the guest. I'll try the patches you specify
> below and post the results. They look like they should take care of this
> issue.
>
> >
> > Another possible thing to try would be these old patches to publish used
> > index from guest to make sure this double interrupt does not happen:
> > [PATCHv2] virtio: put last seen used index into ring itself
> > [PATCHv2] vhost-net: utilize PUBLISH_USED_IDX feature
> >
> > > resulting in some lock contention in the guest (and
> > > high interrupt rates).
> > >
> > > Some thoughts for the transmit path... can vhost be enhanced to do some
> > > adaptive polling so that the number of kick_notify events are reduced and
> > > replaced by kick_no_notify events?
> >
> > Worth a try.
> >
> > > Comparing the transmit path to the receive path, the guest disables
> > > notifications after the first kick and vhost re-enables notifications
> > > after completing processing of the tx ring.
> >
> > Is this really what happens? I though the host disables notifications
> > after the first kick.
>
> Yup, sorry for the confusion. The kick is done by the guest and then vhost
> disables notifications. Maybe a similar approach to the above patches of
> checking the used index in the virtio_net driver could also help here?
If this happens there will be many kicks that find an empty ring.
Could you add a counter in vhost and check please?
--
MST
^ permalink raw reply
* Re: Network performance with small packets - continued
From: Michael S. Tsirkin @ 2011-03-09 16:32 UTC (permalink / raw)
To: Shirley Ma
Cc: Rusty Russell, Krishna Kumar2, David Miller, kvm, netdev, steved,
Tom Lendacky
In-Reply-To: <1299687934.25664.108.camel@localhost.localdomain>
On Wed, Mar 09, 2011 at 08:25:34AM -0800, Shirley Ma wrote:
> On Wed, 2011-03-09 at 18:10 +0200, Michael S. Tsirkin wrote:
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 82dba5a..4477b9a 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -514,11 +514,11 @@ static unsigned int free_old_xmit_skbs(struct
> > virtnet_info *vi)
> > struct sk_buff *skb;
> > unsigned int len, tot_sgs = 0;
> >
> > - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> > + if ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> > pr_debug("Sent skb %p\n", skb);
> > vi->dev->stats.tx_bytes += skb->len;
> > vi->dev->stats.tx_packets++;
> > - tot_sgs += skb_vnet_hdr(skb)->num_sg;
> > + tot_sgs = 2+MAX_SKB_FRAGS;
> > dev_kfree_skb_any(skb);
> > }
> > return tot_sgs;
> > @@ -576,7 +576,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> > struct virtnet_info *vi = netdev_priv(dev);
> > int capacity;
> >
> > - /* Free up any pending old buffers before queueing new ones.
> > */
> > + /* Free up any old buffers so we can queue new ones. */
> > free_old_xmit_skbs(vi);
> >
> > /* Try to transmit */
> > @@ -605,6 +605,10 @@ static netdev_tx_t start_xmit(struct sk_buff
> > *skb, struct net_device *dev)
> > skb_orphan(skb);
> > nf_reset(skb);
> >
> > + /* Free up any old buffers so we can queue new ones. */
> > + if (capacity < 2+MAX_SKB_FRAGS)
> > + capacity += free_old_xmit_skbs(vi);
> > +
> > /* Apparently nice girls don't return TX_BUSY; stop the queue
> > * before it gets out of hand. Naturally, this wastes
> > entries. */
> > if (capacity < 2+MAX_SKB_FRAGS) {
> > --
>
> I tried this one as well. It might improve TCP_RR performance but not
> TCP_STREAM. :) Let's wait for Tom's TCP_RR resutls.
>
> Thanks
> Shirley
I think your issues are with TX overrun.
Besides delaying IRQ on TX, I don't have many ideas.
The one interesting thing is that you see better speed
if you drop packets. netdev crowd says this should not happen,
so could be an indicator of a problem somewhere.
--
MST
^ permalink raw reply
* Re: Network performance with small packets - continued
From: Shirley Ma @ 2011-03-09 16:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, Krishna Kumar2, David Miller, kvm, netdev, steved,
Tom Lendacky
In-Reply-To: <20110309163201.GC7165@redhat.com>
On Wed, 2011-03-09 at 18:32 +0200, Michael S. Tsirkin wrote:
> I think your issues are with TX overrun.
> Besides delaying IRQ on TX, I don't have many ideas.
>
> The one interesting thing is that you see better speed
> if you drop packets. netdev crowd says this should not happen,
> so could be an indicator of a problem somewhere.
Yes, I am looking at why guest didn't see see used_buffers on time from
vhost send TX completion I am trying to collect some data on vhost.
I also wonder whether it's a scheduler issue.
Thanks
Shirley
^ permalink raw reply
* Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state
From: Adam Majer @ 2011-03-09 16:44 UTC (permalink / raw)
To: Américo Wang; +Cc: netdev, Stephen Hemminger, bridge, linux-kernel
In-Reply-To: <20110309150949.GE16951@cr0.redhat.com>
On Wed, Mar 09, 2011 at 11:09:49PM +0800, Américo Wang wrote:
> On Sat, Mar 05, 2011 at 11:18:33PM -0600, Adam Majer wrote:
> >
> >IPv6 address autoconfiguration relies on an UP interface to processes
> >traffic normally. A bridge that is UP enters LEARNING state and this
> >state "eats" any Router Advertising packets sent to the
> >bridge. Issuing a NETDEV_CHANGE notification on the bridge interface
> >when it changes state allows autoconfiguration code to retry
> >querying router information.
> >
> ...
>
> >+static void br_port_change_notifier_handler(struct work_struct *work)
> >+{
> >+ struct net_bridge *br = container_of(work,
> >+ struct net_bridge,
> >+ change_notification_worker);
> >+
> >+ rtnl_lock();
> >+ netdev_state_change(br->dev);
> >+ rtnl_unlock();
> >+}
> >+
>
> Do you really want user-space to get this notification too?
> Why do you put it into a workqueue? Maybe it has to be called in
> process-context?
Stephen Hemminger's patch (posted as a reply in this thread but only
on the netdev mailing list) is much better as it only brings the link
up on the bridge interface when the bridge enters forwarding mode.
Effectively it does similar thing via NETDEV_CHANGE but requires no
other hacks in other code. It should make userland much happier too.
As for my patch, I've put that in the workqueue because the
notification cannot be called in interrupt context. Is there a better
way of calling code that can sleep from interrupt context?
- Adam
--
Adam Majer
adamm@zombino.com
^ 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