* [PATCH 2/3] Expose may_setuid() in user.h and add may_setgid() (v2)
From: Dan Smith @ 2009-08-18 19:57 UTC (permalink / raw)
To: orenl; +Cc: containers, netdev, Serge Hallyn
In-Reply-To: <1250625435-16299-1-git-send-email-danms@us.ibm.com>
Make these helpers available to others.
Changes in v2:
- Avoid checking the groupinfo in ctx->realcred against the current in
may_setgid()
Cc: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: Dan Smith <danms@us.ibm.com>
---
include/linux/user.h | 9 +++++++++
kernel/user.c | 13 ++++++++++++-
2 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/include/linux/user.h b/include/linux/user.h
index 68daf84..c231e9c 100644
--- a/include/linux/user.h
+++ b/include/linux/user.h
@@ -1 +1,10 @@
+#ifndef _LINUX_USER_H
+#define _LINUX_USER_H
+
#include <asm/user.h>
+#include <linux/sched.h>
+
+extern int may_setuid(struct user_namespace *ns, uid_t uid);
+extern int may_setgid(gid_t gid);
+
+#endif
diff --git a/kernel/user.c b/kernel/user.c
index a535ed6..a78fde7 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -604,7 +604,7 @@ int checkpoint_user(struct ckpt_ctx *ctx, void *ptr)
return do_checkpoint_user(ctx, (struct user_struct *) ptr);
}
-static int may_setuid(struct user_namespace *ns, uid_t uid)
+int may_setuid(struct user_namespace *ns, uid_t uid)
{
/*
* this next check will one day become
@@ -631,6 +631,17 @@ static int may_setuid(struct user_namespace *ns, uid_t uid)
return 0;
}
+int may_setgid(gid_t gid)
+{
+ if (capable(CAP_SETGID))
+ return 1;
+
+ if (in_egroup_p(gid))
+ return 1;
+
+ return 0;
+}
+
static struct user_struct *do_restore_user(struct ckpt_ctx *ctx)
{
struct user_struct *u;
--
1.6.2.5
^ permalink raw reply related
* [PATCH 3/3] Save and restore UNIX socket peer credentials (v2)
From: Dan Smith @ 2009-08-18 19:57 UTC (permalink / raw)
To: orenl; +Cc: containers, netdev
In-Reply-To: <1250625435-16299-1-git-send-email-danms@us.ibm.com>
This saves the uid/gid of the sk_peercred structure in the checkpoint
stream. On restart, it uses may_setuid() and may_setgid() to determine
if the uid/gid from the checkpoint stream may be used.
Changes in v2:
- Adjust for may_setgid() change
Signed-off-by: Dan Smith <danms@us.ibm.com>
---
include/linux/checkpoint_hdr.h | 2 ++
net/unix/checkpoint.c | 29 ++++++++++++++++-------------
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
index 4d5c22a..78f1f27 100644
--- a/include/linux/checkpoint_hdr.h
+++ b/include/linux/checkpoint_hdr.h
@@ -414,6 +414,8 @@ struct ckpt_hdr_socket_unix {
struct ckpt_hdr h;
__s32 this;
__s32 peer;
+ __u32 peercred_uid;
+ __u32 peercred_gid;
__u32 flags;
__u32 laddr_len;
__u32 raddr_len;
diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
index 81252e3..366bc80 100644
--- a/net/unix/checkpoint.c
+++ b/net/unix/checkpoint.c
@@ -3,6 +3,7 @@
#include <linux/fs_struct.h>
#include <linux/checkpoint.h>
#include <linux/checkpoint_hdr.h>
+#include <linux/user.h>
#include <net/af_unix.h>
#include <net/tcp_states.h>
@@ -94,6 +95,9 @@ int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock)
goto out;
}
+ un->peercred_uid = sock->sk->sk_peercred.uid;
+ un->peercred_gid = sock->sk->sk_peercred.gid;
+
ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) un);
if (ret < 0)
goto out;
@@ -217,19 +221,6 @@ static int unix_join(struct ckpt_ctx *ctx,
unix_sk(a)->peer = b;
unix_sk(b)->peer = a;
- /* TODO:
- * Checkpoint the credentials, restore them here if the values match
- * the restored creds or we may_setuid()
- */
-
- a->sk_peercred.pid = task_tgid_vnr(current);
- a->sk_peercred.uid = ctx->realcred->uid;
- a->sk_peercred.gid = ctx->realcred->gid;
-
- b->sk_peercred.pid = a->sk_peercred.pid;
- b->sk_peercred.uid = a->sk_peercred.uid;
- b->sk_peercred.gid = a->sk_peercred.gid;
-
if (!UNIX_ADDR_EMPTY(un->raddr_len))
addr = unix_makeaddr(&un->raddr, un->raddr_len);
else if (!UNIX_ADDR_EMPTY(un->laddr_len))
@@ -295,6 +286,18 @@ static int unix_restore_connected(struct ckpt_ctx *ctx,
goto out;
}
+ this->sk_peercred.pid = task_tgid_vnr(current);
+
+ if (may_setuid(ctx->realcred->user->user_ns, un->peercred_uid) &&
+ may_setgid(un->peercred_gid)) {
+ this->sk_peercred.uid = un->peercred_uid;
+ this->sk_peercred.gid = un->peercred_gid;
+ } else {
+ ckpt_debug("peercred %i:%i would require setuid",
+ un->peercred_uid, un->peercred_gid);
+ return -1;
+ }
+
/* Prime the socket's buffer limit with the maximum. These will be
* overwritten with the values in the checkpoint stream in a later
* phase.
--
1.6.2.5
^ permalink raw reply related
* [PATCH 1/3] Set socket flags on restore using sock_setsockopt() where possible (v2)
From: Dan Smith @ 2009-08-18 19:57 UTC (permalink / raw)
To: orenl; +Cc: containers, netdev
In-Reply-To: <1250625435-16299-1-git-send-email-danms@us.ibm.com>
Fail on the TIMESTAMPING_* flags for the moment, with a TODO in place to
handle them later.
Also remove other explicit flag checks because they're no longer copied
blindly into the socket object, so existing checks will be sufficient.
Changes in v2:
- Avoid removing the sock->sk_socket check before sync'ing the socket.flags
- Rename sock_rst_flags() to sock_restore_flags()
- Rebase on top of Oren's cleanup patch
Acked-by: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: Dan Smith <danms@us.ibm.com>
---
net/checkpoint.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 102 insertions(+), 11 deletions(-)
diff --git a/net/checkpoint.c b/net/checkpoint.c
index c64483e..fdbf8e7 100644
--- a/net/checkpoint.c
+++ b/net/checkpoint.c
@@ -178,10 +178,6 @@ static int sock_cptrst_verify(struct ckpt_hdr_socket *h)
if (!ckpt_validate_errno(h->sock.err))
return -EINVAL;
- /* None of our supported types use this flag */
- if (h->sock.flags & SOCK_DESTROY)
- return -EINVAL;
-
return 0;
}
@@ -238,11 +234,97 @@ static int sock_cptrst_bufopts(int op, struct sock *sk,
return 0;
}
+static int sock_restore_flags(struct socket *sock,
+ struct ckpt_hdr_socket *h)
+{
+ int ret;
+ int v = 1;
+ unsigned long sk_flags = h->sock.flags;
+ unsigned long sock_flags = h->socket.flags;
+
+ if (test_and_clear_bit(SOCK_URGINLINE, &sk_flags)) {
+ ret = sock_setsockopt(sock, SOL_SOCKET, SO_OOBINLINE,
+ (char *)&v, sizeof(v));
+ if (ret)
+ return ret;
+ }
+
+ if (test_and_clear_bit(SOCK_KEEPOPEN, &sk_flags)) {
+ ret = sock_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
+ (char *)&v, sizeof(v));
+ if (ret)
+ return ret;
+ }
+
+ if (test_and_clear_bit(SOCK_BROADCAST, &sk_flags)) {
+ ret = sock_setsockopt(sock, SOL_SOCKET, SO_BROADCAST,
+ (char *)&v, sizeof(v));
+ if (ret)
+ return ret;
+ }
+
+ if (test_and_clear_bit(SOCK_RCVTSTAMP, &sk_flags)) {
+ ret = sock_setsockopt(sock, SOL_SOCKET, SO_TIMESTAMP,
+ (char *)&v, sizeof(v));
+ if (ret)
+ return ret;
+ }
+
+ if (test_and_clear_bit(SOCK_RCVTSTAMPNS, &sk_flags)) {
+ ret = sock_setsockopt(sock, SOL_SOCKET, SO_TIMESTAMPNS,
+ (char *)&v, sizeof(v));
+ if (ret)
+ return ret;
+ }
+
+ if (test_and_clear_bit(SOCK_DBG, &sk_flags)) {
+ ret = sock_setsockopt(sock, SOL_SOCKET, SO_DEBUG,
+ (char *)&v, sizeof(v));
+ if (ret)
+ return ret;
+ }
+
+ if (test_and_clear_bit(SOCK_LOCALROUTE, &sk_flags)) {
+ ret = sock_setsockopt(sock, SOL_SOCKET, SO_DONTROUTE,
+ (char *)&v, sizeof(v));
+ if (ret)
+ return ret;
+ }
+
+ if (test_and_clear_bit(SOCK_PASSCRED, &sock_flags)) {
+ ret = sock_setsockopt(sock, SOL_SOCKET, SO_PASSCRED,
+ (char *)&v, sizeof(v));
+ if (ret)
+ return ret;
+ }
+
+ /* TODO: Handle SOCK_TIMESTAMPING_* flags */
+ if (test_bit(SOCK_TIMESTAMPING_TX_HARDWARE, &sk_flags) ||
+ test_bit(SOCK_TIMESTAMPING_TX_SOFTWARE, &sk_flags) ||
+ test_bit(SOCK_TIMESTAMPING_RX_HARDWARE, &sk_flags) ||
+ test_bit(SOCK_TIMESTAMPING_RX_SOFTWARE, &sk_flags) ||
+ test_bit(SOCK_TIMESTAMPING_SOFTWARE, &sk_flags) ||
+ test_bit(SOCK_TIMESTAMPING_RAW_HARDWARE, &sk_flags) ||
+ test_bit(SOCK_TIMESTAMPING_SYS_HARDWARE, &sk_flags)) {
+ ckpt_debug("SOF_TIMESTAMPING_* flags are not supported\n");
+ return -ENOSYS;
+ }
+
+ /* Anything that is still set in the flags that isn't part of
+ * our protocol's default set, indicates an error
+ */
+ if (sk_flags & ~sock->sk->sk_flags) {
+ ckpt_debug("Unhandled sock flags: %lx\n", sk_flags);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
struct ckpt_hdr_socket *h, int op)
{
if (sk->sk_socket) {
- CKPT_COPY(op, h->socket.flags, sk->sk_socket->flags);
CKPT_COPY(op, h->socket.state, sk->sk_socket->state);
}
@@ -259,12 +341,6 @@ static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
CKPT_COPY(op, h->sock.state, sk->sk_state);
CKPT_COPY(op, h->sock.backlog, sk->sk_max_ack_backlog);
- /* TODO:
- * Break out setting each of the flags to use setsockopt() or
- * perform proper security check
- */
- CKPT_COPY(op, h->sock.flags, sk->sk_flags);
-
if (sock_cptrst_bufopts(op, sk, h))
return -EINVAL;
@@ -298,6 +374,21 @@ static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
return -EINVAL;
}
+ if (op == CKPT_CPT) {
+ h->sock.flags = sk->sk_flags;
+ h->socket.flags = sk->sk_socket->flags;
+ } else {
+ int ret;
+ mm_segment_t old_fs;
+
+ old_fs = get_fs();
+ set_fs(KERNEL_DS);
+ ret = sock_restore_flags(sk->sk_socket, h);
+ set_fs(old_fs);
+ if (ret)
+ return ret;
+ }
+
if ((h->socket.state == SS_CONNECTED) &&
(h->sock.state != TCP_ESTABLISHED)) {
ckpt_debug("socket/sock in inconsistent state: %i/%i",
--
1.6.2.5
^ permalink raw reply related
* Socket C/R additional features
From: Dan Smith @ 2009-08-18 19:57 UTC (permalink / raw)
To: orenl; +Cc: containers, netdev
This is an updated version of my previous patch set, with the fourth patch
removed. This is rebased on current v17-dev and includes Serge's
recommended changes to may_setgid().
^ permalink raw reply
* Re: bad nat connection tracking performance with ip_gre
From: Timo Teräs @ 2009-08-18 19:36 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netfilter-devel, netdev
In-Reply-To: <4A8AE76D.7040707@iki.fi>
Timo Teräs wrote:
> Yes. But my observation was that for the same amount of packets
> sent locally the CPU usage is significantly higher than if they
> are forwarded from physical interface. That's what made me
> curious.
>
> If I had remember that icmp conn track entries get pruned right
> when they get icmp reply back, I would not have probably bothered
> to bug you. But that made me think it was more of generic problem
> than my patch.
>
> I'll also double check with oprofile the local sendto() approach
> where it dies.
Ok, finally figured out the difference. Looks like depending
on the sendto() / local route / forward route / my patched mrt
the skb that gets passed to ipgre_tunnel_xmit() seems to have
nfctinfo either 0 or 2. This value is not modified; nf_reset()
is called just before ip_local_out(). Looks like nf_reset()
clears nfct to NULL, but does not touch nfctinfo.
So when LOCAL_OUT hook for the GRE packet is hit, depending
where the packet came: it has nfct=NULL and nfctinfo=ESTABLISHED
or NEW. This also seems to affect if that specific skb gets
the nat/OUTPUT hook called.
Is this behaviour for nf_reset() intentional?
- Timo
^ permalink raw reply
* Re: [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Avi Kivity @ 2009-08-18 19:08 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Gregory Haskins, Ingo Molnar, kvm, alacrityvm-devel, linux-kernel,
netdev, Michael S. Tsirkin
In-Reply-To: <200908182020.11126.arnd@arndb.de>
On 08/18/2009 09:20 PM, Arnd Bergmann wrote:
>> Well, the interrupt model to name one.
>>
> The performance aspects of your interrupt model are independent
> of the vbus proxy, or at least they should be. Let's assume for
> now that your event notification mechanism gives significant
> performance improvements (which we can't measure independently
> right now). I don't see a reason why we could not get the
> same performance out of a paravirtual interrupt controller
> that uses the same method, and it would be straightforward
> to implement one and use that together with all the existing
> emulated PCI devices and virtio devices including vhost_net.
>
Interesting. You could even configure those vectors using the standard
MSI configuration mechanism; simply replace the address/data pair with
something meaningful to the paravirt interrupt controller.
I'd have to see really hard numbers to be tempted to merge something
like this though. We've merged paravirt mmu, for example, and now it
underperforms both hardware two-level paging and software shadow paging.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Avi Kivity @ 2009-08-18 18:52 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <20090818182735.GD17631@ovro.caltech.edu>
On 08/18/2009 09:27 PM, Ira W. Snyder wrote:
>> I think in this case you want one side to be virtio-net (I'm guessing
>> the x86) and the other side vhost-net (the ppc boards with the dma
>> engine). virtio-net on x86 would communicate with userspace on the ppc
>> board to negotiate features and get a mac address, the fast path would
>> be between virtio-net and vhost-net (which would use the dma engine to
>> push and pull data).
>>
>>
>
> Ah, that seems backwards, but it should work after vhost-net learns how
> to use the DMAEngine API.
>
> I haven't studied vhost-net very carefully yet. As soon as I saw the
> copy_(to|from)_user() I stopped reading, because it seemed useless for
> my case. I'll look again and try to find where vhost-net supports
> setting MAC addresses and other features.
>
It doesn't; all it does is pump the rings, leaving everything else to
userspace.
> Also, in my case I'd like to boot Linux with my rootfs over NFS. Is
> vhost-net capable of this?
>
It's just another network interface. You'd need an initramfs though to
contain the needed userspace.
> I've had Arnd, BenH, and Grant Likely (and others, privately) contact me
> about devices they are working with that would benefit from something
> like virtio-over-PCI. I'd like to see vhost-net be merged with the
> capability to support my use case. There are plenty of others that would
> benefit, not just myself.
>
> I'm not sure vhost-net is being written with this kind of future use in
> mind. I'd hate to see it get merged, and then have to change the ABI to
> support physical-device-to-device usage. It would be better to keep
> future use in mind now, rather than try and hack it in later.
>
Please review and comment then. I'm fairly confident there won't be any
ABI issues since vhost-net does so little outside pumping the rings.
Note the signalling paths go through eventfd: when vhost-net wants the
other side to look at its ring, it tickles an eventfd which is supposed
to trigger an interrupt on the other side. Conversely, when another
eventfd is signalled, vhost-net will look at the ring and process any
data there. You'll need to wire your signalling to those eventfds,
either in userspace or in the kernel.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Ira W. Snyder @ 2009-08-18 18:27 UTC (permalink / raw)
To: Avi Kivity
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <4A8AE918.5000109@redhat.com>
On Tue, Aug 18, 2009 at 08:47:04PM +0300, Avi Kivity wrote:
> On 08/18/2009 08:27 PM, Ira W. Snyder wrote:
>>> In fact, modern x86s do have dma engines these days (google for Intel
>>> I/OAT), and one of our plans for vhost-net is to allow their use for
>>> packets above a certain size. So a patch allowing vhost-net to
>>> optionally use a dma engine is a good thing.
>>>
>> Yes, I'm aware that very modern x86 PCs have general purpose DMA
>> engines, even though I don't have any capable hardware. However, I think
>> it is better to support using any PC (with or without DMA engine, any
>> architecture) as the PCI master, and just handle the DMA all from the
>> PCI agent, which is known to have DMA?
>>
>
> Certainly; but if your PCI agent will support the DMA API, then the same
> vhost code will work with both I/OAT and your specialized hardware.
>
Yes, that's true. My ppc is a Freescale MPC8349EMDS. It has a Linux
DMAEngine driver in mainline, which I've used. That's excellent.
>>> Exposing a knob to userspace is not an insurmountable problem; vhost-net
>>> already allows changing the memory layout, for example.
>>>
>>>
>> Let me explain the most obvious problem I ran into: setting the MAC
>> addresses used in virtio.
>>
>> On the host (PCI master), I want eth0 (virtio-net) to get a random MAC
>> address.
>>
>> On the guest (PCI agent), I want eth0 (virtio-net) to get a specific MAC
>> address, aa:bb:cc:dd:ee:ff.
>>
>> The virtio feature negotiation code handles this, by seeing the
>> VIRTIO_NET_F_MAC feature in it's configuration space. If BOTH drivers do
>> not have VIRTIO_NET_F_MAC set, then NEITHER will use the specified MAC
>> address. This is because the feature negotiation code only accepts a
>> feature if it is offered by both sides of the connection.
>>
>> In this case, I must have the guest generate a random MAC address and
>> have the host put aa:bb:cc:dd:ee:ff into the guest's configuration
>> space. This basically means hardcoding the MAC addresses in the Linux
>> drivers, which is a big no-no.
>>
>> What would I expose to userspace to make this situation manageable?
>>
>>
>
> I think in this case you want one side to be virtio-net (I'm guessing
> the x86) and the other side vhost-net (the ppc boards with the dma
> engine). virtio-net on x86 would communicate with userspace on the ppc
> board to negotiate features and get a mac address, the fast path would
> be between virtio-net and vhost-net (which would use the dma engine to
> push and pull data).
>
Ah, that seems backwards, but it should work after vhost-net learns how
to use the DMAEngine API.
I haven't studied vhost-net very carefully yet. As soon as I saw the
copy_(to|from)_user() I stopped reading, because it seemed useless for
my case. I'll look again and try to find where vhost-net supports
setting MAC addresses and other features.
Also, in my case I'd like to boot Linux with my rootfs over NFS. Is
vhost-net capable of this?
I've had Arnd, BenH, and Grant Likely (and others, privately) contact me
about devices they are working with that would benefit from something
like virtio-over-PCI. I'd like to see vhost-net be merged with the
capability to support my use case. There are plenty of others that would
benefit, not just myself.
I'm not sure vhost-net is being written with this kind of future use in
mind. I'd hate to see it get merged, and then have to change the ABI to
support physical-device-to-device usage. It would be better to keep
future use in mind now, rather than try and hack it in later.
Thanks for the comments.
Ira
^ permalink raw reply
* Re: [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Arnd Bergmann @ 2009-08-18 18:20 UTC (permalink / raw)
To: Gregory Haskins
Cc: Avi Kivity, Ingo Molnar, kvm, alacrityvm-devel, linux-kernel,
netdev, Michael S. Tsirkin
In-Reply-To: <4A8ABEC9.6090006@gmail.com>
On Tuesday 18 August 2009, Gregory Haskins wrote:
> Avi Kivity wrote:
> > On 08/17/2009 10:33 PM, Gregory Haskins wrote:
> >
> > One point of contention is that this is all managementy stuff and should
> > be kept out of the host kernel. Exposing shared memory, interrupts, and
> > guest hypercalls can all be easily done from userspace (as virtio
> > demonstrates). True, some devices need kernel acceleration, but that's
> > no reason to put everything into the host kernel.
>
> See my last reply to Anthony. My two points here are that:
>
> a) having it in-kernel makes it a complete subsystem, which perhaps has
> diminished value in kvm, but adds value in most other places that we are
> looking to use vbus.
>
> b) the in-kernel code is being overstated as "complex". We are not
> talking about your typical virt thing, like an emulated ICH/PCI chipset.
> Its really a simple list of devices with a handful of attributes. They
> are managed using established linux interfaces, like sysfs/configfs.
IMHO the complexity of the code is not so much of a problem. What I
see as a problem is the complexity a kernel/user space interface that
manages a the devices with global state.
One of the greatest features of Michaels vhost driver is that all
the state is associated with open file descriptors that either exist
already or belong to the vhost_net misc device. When a process dies,
all the file descriptors get closed and the whole state is cleaned
up implicitly.
AFAICT, you can't do that with the vbus host model.
> > What performance oriented items have been left unaddressed?
>
> Well, the interrupt model to name one.
The performance aspects of your interrupt model are independent
of the vbus proxy, or at least they should be. Let's assume for
now that your event notification mechanism gives significant
performance improvements (which we can't measure independently
right now). I don't see a reason why we could not get the
same performance out of a paravirtual interrupt controller
that uses the same method, and it would be straightforward
to implement one and use that together with all the existing
emulated PCI devices and virtio devices including vhost_net.
Arnd <><
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Avi Kivity @ 2009-08-18 17:47 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <20090818172752.GC17631@ovro.caltech.edu>
On 08/18/2009 08:27 PM, Ira W. Snyder wrote:
>> In fact, modern x86s do have dma engines these days (google for Intel
>> I/OAT), and one of our plans for vhost-net is to allow their use for
>> packets above a certain size. So a patch allowing vhost-net to
>> optionally use a dma engine is a good thing.
>>
> Yes, I'm aware that very modern x86 PCs have general purpose DMA
> engines, even though I don't have any capable hardware. However, I think
> it is better to support using any PC (with or without DMA engine, any
> architecture) as the PCI master, and just handle the DMA all from the
> PCI agent, which is known to have DMA?
>
Certainly; but if your PCI agent will support the DMA API, then the same
vhost code will work with both I/OAT and your specialized hardware.
>> Exposing a knob to userspace is not an insurmountable problem; vhost-net
>> already allows changing the memory layout, for example.
>>
>>
> Let me explain the most obvious problem I ran into: setting the MAC
> addresses used in virtio.
>
> On the host (PCI master), I want eth0 (virtio-net) to get a random MAC
> address.
>
> On the guest (PCI agent), I want eth0 (virtio-net) to get a specific MAC
> address, aa:bb:cc:dd:ee:ff.
>
> The virtio feature negotiation code handles this, by seeing the
> VIRTIO_NET_F_MAC feature in it's configuration space. If BOTH drivers do
> not have VIRTIO_NET_F_MAC set, then NEITHER will use the specified MAC
> address. This is because the feature negotiation code only accepts a
> feature if it is offered by both sides of the connection.
>
> In this case, I must have the guest generate a random MAC address and
> have the host put aa:bb:cc:dd:ee:ff into the guest's configuration
> space. This basically means hardcoding the MAC addresses in the Linux
> drivers, which is a big no-no.
>
> What would I expose to userspace to make this situation manageable?
>
>
I think in this case you want one side to be virtio-net (I'm guessing
the x86) and the other side vhost-net (the ppc boards with the dma
engine). virtio-net on x86 would communicate with userspace on the ppc
board to negotiate features and get a mac address, the fast path would
be between virtio-net and vhost-net (which would use the dma engine to
push and pull data).
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [PATCH] revert TCP retransmission backoff on ICMP destination unreachable
From: Damian Lukowski @ 2009-08-18 17:40 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: David Miller, Netdev
In-Reply-To: <alpine.DEB.2.00.0908181514560.10654@wel-95.cs.helsinki.fi>
> On Fri, 14 Aug 2009, Damian Lukowski wrote:
>
>>> Longer than 80 columns, and use an inline function instead
>>> of a macro in order to get proper type checking.
>>> [...]
>>> Do not break up the function local variables with spurious new lines
>>> like this, please.
>>> [...]
>>> The indentation and tabbing is messed up in all of the code you are
>>> adding, please fix it up to be consistent with the surrounding code
>>> and the rest of the TCP stack.
>>>
>>> Do not use C++ style // comments.
>>
>> Better?
>
> Please, include the changelog message on resubmits too next time.
I'm sorry, which message do you mean? I used plain diff without GIT or
anything.
>> + if ((code != ICMP_NET_UNREACH && code != ICMP_HOST_UNREACH) ||
>> + seq != tp->snd_una || !icsk->icsk_retransmits ||
>> + !icsk->icsk_backoff)
>
> I'd recommend you break this into two if()\nbreak's, one for filtering
> other icmps and other for filtering mismatching against the socket's state.
Ok.
>> + icsk->icsk_backoff--;
>> + icsk->icsk_rto >>= 1;
>
> Are you absolute sure that we don't go to zero here when phase of the
> moon is just right? I'd put a max(..., 1) guard there.
Well, the retransmission timer doubles icsk_rto whenever it increases
icsk_backoff, so we should reach the original icsk_rto, when
icsk_backoff becomes zero.
>> + skb_r = skb_peek(&sk->sk_write_queue);
>
> tcp_write_queue_head(sk)
Ok.
>
>> + BUG_ON(!skb_r);
>> +
>> + if (sock_owned_by_user(sk)) {
>> + /* Deferring retransmission clocked by ICMP
>> + * due to locked socket. */
>> + inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
>> + min(icsk->icsk_rto, TCP_RESOURCE_PROBE_INTERVAL),
>
> ??? (besides having indent wrong). What makes you think HZ/2 is right
> value if icsk_rto is large?
To be honest, I have taken over the code from my predecessor and didn't
question this part. When thinking about it, I would remove this block
completely. Will bad things happen, if I mess with the timer, when the
socket is locked?
>> + TCP_RTO_MAX);
>
> Perhaps you lack here something to exit?
>
>> + }
>> +
>> + if (tcp_time_stamp - TCP_SKB_CB(skb_r)->when >
>> + inet_csk(sk)->icsk_rto) {
>
> icsk->icsk_rto !?!
>
>> + /* RTO revert clocked out retransmission. */
>> + tcp_retransmit_skb(sk, skb_r);
>
> This is plain wrong, tcp_sock counters will get messed up if
> TCPCB_SACKED_RETRANS is already set while calling tcp_retransmit_skb. As
> a sidenote, it would be probably useful to move the check + clear of
> that bit before doing the retransmission to some common place one day.
What, if I call tcp_retransmit_timer() manually instead? Will there
still be problems with SACK?
>> + inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
>> + icsk->icsk_rto, TCP_RTO_MAX);
>
> RFC 2988, step 5.5 missing? Have you verified this patch for real?
Thats the whole point of the draft. If consecutive retransmissions
trigger fitting ICMPs, the RTO value is not doubled, but remains
constant. So you can have a retransmission schedule of
t, t+r, t+2r, t+3r, t+4r, t+5r ...
instead of
t, t+r, t+3r, t+7r, t+15r, t+31r ...
I can show you trace plots of patched an unpatched TCP, if you like.
Should I attach files in the mails, or better post hyperlinks to them?
>> + } else {
>> + /* RTO revert shortened timer. */
>> + inet_csk_reset_xmit_timer(
>> + sk, ICSK_TIME_RETRANS,
>> + icsk->icsk_rto-
>> + (tcp_time_stamp-TCP_SKB_CB(skb_r)->when),
>
> Spacing.
>
>> + TCP_RTO_MAX);
>> + }
>> +
>
> How about:
>
> u32 remaining;
>
> remaining = icsk->icsk_rto - min(icsk->icsk_rto,
> tcp_time_stamp - TCP_SKB_CB(skb_r)->when));
> if (!remaining) {
> tcp_retransmit_skb(...);
> remaining = icsk->icsk_rto;
> }
> inet_csk_reset_xmit_timer(..., remaining);
Seems to be ok, but isn't tcp_retransmit_timer(...) better?
>> - if (icsk->icsk_retransmits >= sysctl_tcp_retries1) {
>> + if (retrans_overstepped(sk, sysctl_tcp_retries1)) {
>
> ??? Could you justify these changes and explain their relation to the
> draft and icmp messages? If not, please drop them and try with proper
> description and description for them in a separate patch. I fail to see
> what you're trying to achieve here. You just ended up redefining the
> sysctl meaning too I think...
Well, you're quite right. RFCs specify timeouts in dimensions of time
(seconds, minutes, etc.), but Linux specifies timeouts in form of
numbers of retransmissions. One can do this, as long as the
retransmission schedule (exponential backoff in RFC case) is known. But
with this patch, the schedule can be completely different. In the
"worst" case, the time intervals between consecutive retransmission
probes are constant, leading to a TCP timeout after few seconds, instead
of several minutes.
What I did here, is to calculate the TCP timeout as if the schedule were
still an exponential backoff, given the allowed number of
retransmissions. It is possible, that TCP will now retransmit many more
times than tcp_retries indicates, but the duration until the TCP
connection times out, is more or less equivalent to unpatched TCP with
same tcp_retries values.
So, whenever some control block uses count values, but means time
values, I have to replace the control block appropriately; and that's
what retrans_overstepped() is supposed to do.
Thanks for your help, so far
Damian
^ permalink raw reply
* Re: bad nat connection tracking performance with ip_gre
From: Timo Teräs @ 2009-08-18 17:39 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netfilter-devel, netdev
In-Reply-To: <4A8AC1A0.6000602@trash.net>
Patrick McHardy wrote:
> Timo Teräs wrote:
>> Looped back by multicast routing:
>>
>> raw:PREROUTING:policy:1 IN=eth1 OUT= MAC= SRC=10.252.5.1
>> DST=239.255.12.42 LEN=1344 TOS=0x00 PREC=0x00 TTL=8 ID=36594 DF
>> PROTO=UDP SPT=33977 DPT=1234 LEN=1324 mangle:PREROUTING:policy:1 IN=eth1
>> OUT= MAC= SRC=10.252.5.1 DST=239.255.12.42 LEN=1344 TOS=0x00 PREC=0x00
>> TTL=8 ID=36594 DF PROTO=UDP SPT=33977 DPT=1234 LEN=1324
>> The cpu hogging happens somewhere below this, since the more
>> multicast destinations I have the more CPU it takes.
>
> So you're sending to multiple destinations? That obviously increases
> the time spent in netfilter and the remaining networking stack.
Yes. But my observation was that for the same amount of packets
sent locally the CPU usage is significantly higher than if they
are forwarded from physical interface. That's what made me
curious.
If I had remember that icmp conn track entries get pruned right
when they get icmp reply back, I would not have probably bothered
to bug you. But that made me think it was more of generic problem
than my patch.
>> Multicast forwarded (I hacked this into the code; but similar
>> dump happens on local sendto()):
>>
>> Actually, now that I think, here we should have the inner IP
>> contents, and not the incomplete outer yet. So apparently
>> the ipgre_header() messes the network_header position.
>
> It shouldn't even have been called at this point. Please retry this
> without your changes.
I patched ipmr.c to explicitly call dev_hard_header to setup the
ipgre nbma receiver. Sadly, the call was wrong side of the nf_hook.
Adjusting that makes the forward hooks look ok.
I thought hook was using network_header to figure out where the
IP header is, but looks like that isn't the case.
>> mangle:FORWARD:policy:1 IN=eth1 OUT=gre1 SRC=0.0.0.0 DST=re.mo.te.ip
>> LEN=0 TOS=0x00 PREC=0x00 TTL=64 ID=0 DF PROTO=47 filter:FORWARD:rule:2
>> IN=eth1 OUT=gre1 SRC=0.0.0.0 DST=re.mo.te.ip LEN=0 TOS=0x00 PREC=0x00
>> TTL=64 ID=0 DF PROTO=47
>
> This looks really broken. Why is the protocol already 47 before it even
> reaches the gre tunnel?
Broken by me as explained.
>> ip_gre xmit sends out:
>
> There should be a POSTROUTING hook here.
Hmm... Looking at the code I probably broke this too. Could missing
this hook have a performance penalty for future packets for the
same flow?
Ok. I'll go back to drawing board. I should have done the
multicast handling for nbma destinations on ip_gre side as I was
wondering earlier. I'll also double check with oprofile the local
sendto() approach where it dies.
- Timo
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" 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: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Ira W. Snyder @ 2009-08-18 17:27 UTC (permalink / raw)
To: Avi Kivity
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <4A8ADC09.3030205@redhat.com>
On Tue, Aug 18, 2009 at 07:51:21PM +0300, Avi Kivity wrote:
> On 08/18/2009 06:53 PM, Ira W. Snyder wrote:
>> So, in my system, copy_(to|from)_user() is completely wrong. There is no
>> userspace, only a physical system. In fact, because normal x86 computers
>> do not have DMA controllers, the host system doesn't actually handle any
>> data transfer!
>>
>
> In fact, modern x86s do have dma engines these days (google for Intel
> I/OAT), and one of our plans for vhost-net is to allow their use for
> packets above a certain size. So a patch allowing vhost-net to
> optionally use a dma engine is a good thing.
>
Yes, I'm aware that very modern x86 PCs have general purpose DMA
engines, even though I don't have any capable hardware. However, I think
it is better to support using any PC (with or without DMA engine, any
architecture) as the PCI master, and just handle the DMA all from the
PCI agent, which is known to have DMA?
>> I used virtio-net in both the guest and host systems in my example
>> virtio-over-PCI patch, and succeeded in getting them to communicate.
>> However, the lack of any setup interface means that the devices must be
>> hardcoded into both drivers, when the decision could be up to userspace.
>> I think this is a problem that vbus could solve.
>>
>
> Exposing a knob to userspace is not an insurmountable problem; vhost-net
> already allows changing the memory layout, for example.
>
Let me explain the most obvious problem I ran into: setting the MAC
addresses used in virtio.
On the host (PCI master), I want eth0 (virtio-net) to get a random MAC
address.
On the guest (PCI agent), I want eth0 (virtio-net) to get a specific MAC
address, aa:bb:cc:dd:ee:ff.
The virtio feature negotiation code handles this, by seeing the
VIRTIO_NET_F_MAC feature in it's configuration space. If BOTH drivers do
not have VIRTIO_NET_F_MAC set, then NEITHER will use the specified MAC
address. This is because the feature negotiation code only accepts a
feature if it is offered by both sides of the connection.
In this case, I must have the guest generate a random MAC address and
have the host put aa:bb:cc:dd:ee:ff into the guest's configuration
space. This basically means hardcoding the MAC addresses in the Linux
drivers, which is a big no-no.
What would I expose to userspace to make this situation manageable?
Thanks for the response,
Ira
^ permalink raw reply
* [PATCH] mdio: mdio_if_info::mmds should not be __bitwise
From: Ben Hutchings @ 2009-08-18 16:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
I misunderstood the meaning of __bitwise. In practice it makes sparse
warn about every use of mmds which is certainly not what we want.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
This should be safe to include in 2.6.31 but it's hardly critical.
Ben.
include/linux/mdio.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index cfdf1df..c779b49 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -304,7 +304,7 @@ static inline __u16 mdio_phy_id_devad(int phy_id)
*/
struct mdio_if_info {
int prtad;
- u32 __bitwise mmds;
+ u32 mmds;
unsigned mode_support;
struct net_device *dev;
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* Re: mlx4 2.6.31-rc5: SW2HW_EQ failed.
From: Roland Dreier @ 2009-08-18 16:56 UTC (permalink / raw)
To: Christoph Lameter; +Cc: netdev, Yevgeny Petrilin
In-Reply-To: <alpine.DEB.1.10.0908181148460.3840@gentwo.org>
> Never heard of device FW.
I just meant firmware running on the device (in this case the connectx adapter).
> The Mellanox NIC has two ports. Could that be it?
No, the device is basically doing
nreq = num_possible_cpus() + 1;
pci_enable_msix(dev->pdev, entries, nreq);
(edited a bit but that's the code). And nreq is ending up as 33 on your
box.
> Will get back to you shortly when we have tested this patch.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Michael S. Tsirkin @ 2009-08-18 16:51 UTC (permalink / raw)
To: Gregory Haskins
Cc: Avi Kivity, Anthony Liguori, Ingo Molnar, Gregory Haskins, kvm,
alacrityvm-devel, linux-kernel, netdev
In-Reply-To: <4A8ACE1F.6020402@gmail.com>
On Tue, Aug 18, 2009 at 11:51:59AM -0400, Gregory Haskins wrote:
> > It's not laughably trivial when you try to support the full feature set
> > of kvm (for example, live migration will require dirty memory tracking,
> > and exporting all state stored in the kernel to userspace).
>
> Doesn't vhost suffer from the same issue? If not, could I also apply
> the same technique to support live-migration in vbus?
vhost does this by switching to userspace for the duration of live
migration. venet could do this I guess, but you'd need to write a
userspace implementation. vhost just reuses existing userspace virtio.
> With all due respect, I didnt ask you do to anything, especially not
> abandon something you are happy with.
>
> All I did was push guest drivers to LKML. The code in question is
> independent of KVM, and its proven to improve the experience of using
> Linux as a platform. There are people interested in using them (by
> virtue of the number of people that have signed up for the AlacrityVM
> list, and have mailed me privately about this work).
>
> So where is the problem here?
If virtio net in guest could be improved instead, everyone would
benefit. I am doing this, and I wish more people would join. Instead,
you change ABI in a incompatible way. So now, there's no single place to
work on kvm networking performance. Now, it would all be understandable
if the reason was e.g. better performance. But you say yourself it
isn't. See the problem?
--
MST
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Avi Kivity @ 2009-08-18 16:51 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <20090818155329.GD31060@ovro.caltech.edu>
On 08/18/2009 06:53 PM, Ira W. Snyder wrote:
> So, in my system, copy_(to|from)_user() is completely wrong. There is no
> userspace, only a physical system. In fact, because normal x86 computers
> do not have DMA controllers, the host system doesn't actually handle any
> data transfer!
>
In fact, modern x86s do have dma engines these days (google for Intel
I/OAT), and one of our plans for vhost-net is to allow their use for
packets above a certain size. So a patch allowing vhost-net to
optionally use a dma engine is a good thing.
> I used virtio-net in both the guest and host systems in my example
> virtio-over-PCI patch, and succeeded in getting them to communicate.
> However, the lack of any setup interface means that the devices must be
> hardcoded into both drivers, when the decision could be up to userspace.
> I think this is a problem that vbus could solve.
>
Exposing a knob to userspace is not an insurmountable problem; vhost-net
already allows changing the memory layout, for example.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Avi Kivity @ 2009-08-18 16:47 UTC (permalink / raw)
To: Gregory Haskins
Cc: Anthony Liguori, Ingo Molnar, Gregory Haskins, kvm,
alacrityvm-devel, linux-kernel, netdev, Michael S. Tsirkin
In-Reply-To: <4A8ACE1F.6020402@gmail.com>
On 08/18/2009 06:51 PM, Gregory Haskins wrote:
>
>> It's not laughably trivial when you try to support the full feature set
>> of kvm (for example, live migration will require dirty memory tracking,
>> and exporting all state stored in the kernel to userspace).
>>
> Doesn't vhost suffer from the same issue? If not, could I also apply
> the same technique to support live-migration in vbus?
>
It does. There are two possible solutions to that: dropping the entire
protocol to userspace, or the one I prefer, proxying the ring and
eventfds in userspace but otherwise letting vhost-net run normally.
This way userspace gets to see descriptors and mark the pages as dirty.
Both these approaches rely on vhost-net being an accelerator to a
userspace based component, but maybe you can adapt venet to use
something similar.
>> Oh come on, I wrote "steal" as a convenient shorthand for
>> "cross-pollinate your ideas into our code according to the letter and
>> spirit of the GNU General Public License".
>>
> Is that supposed to make me feel better about working with you? I mean,
> writing, testing, polishing patches for LKML-type submission is time
> consuming. If all you are going to do is take those ideas and rewrite
> it yourself, why should I go through that effort?
>
If you're posting your ideas for everyone to read in the form of code,
why not post them in the form of design ideas as well? In any case
you've given up any secrets. In the worst case you've lost nothing, in
the best case you may get some hopefully constructive criticism and
maybe improvements.
I'm perfectly happy picking up ideas from competing projects (and I
have) and seeing my ideas picked up in competing projects (which I also
have).
Really, isn't that the point of open source? Share code, but also share
ideas?
> And its not like that was the first time you have said that to me.
>
And I meant it every time.
Haven't you just asked how vhost-net plans to do live migration?
>> Since we're all trying to improve Linux we may as well cooperate.
>>
> Well, I don't think anyone can say that I haven't been trying.
>
I'd be obliged if you reveal some of your secret sauce then (only the
parts you plan to GPL anyway of course).
>>> "sorry, we are going to reinvent our own instead".
>>>
>> No. Adopting venet/vbus would mean reinventing something that already
>> existed.
>>
> But yet, it doesn't.
>
We'll need to do the agree to disagree thing again here.
>> Continuing to support virtio/pci is not reinventing anything.
>>
> No one asked you to do otherwise.
>
Right, and I'm not keen on supporting both. See why I want to stick to
virtio/pci as long as I possibly can?
>> You haven't convinced me that your ideas are worth the effort of
>> abandoning virtio/pci or maintaining both venet/vbus and virtio/pci.
>>
> With all due respect, I didnt ask you do to anything, especially not
> abandon something you are happy with.
>
> All I did was push guest drivers to LKML. The code in question is
> independent of KVM, and its proven to improve the experience of using
> Linux as a platform. There are people interested in using them (by
> virtue of the number of people that have signed up for the AlacrityVM
> list, and have mailed me privately about this work).
>
> So where is the problem here?
>
I'm unhappy with the duplication of effort and potential fragmentation
of the developer and user communities, that's all. I'd rather see the
work going into vbus/venet going into virtio. I think it's a legitimate
concern.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH] Speed-up pfifo_fast lookup using a bitmap
From: Krishna Kumar2 @ 2009-08-18 16:46 UTC (permalink / raw)
To: netdev
In-Reply-To: <20090817.190343.49229178.davem@davemloft.net>
Sorry about the delay, I am sick with the flu for the last few days.
I will send the patch tomorrow.
thanks,
- KK
David Miller <davem@davemloft.net> wrote on 08/18/2009 07:33:43 AM:
> David Miller <davem@davemloft.net>
> 08/18/2009 07:33 AM
>
> To
>
> jarkao2@gmail.com
>
> cc
>
> Krishna Kumar2/India/IBM@IBMIN, kaber@trash.net, netdev@vger.kernel.org,
> herbert@gondor.apana.org.au
>
> Subject
>
> Re: [PATCH] Speed-up pfifo_fast lookup using a bitmap
>
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Fri, 14 Aug 2009 23:36:43 +0200
>
> > I definitely can't see a reason to make this variable public, and
> > prefer the private (v2) version (which still lacks a changelog, btw).
>
> I'm fine with patch v2, but yes it needs to be properly submitted
> with a real changelog before I can apply it :-)
^ permalink raw reply
* Re: [PATCH 3/3] net: skb ftracer - Add actual ftrace code to kernel (v3)
From: Neil Horman @ 2009-08-18 16:39 UTC (permalink / raw)
To: Steven Rostedt; +Cc: netdev, davem
In-Reply-To: <alpine.DEB.2.00.0908171647470.25890@gandalf.stny.rr.com>
On Mon, Aug 17, 2009 at 04:55:38PM -0400, Steven Rostedt wrote:
>
> Hi Neil!
>
> Sorry for the late reply, I've been on vacation for the last week.
>
> On Thu, 13 Aug 2009, Neil Horman wrote:
>
> > skb allocation / consumption correlator
> >
> > Add ftracer module to kernel to print out a list that correlates a process id,
> > an skb it read, and the numa nodes on wich the process was running when it was
> > read along with the numa node the skbuff was allocated on.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> >
> >
> > Makefile | 1
> > trace.h | 19 ++++++
> > trace_skb_sources.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 174 insertions(+)
> >
> > diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> > index 844164d..ee5e5b1 100644
> > --- a/kernel/trace/Makefile
> > +++ b/kernel/trace/Makefile
> > @@ -49,6 +49,7 @@ obj-$(CONFIG_BLK_DEV_IO_TRACE) += blktrace.o
> > ifeq ($(CONFIG_BLOCK),y)
> > obj-$(CONFIG_EVENT_TRACING) += blktrace.o
> > endif
> > +obj-$(CONFIG_SKB_SOURCES_TRACER) += trace_skb_sources.o
> > obj-$(CONFIG_EVENT_TRACING) += trace_events.o
> > obj-$(CONFIG_EVENT_TRACING) += trace_export.o
> > obj-$(CONFIG_FTRACE_SYSCALLS) += trace_syscalls.o
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 8b9f4f6..8a6281b 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -11,6 +11,7 @@
> > #include <trace/boot.h>
> > #include <linux/kmemtrace.h>
> > #include <trace/power.h>
> > +#include <trace/events/skb.h>
> >
> > #include <linux/trace_seq.h>
> > #include <linux/ftrace_event.h>
> > @@ -40,6 +41,7 @@ enum trace_type {
> > TRACE_KMEM_FREE,
> > TRACE_POWER,
> > TRACE_BLK,
> > + TRACE_SKB_SOURCE,
> >
> > __TRACE_LAST_TYPE,
> > };
> > @@ -171,6 +173,21 @@ struct trace_power {
> > struct power_trace state_data;
> > };
> >
> > +struct skb_record {
> > + pid_t pid; /* pid of the copying process */
> > + int anid; /* node where skb was allocated */
> > + int cnid; /* node to which skb was copied in userspace */
> > + char ifname[IFNAMSIZ]; /* Name of the receiving interface */
> > + int rx_queue; /* The rx queue the skb was received on */
> > + int ccpu; /* Cpu the application got this frame from */
> > + int len; /* length of the data copied */
> > +};
> > +
> > +struct trace_skb_event {
> > + struct trace_entry ent;
> > + struct skb_record event_data;
> > +};
> > +
> > enum kmemtrace_type_id {
> > KMEMTRACE_TYPE_KMALLOC = 0, /* kmalloc() or kfree(). */
> > KMEMTRACE_TYPE_CACHE, /* kmem_cache_*(). */
> > @@ -323,6 +340,8 @@ extern void __ftrace_bad_type(void);
> > TRACE_SYSCALL_ENTER); \
> > IF_ASSIGN(var, ent, struct syscall_trace_exit, \
> > TRACE_SYSCALL_EXIT); \
> > + IF_ASSIGN(var, ent, struct trace_skb_event, \
> > + TRACE_SKB_SOURCE); \
> > __ftrace_bad_type(); \
> > } while (0)
> >
> > diff --git a/kernel/trace/trace_skb_sources.c b/kernel/trace/trace_skb_sources.c
> > new file mode 100644
> > index 0000000..4ba3671
> > --- /dev/null
> > +++ b/kernel/trace/trace_skb_sources.c
> > @@ -0,0 +1,154 @@
> > +/*
> > + * ring buffer based tracer for analyzing per-socket skb sources
> > + *
> > + * Neil Horman <nhorman@tuxdriver.com>
> > + * Copyright (C) 2009
> > + *
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/debugfs.h>
> > +#include <trace/events/skb.h>
> > +#include <linux/kallsyms.h>
> > +#include <linux/module.h>
> > +#include <linux/hardirq.h>
> > +#include <linux/netdevice.h>
> > +#include <net/sock.h>
> > +
> > +#include "trace.h"
> > +#include "trace_output.h"
> > +
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(skb_copy_datagram_iovec);
> > +
> > +static struct trace_array *skb_trace;
> > +static int __read_mostly trace_skb_source_enabled;
> > +
> > +static void probe_skb_dequeue(const struct sk_buff *skb, int len)
> > +{
> > + struct ring_buffer_event *event;
> > + struct trace_skb_event *entry;
> > + struct trace_array *tr = skb_trace;
> > + struct net_device *dev;
> > +
> > + if (!trace_skb_source_enabled)
> > + return;
> > +
> > + if (in_interrupt())
> > + return;
>
> Is there a reason for not doing this in an interrupt?
>
Because the idea is to correlate skb consumption to a process. If we get in
this tracepoint in an interrupt, it doesn't make sense to record.
> > +
> > + event = trace_buffer_lock_reserve(tr, TRACE_SKB_SOURCE,
> > + sizeof(*entry), 0, 0);
> > + if (!event)
> > + return;
> > + entry = ring_buffer_event_data(event);
> > +
> > + entry->event_data.pid = current->pid;
>
> Note, the trace_buffer_lock_reserve will record the current pid, thus you
> do not need to record it here.
>
> > + entry->event_data.anid = page_to_nid(virt_to_page(skb->data));
> > + entry->event_data.cnid = cpu_to_node(smp_processor_id());
> > + entry->event_data.len = len;
> > + entry->event_data.rx_queue = skb->queue_mapping;
> > + entry->event_data.ccpu = smp_processor_id();
>
> Also, the cpu is recorded in the ring buffer. They are per cpu ring
> buffers and that determines the cpu it was recorded on.
>
> > +
> > + dev = dev_get_by_index(sock_net(skb->sk), skb->iif);
> > + if (dev) {
> > + memcpy(entry->event_data.ifname, dev->name, IFNAMSIZ);
> > + dev_put(dev);
> > + } else {
> > + strcpy(entry->event_data.ifname, "Unknown");
> > + }
> > +
> > + trace_buffer_unlock_commit(tr, event, 0, 0);
> > +}
> > +
> > +static int tracing_skb_source_register(void)
> > +{
> > + int ret;
> > +
> > + ret = register_trace_skb_copy_datagram_iovec(probe_skb_dequeue);
> > + if (ret)
> > + pr_info("skb source trace: Couldn't activate dequeue tracepoint");
> > +
> > + return ret;
> > +}
> > +
> > +static void start_skb_source_trace(struct trace_array *tr)
> > +{
> > + trace_skb_source_enabled = 1;
> > +}
> > +
> > +static void stop_skb_source_trace(struct trace_array *tr)
> > +{
> > + trace_skb_source_enabled = 0;
> > +}
> > +
> > +static void skb_source_trace_reset(struct trace_array *tr)
> > +{
> > + trace_skb_source_enabled = 0;
> > + unregister_trace_skb_copy_datagram_iovec(probe_skb_dequeue);
> > +}
> > +
> > +
> > +static int skb_source_trace_init(struct trace_array *tr)
> > +{
> > + int cpu;
> > + skb_trace = tr;
> > +
> > + trace_skb_source_enabled = 1;
> > + tracing_skb_source_register();
> > +
> > + for_each_cpu(cpu, cpu_possible_mask)
> > + tracing_reset(tr, cpu);
> > + return 0;
> > +}
> > +
> > +static enum print_line_t skb_source_print_line(struct trace_iterator *iter)
> > +{
> > + int ret = 0;
> > + struct trace_entry *entry = iter->ent;
>
> iter->cpu has the cpu that trace was recorded on.
> entry->pid has the pid of the process that did the recording.
>
ok, I'll clean this up in a subsequent patch, since davem has already rolled
them in.
> > + struct trace_skb_event *event;
> > + struct skb_record *record;
> > + struct trace_seq *s = &iter->seq;
> > +
> > + trace_assign_type(event, entry);
> > + record = &event->event_data;
> > + if (entry->type != TRACE_SKB_SOURCE)
> > + return TRACE_TYPE_UNHANDLED;
> > +
> > + ret = trace_seq_printf(s, " %d %d %d %s %d %d %d\n",
> > + record->pid,
> > + record->anid,
> > + record->cnid,
> > + record->ifname,
> > + record->rx_queue,
> > + record->ccpu,
> > + record->len);
> > +
> > + if (!ret)
> > + return TRACE_TYPE_PARTIAL_LINE;
> > +
> > + return TRACE_TYPE_HANDLED;
> > +}
> > +
> > +static void skb_source_print_header(struct seq_file *s)
> > +{
> > + seq_puts(s, "# PID ANID CNID IFC RXQ CCPU LEN\n");
> > + seq_puts(s, "# | | | | | | |\n");
> > +}
> > +
> > +static struct tracer skb_source_tracer __read_mostly =
> > +{
> > + .name = "skb_sources",
> > + .init = skb_source_trace_init,
> > + .start = start_skb_source_trace,
> > + .stop = stop_skb_source_trace,
> > + .reset = skb_source_trace_reset,
> > + .print_line = skb_source_print_line,
> > + .print_header = skb_source_print_header,
> > +};
> > +
> > +static int init_skb_source_trace(void)
> > +{
> > + return register_tracer(&skb_source_tracer);
> > +}
> > +device_initcall(init_skb_source_trace);
> >
>
> BTW, why not just do this as events? Or was this just a easy way to
> communicate with the user space tools?
>
Thats exactly why I did it. the idea is for me to now write a user space tool
that lets me analyze the events and ajust process scheduling to optimize the rx
path.
Neil
> -- Steve
>
>
^ permalink raw reply
* Re: [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Michael S. Tsirkin @ 2009-08-18 16:39 UTC (permalink / raw)
To: Gregory Haskins
Cc: Ingo Molnar, kvm, Avi Kivity, alacrityvm-devel, linux-kernel,
netdev
In-Reply-To: <4A8ACB2D.9060108@gmail.com>
On Tue, Aug 18, 2009 at 11:39:25AM -0400, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Mon, Aug 17, 2009 at 03:33:30PM -0400, Gregory Haskins wrote:
> >> There is a secondary question of venet (a vbus native device) verses
> >> virtio-net (a virtio native device that works with PCI or VBUS). If
> >> this contention is really around venet vs virtio-net, I may possibly
> >> conceed and retract its submission to mainline.
> >
> > For me yes, venet+ioq competing with virtio+virtqueue.
> >
> >> I've been pushing it to date because people are using it and I don't
> >> see any reason that the driver couldn't be upstream.
> >
> > If virtio is just as fast, they can just use it without knowing it.
> > Clearly, that's better since we support virtio anyway ...
>
> More specifically: kvm can support whatever it wants. I am not asking
> kvm to support venet.
>
> If we (the alacrityvm community) decide to keep maintaining venet, _we_
> will support it, and I have no problem with that.
>
> As of right now, we are doing some interesting things with it in the lab
> and its certainly more flexible for us as a platform since we maintain
> the ABI and feature set. So for now, I do not think its a big deal if
> they both co-exist, and it has no bearing on KVM upstream.
As someone who extended them recently, both ABI and feature set with
virtio are pretty flexible. What's the problem? Will every single
contributor now push a driver with an incompatible ABI upstream because
this way he maintains both ABI and feature set? Oh well ...
--
MST
^ permalink raw reply
* Re: [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Avi Kivity @ 2009-08-18 16:27 UTC (permalink / raw)
To: Gregory Haskins
Cc: Ingo Molnar, kvm, alacrityvm-devel, linux-kernel, netdev,
Michael S. Tsirkin
In-Reply-To: <4A8ABEC9.6090006@gmail.com>
On 08/18/2009 05:46 PM, Gregory Haskins wrote:
>
>> Can you explain how vbus achieves RDMA?
>>
>> I also don't see the connection to real time guests.
>>
> Both of these are still in development. Trying to stay true to the
> "release early and often" mantra, the core vbus technology is being
> pushed now so it can be reviewed. Stay tuned for these other developments.
>
Hopefully you can outline how it works. AFAICT, RDMA and kernel bypass
will need device assignment. If you're bypassing the call into the host
kernel, it doesn't really matter how that call is made, does it?
>>> I also designed it in such a way that
>>> we could, in theory, write one set of (linux-based) backends, and have
>>> them work across a variety of environments (such as containers/VMs like
>>> KVM, lguest, openvz, but also physical systems like blade enclosures and
>>> clusters, or even applications running on the host).
>>>
>>>
>> Sorry, I'm still confused. Why would openvz need vbus?
>>
> Its just an example. The point is that I abstracted what I think are
> the key points of fast-io, memory routing, signal routing, etc, so that
> it will work in a variety of (ideally, _any_) environments.
>
> There may not be _performance_ motivations for certain classes of VMs
> because they already have decent support, but they may want a connector
> anyway to gain some of the new features available in vbus.
>
> And looking forward, the idea is that we have commoditized the backend
> so we don't need to redo this each time a new container comes along.
>
I'll wait until a concrete example shows up as I still don't understand.
>> One point of contention is that this is all managementy stuff and should
>> be kept out of the host kernel. Exposing shared memory, interrupts, and
>> guest hypercalls can all be easily done from userspace (as virtio
>> demonstrates). True, some devices need kernel acceleration, but that's
>> no reason to put everything into the host kernel.
>>
> See my last reply to Anthony. My two points here are that:
>
> a) having it in-kernel makes it a complete subsystem, which perhaps has
> diminished value in kvm, but adds value in most other places that we are
> looking to use vbus.
>
It's not a complete system unless you want users to administer VMs using
echo and cat and configfs. Some userspace support will always be necessary.
> b) the in-kernel code is being overstated as "complex". We are not
> talking about your typical virt thing, like an emulated ICH/PCI chipset.
> Its really a simple list of devices with a handful of attributes. They
> are managed using established linux interfaces, like sysfs/configfs.
>
They need to be connected to the real world somehow. What about
security? can any user create a container and devices and link them to
real interfaces? If not, do you need to run the VM as root?
virtio and vhost-net solve these issues. Does vbus?
The code may be simple to you. But the question is whether it's
necessary, not whether it's simple or complex.
>> Exposing devices as PCI is an important issue for me, as I have to
>> consider non-Linux guests.
>>
> Thats your prerogative, but obviously not everyone agrees with you.
>
I hope everyone agrees that it's an important issue for me and that I
have to consider non-Linux guests. I also hope that you're considering
non-Linux guests since they have considerable market share.
> Getting non-Linux guests to work is my problem if you chose to not be
> part of the vbus community.
>
I won't be writing those drivers in any case.
>> Another issue is the host kernel management code which I believe is
>> superfluous.
>>
> In your opinion, right?
>
Yes, this is why I wrote "I believe".
>> Given that, why spread to a new model?
>>
> Note: I haven't asked you to (at least, not since April with the vbus-v3
> release). Spreading to a new model is currently the role of the
> AlacrityVM project, since we disagree on the utility of a new model.
>
Given I'm not the gateway to inclusion of vbus/venet, you don't need to
ask me anything. I'm still free to give my opinion.
>>> A) hardware can only generate byte/word sized requests at a time because
>>> that is all the pcb-etch and silicon support. So hardware is usually
>>> expressed in terms of some number of "registers".
>>>
>>>
>> No, hardware happily DMAs to and fro main memory.
>>
> Yes, now walk me through how you set up DMA to do something like a call
> when you do not know addresses apriori. Hint: count the number of
> MMIO/PIOs you need. If the number is> 1, you've lost.
>
With virtio, the number is 1 (or less if you amortize). Set up the ring
entries and kick.
>> Some hardware of
>> course uses mmio registers extensively, but not virtio hardware. With
>> the recent MSI support no registers are touched in the fast path.
>>
> Note we are not talking about virtio here. Just raw PCI and why I
> advocate vbus over it.
>
There's no such thing as raw PCI. Every PCI device has a protocol. The
protocol virtio chose is optimized for virtualization.
>>> D) device-ids are in a fixed width register and centrally assigned from
>>> an authority (e.g. PCI-SIG).
>>>
>>>
>> That's not an issue either. Qumranet/Red Hat has donated a range of
>> device IDs for use in virtio.
>>
> Yes, and to get one you have to do what? Register it with kvm.git,
> right? Kind of like registering a MAJOR/MINOR, would you agree? Maybe
> you do not mind (especially given your relationship to kvm.git), but
> there are disadvantages to that model for most of the rest of us.
>
Send an email, it's not that difficult. There's also an experimental range.
>> Device IDs are how devices are associated
>> with drivers, so you'll need something similar for vbus.
>>
> Nope, just like you don't need to do anything ahead of time for using a
> dynamic misc-device name. You just have both the driver and device know
> what they are looking for (its part of the ABI).
>
If you get a device ID clash, you fail. If you get a device name clash,
you fail in the same way.
>>> E) Interrupt/MSI routing is per-device oriented
>>>
>>>
>> Please elaborate. What is the issue? How does vbus solve it?
>>
> There are no "interrupts" in vbus..only shm-signals. You can establish
> an arbitrary amount of shm regions, each with an optional shm-signal
> associated with it. To do this, the driver calls dev->shm(), and you
> get back a shm_signal object.
>
> Underneath the hood, the vbus-connector (e.g. vbus-pcibridge) decides
> how it maps real interrupts to shm-signals (on a system level, not per
> device). This can be 1:1, or any other scheme. vbus-pcibridge uses one
> system-wide interrupt per priority level (today this is 8 levels), each
> with an IOQ based event channel. "signals" come as an event on that
> channel.
>
> So the "issue" is that you have no real choice with PCI. You just get
> device oriented interrupts. With vbus, its abstracted. So you can
> still get per-device standard MSI, or you can do fancier things like do
> coalescing and prioritization.
>
As I've mentioned before, prioritization is available on x86, and
coalescing scales badly.
>>> F) Interrupts/MSI are assumed cheap to inject
>>>
>>>
>> Interrupts are not assumed cheap; that's why interrupt mitigation is
>> used (on real and virtual hardware).
>>
> Its all relative. IDT dispatch and EOI overhead are "baseline" on real
> hardware, whereas they are significantly more expensive to do the
> vmenters and vmexits on virt (and you have new exit causes, like
> irq-windows, etc, that do not exist in real HW).
>
irq window exits ought to be pretty rare, so we're only left with
injection vmexits. At around 1us/vmexit, even 100,000 interrupts/vcpu
(which is excessive) will only cost you 10% cpu time.
>>> G) Interrupts/MSI are non-priortizable.
>>>
>>>
>> They are prioritizable; Linux ignores this though (Windows doesn't).
>> Please elaborate on what the problem is and how vbus solves it.
>>
> It doesn't work right. The x86 sense of interrupt priority is, sorry to
> say it, half-assed at best. I've worked with embedded systems that have
> real interrupt priority support in the hardware, end to end, including
> the PIC. The LAPIC on the other hand is really weak in this dept, and
> as you said, Linux doesn't even attempt to use whats there.
>
Maybe prioritization is not that important then. If it is, it needs to
be fixed at the lapic level, otherwise you have no real prioritization
wrt non-vbus interrupts.
>>> H) Interrupts/MSI are statically established
>>>
>>>
>> Can you give an example of why this is a problem?
>>
> Some of the things we are building use the model of having a device that
> hands out shm-signal in response to guest events (say, the creation of
> an IPC channel). This would generally be handled by a specific device
> model instance, and it would need to do this without pre-declaring the
> MSI vectors (to use PCI as an example).
>
You're free to demultiplex an MSI to however many consumers you want,
there's no need for a new bus for that.
>> What performance oriented items have been left unaddressed?
>>
> Well, the interrupt model to name one.
>
Like I mentioned, you can merge MSI interrupts, but that's not
necessarily a good idea.
>> How do you handle conflicts? Again you need a central authority to hand
>> out names or prefixes.
>>
> Not really, no. If you really wanted to be formal about it, you could
> adopt any series of UUID schemes. For instance, perhaps venet should be
> "com.novell::virtual-ethernet". Heck, I could use uuidgen.
>
Do you use DNS. We use PCI-SIG. If Novell is a PCI-SIG member you can
get a vendor ID and control your own virtio space.
>>> As another example, the connector design coalesces *all* shm-signals
>>> into a single interrupt (by prio) that uses the same context-switch
>>> mitigation techniques that help boost things like networking. This
>>> effectively means we can detect and optimize out ack/eoi cycles from the
>>> APIC as the IO load increases (which is when you need it most). PCI has
>>> no such concept.
>>>
>>>
>> That's a bug, not a feature. It means poor scaling as the number of
>> vcpus increases and as the number of devices increases.
>>
> So the "avi-vbus-connector" can use 1:1, if you prefer. Large vcpu
> counts (which are not typical) and irq-affinity is not a target
> application for my design, so I prefer the coalescing model in the
> vbus-pcibridge included in this series. YMMV
>
So far you've left out live migration, Windows, large guests, and
multiqueue out of your design. If you wish to position vbus/venet for
large scale use you'll need to address all of them.
>> Note nothing prevents steering multiple MSIs into a single vector. It's
>> a bad idea though.
>>
> Yes, it is a bad idea...and not the same thing either. This would
> effectively create a shared-line scenario in the irq code, which is not
> what happens in vbus.
>
Ok.
>>> In addition, the signals and interrupts are priority aware, which is
>>> useful for things like 802.1p networking where you may establish 8-tx
>>> and 8-rx queues for your virtio-net device. x86 APIC really has no
>>> usable equivalent, so PCI is stuck here.
>>>
>>>
>> x86 APIC is priority aware.
>>
> Have you ever tried to use it?
>
I haven't, but Windows does.
>>> Also, the signals can be allocated on-demand for implementing things
>>> like IPC channels in response to guest requests since there is no
>>> assumption about device-to-interrupt mappings. This is more flexible.
>>>
>>>
>> Yes. However given that vectors are a scarce resource you're severely
>> limited in that.
>>
> The connector I am pushing out does not have this limitation.
>
Okay.
>
>> And if you're multiplexing everything on one vector,
>> then you can just as well demultiplex your channels in the virtio driver
>> code.
>>
> Only per-device, not system wide.
>
Right. I still think multiplexing interrupts is a bad idea in a large
system. In a small system... why would you do it at all?
>>> And through all of this, this design would work in any guest even if it
>>> doesn't have PCI (e.g. lguest, UML, physical systems, etc).
>>>
>>>
>> That is true for virtio which works on pci-less lguest and s390.
>>
> Yes, and lguest and s390 had to build their own bus-model to do it, right?
>
They had to build connectors just like you propose to do.
> Thank you for bringing this up, because it is one of the main points
> here. What I am trying to do is generalize the bus to prevent the
> proliferation of more of these isolated models in the future. Build
> one, fast, in-kernel model so that we wouldn't need virtio-X, and
> virtio-Y in the future. They can just reuse the (performance optimized)
> bus and models, and only need to build the connector to bridge them.
>
But you still need vbus-connector-lguest and vbus-connector-s390 because
they all talk to the host differently. So what's changed? the names?
>> That is exactly the design goal of virtio (except it limits itself to
>> virtualization).
>>
> No, virtio is only part of the picture. It not including the backend
> models, or how to do memory/signal-path abstraction for in-kernel, for
> instance. But otherwise, virtio as a device model is compatible with
> vbus as a bus model. They compliment one another.
>
Well, venet doesn't complement virtio-net, and virtio-pci doesn't
complement vbus-connector.
>>> Then device models like virtio can ride happily on top and we end up
>>> with a really robust and high-performance Linux-based stack. I don't
>>> buy the argument that we already have PCI so lets use it. I don't think
>>> its the best design and I am not afraid to make an investment in a
>>> change here because I think it will pay off in the long run.
>>>
>>>
>> Sorry, I don't think you've shown any quantifiable advantages.
>>
> We can agree to disagree then, eh? There are certainly quantifiable
> differences. Waving your hand at the differences to say they are not
> advantages is merely an opinion, one that is not shared universally.
>
I've addressed them one by one. We can agree to disagree on interrupt
multiplexing, and the importance of compatibility, Windows, large
guests, multiqueue, and DNS vs. PCI-SIG.
> The bottom line is all of these design distinctions are encapsulated
> within the vbus subsystem and do not affect the kvm code-base. So
> agreement with kvm upstream is not a requirement, but would be
> advantageous for collaboration.
>
Certainly.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Michael S. Tsirkin @ 2009-08-18 16:25 UTC (permalink / raw)
To: Gregory Haskins
Cc: Anthony Liguori, Ingo Molnar, kvm, Avi Kivity, alacrityvm-devel,
linux-kernel, netdev
In-Reply-To: <4A8AC68C.6040308@gmail.com>
On Tue, Aug 18, 2009 at 11:19:40AM -0400, Gregory Haskins wrote:
> >>>> OTOH, Michael's patch is purely targeted at improving virtio-net on kvm,
> >>>> and its likewise constrained by various limitations of that decision
> >>>> (such as its reliance of the PCI model, and the kvm memory scheme).
> >>> vhost is actually not related to PCI in any way. It simply leaves all
> >>> setup for userspace to do. And the memory scheme was intentionally
> >>> separated from kvm so that it can easily support e.g. lguest.
> >>>
> >> I think you have missed my point. I mean that vhost requires a separate
> >> bus-model (ala qemu-pci).
> >
> > So? That can be in userspace, and can be anything including vbus.
>
> -ENOPARSE
>
> Can you elaborate?
Write a device that signals an eventfd on virtio kick, and poll eventfd
for notifications, and you can use vhost-net. vbus, surely, can do
this?
> >
> >> And no, your memory scheme is not separated,
> >> at least, not very well. It still assumes memory-regions and
> >> copy_to_user(), which is very kvm-esque.
> >
> > I don't think so: works for lguest, kvm, UML and containers
>
> kvm _esque_ , meaning anything that follows the region+copy_to_user
> model. Not all things do.
Pretty much all things where it makes sense to share code with
vhost-net. If there's hardware that wants direct access to descriptor
rings, it just needs a driver.
> >> Vbus has people using things
> >> like userspace containers (no regions),
> >
> > vhost by default works without regions
>
> Thats a start, but not good enough if you were trying to achieve the
> same thing as vbus. As I said before, I've never said you had to
> achieve the same thing, but do note they are distinctly different with
> different goals. You are solving a directed problem. I am solving a
> general problem, and trying to solve it once.
Heh. A good demonstration of vbus generality would be a solution that
speeds up virtio in guests. What venet seems to illustrate instead is
that one has to rework all of host, guest and hypervisor to use vbus.
Maybe it does not need to be that way - it just seems so.
> >> and physical hardware (dma
> >> controllers, so no regions or copy_to_user) so your scheme quickly falls
> >> apart once you get away from KVM.
> >
> > Someone took a driver and is building hardware for it ... so what?
>
> What is your point?
OK, can we forget about that physical hardware then?
> >> Don't get me wrong: That design may have its place. Perhaps you only
> >> care about fixing KVM, which is a perfectly acceptable strategy.
> >> Its just not a strategy that I think is the best approach. Essentially you
> >> are promoting the proliferation of competing backends, and I am trying
> >> to unify them (which is ironic that this thread started with concerns I
> >> was fragmenting things ;).
> >
> > So, you don't see how venet fragments things? It's pretty obvious ...
>
> I never said it doesn't. venet started as a test harness, but now it is
> inadvertently fragmenting the virtio-net effort. I admit it. It wasn't
> intentional, but just worked out that way. Until your vhost idea is
> vetted and benchmarked, its not even in the running.
>
> Venet is currently
> the highest performing 802.x acceleration for KVM that I am aware of, so
> it will continue to garner interest from users concerned with performance.
>
> But likewise, vhost has the potential to fragment the back-end model.
> That was my point.
You don't see the difference? Long term vhost-net can just be enabled by
default whenever it is present, and there is a single guest driver to
support. OTOH, venet means that we have to support 2 guest drivers:
virtio and venet, for a long time.
> >
> >> The bottom line is, you have a simpler solution that is more finely
> >> targeted at KVM and virtio-networking. It fixes probably a lot of
> >> problems with the existing implementation, but it still has limitations.
> >>
> >> OTOH, what I am promoting is more complex, but more flexible. That is
> >> the tradeoff. You can't have both ;)
> >
> > We can. connect eventfds to hypercalls, and vhost will work with vbus.
>
> -ENOPARSE
>
> vbus doesnt use hypercalls, and I do not see why or how you would
> connect two backend models together like this. Can you elaborate.
I think some older version did. But whatever. signal eventfd on guest
kick, poll eventfd to notify guest, and you can use vhost-net with vbus.
> >
> >> So do not for one second think
> >> that what you implemented is equivalent, because they are not.
> >>
> >> In fact, I believe I warned you about this potential problem when you
> >> decided to implement your own version. I think I said something to the
> >> effect of "you will either have a subset of functionality, or you will
> >> ultimately reinvent what I did". Right now you are in the subset phase.
> >
> > No. Unlike vbus, vhost supports unmodified guests and live migration.
>
> By "subset", I am referring to your interfaces and the scope of its
> applicability. The things you need to do to make vhost work and a vbus
> device work from a memory and signaling abstration POV are going to be
> extremely similar.
>
> The difference in how the guest sees them these backends is all
> contained in the vbus-connector. Therefore, what you *could* have done
> is simply written a connector that does something like only support
> "virtio" backends, and surfaced them as regular PCI devices to the
> guest. Then you could have reused all the abstraction features in vbus,
> instead of reinventing them (case in point, your region+copy_to_user
> code). And likewise, anyone using vbus could use your virtio-net backend.
>
> Instead, I am still left with no virtio-net backend implemented, and you
> were left designing, writing, and testing facilities that I've already
> completed. So it was duplicative effort.
>
> Kind Regards,
> -Greg
>
As I said, I couldn't reuse your code the way it's written. But happily
you can reuse vhost - it's just a library, link with it - or even vhost
net as I explained above.
--
MST
^ permalink raw reply
* Re: [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Ingo Molnar @ 2009-08-18 16:14 UTC (permalink / raw)
To: Gregory Haskins
Cc: Avi Kivity, Anthony Liguori, Gregory Haskins, kvm,
alacrityvm-devel, linux-kernel, netdev, Michael S. Tsirkin
In-Reply-To: <4A8ACE1F.6020402@gmail.com>
* Gregory Haskins <gregory.haskins@gmail.com> wrote:
> > You haven't convinced me that your ideas are worth the effort
> > of abandoning virtio/pci or maintaining both venet/vbus and
> > virtio/pci.
>
> With all due respect, I didnt ask you do to anything, especially
> not abandon something you are happy with.
>
> All I did was push guest drivers to LKML. The code in question
> is independent of KVM, and its proven to improve the experience
> of using Linux as a platform. There are people interested in
> using them (by virtue of the number of people that have signed up
> for the AlacrityVM list, and have mailed me privately about this
> work).
This thread started because i asked you about your technical
arguments why we'd want vbus instead of virtio. Your answer above
now basically boils down to: "because I want it so, why dont you
leave me alone".
What you are doing here is to in essence to fork KVM, regardless of
the technical counter arguments given against such a fork and
regardless of the ample opportunity given to you to demostrate the
technical advantages of your code. (in which case KVM would happily
migrate to your code)
We all love faster code and better management interfaces and tons
of your prior patches got accepted by Avi. This time you didnt even
_try_ to improve virtio. It's not like you posted a lot of virtio
patches which were not applied. You didnt even try and you need to
try _much_ harder than that before forking a project.
And fragmentation matters quite a bit. To Linux users, developers,
administrators, packagers it's a big deal whether two overlapping
pieces of functionality for the same thing exist within the same
kernel. The kernel is not an anarchy where everyone can have their
own sys_fork() version or their own sys_write() version. Would you
want to have two dozen read() variants, sys_read_oracle() and a
sys_read_db2()?
I certainly dont want that. Instead we (at great expense and work)
try to reach the best technical solution. That means we throw away
inferior code and adopt the better one. (with a reasonable
migration period)
You are ignoring that principle with hand-waving about 'the
community wants this'. I can assure you, users _DONT WANT_ split
interfaces and incompatible drivers for the same thing. They want
stuff that works well.
If the community wants this then why cannot you convince one of the
most prominent representatives of that community, the KVM
developers?
Furthermore, 99% of your work is KVM, why dont you respect that
work by not forking it? Why dont you respect the KVM community and
Linux in general by improving existing pieces of infrastructure
instead of forcefully forking it?
Ingo
^ permalink raw reply
* Re: [PATCH 1/2] alchemy: add au1000-eth platform device
From: Florian Fainelli @ 2009-08-18 16:01 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Ralf Baechle, linux-mips, Manuel Lauss, David Miller, netdev
In-Reply-To: <4A8AC125.3020602@ru.mvista.com>
Le Tuesday 18 August 2009 16:56:37 Sergei Shtylyov, vous avez écrit :
> Hello.
>
> Florian Fainelli wrote:
> > This patch adds the board code to register a per-board au1000-eth
> > platform device to be used wit the au1000-eth platform driver in a
> > subsequent patch. Note that the au1000-eth driver knows about the
> > default driver settings such that we do not need to pass any
> > platform_data informations in most cases except db1x00.
>
> Sigh, NAK...
> Please don't register the SoC device per board, do it in
> alchemy/common/platfrom.c and find a way to pass the board specific
> platform data from the board file there instead -- something like
> arch/arm/mach-davinci/usb.c does.
Ok, like I promised, this was the per-board device registration. Do you prefer something like this:
--
From fd75b7c7fa3c05c21122c43e43260d2785475a79 Mon Sep 17 00:00:00 2001
From: Florian Fainelli <florian@openwrt.org>
Date: Tue, 18 Aug 2009 17:53:21 +0200
Subject: [PATCH] alchemy: add au1000-eth platform device (v2)
This patch makes the board code register the au1000-eth
platform device. The au1000-eth platform data can be
overriden with the au1xxx_override_eth0_cfg function
like it has to be done for the Bosporus board.
Changes from v1:
- remove per-board platform.c file
- add an override function to pass custom eth0 platform_data PHY settings
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/arch/mips/alchemy/common/platform.c b/arch/mips/alchemy/common/platform.c
index 117f99f..559294a 100644
--- a/arch/mips/alchemy/common/platform.c
+++ b/arch/mips/alchemy/common/platform.c
@@ -19,6 +19,7 @@
#include <asm/mach-au1x00/au1xxx.h>
#include <asm/mach-au1x00/au1xxx_dbdma.h>
#include <asm/mach-au1x00/au1100_mmc.h>
+#include <asm/mach-au1x00/au1xxx_eth.h>
#define PORT(_base, _irq) \
{ \
@@ -331,6 +332,76 @@ static struct platform_device pbdb_smbus_device = {
};
#endif
+/* Macro to help defining the Ethernet MAC resources */
+#define MAC_RES(_base, _enable, _irq) \
+ { \
+ .start = CPHYSADDR(_base), \
+ .end = CPHYSADDR(_base + 0xffff), \
+ .flags = IORESOURCE_MEM, \
+ }, \
+ { \
+ .start = CPHYSADDR(_enable), \
+ .end = CPHYSADDR(_enable + 0x3), \
+ .flags = IORESOURCE_MEM, \
+ }, \
+ { \
+ .start = _irq, \
+ .end = _irq, \
+ .flags = IORESOURCE_IRQ \
+ }
+
+static struct resource au1xxx_eth0_resources[] = {
+#if defined(CONFIG_SOC_AU1000)
+ MAC_RES(AU1000_ETH0_BASE, AU1000_MAC0_ENABLE, AU1000_MAC0_DMA_INT),
+#elif defined(CONFIG_SOC_AU1100)
+ MAC_RES(AU1100_ETH0_BASE, AU1100_MAC0_ENABLE, AU1100_MAC0_DMA_INT),
+#elif defined(CONFIG_SOC_AU1550)
+ MAC_RES(AU1550_ETH0_BASE, AU1550_MAC0_ENABLE, AU1550_MAC0_DMA_INT),
+#elif defined(CONFIG_SOC_AU1500)
+ MAC_RES(AU1500_ETH0_BASE, AU1500_MAC0_ENABLE, AU1500_MAC0_DMA_INT),
+#endif
+};
+
+static struct resource au1xxx_eth1_resources[] = {
+#if defined(CONFIG_SOC_AU1000)
+ MAC_RES(AU1000_ETH1_BASE, AU1000_MAC1_ENABLE, AU1000_MAC1_DMA_INT),
+#elif defined(CONFIG_SOC_AU1550)
+ MAC_RES(AU1550_ETH1_BASE, AU1550_MAC1_ENABLE, AU1550_MAC1_DMA_INT),
+#elif defined(CONFIG_SOC_AU1500)
+ MAC_RES(AU1500_ETH1_BASE, AU1500_MAC1_ENABLE, AU1500_MAC1_DMA_INT),
+#endif
+};
+
+static struct au1000_eth_platform_data au1xxx_eth0_platform_data = {
+ .phy1_search_mac0 = 1,
+};
+
+static struct platform_device au1xxx_eth0_device = {
+ .name = "au1000-eth",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(au1xxx_eth0_resources),
+ .resource = au1xxx_eth0_resources,
+ .dev.platform_data = &au1xxx_eth0_platform_data,
+};
+
+#ifndef CONFIG_SOC_AU1100
+static struct platform_device au1xxx_eth1_device = {
+ .name = "au1000-eth",
+ .id = 1,
+ .num_resources = ARRAY_SIZE(au1xxx_eth1_resources),
+ .resource = au1xxx_eth1_resources,
+};
+#endif
+
+void __init au1xxx_override_eth0_cfg(struct au1000_eth_platform_data *eth_data)
+{
+ if (!eth_data)
+ return;
+
+ memcpy(&au1xxx_eth0_platform_data, eth_data,
+ sizeof(struct au1000_eth_platform_data));
+}
+
static struct platform_device *au1xxx_platform_devices[] __initdata = {
&au1xx0_uart_device,
&au1xxx_usb_ohci_device,
@@ -351,17 +422,25 @@ static struct platform_device *au1xxx_platform_devices[] __initdata = {
#ifdef SMBUS_PSC_BASE
&pbdb_smbus_device,
#endif
+ &au1xxx_eth0_device,
};
static int __init au1xxx_platform_init(void)
{
unsigned int uartclk = get_au1x00_uart_baud_base() * 16;
- int i;
+ int i, ni;
/* Fill up uartclk. */
for (i = 0; au1x00_uart_data[i].flags; i++)
au1x00_uart_data[i].uartclk = uartclk;
+ /* Register second MAC if enabled in pinfunc */
+#ifndef CONFIG_SOC_AU1100
+ ni = (int)((au_readl(SYS_PINFUNC) & (u32)(SYS_PF_NI2)) >> 4);
+ if (!(ni + 1))
+ platform_device_register(&au1xxx_eth1_device);
+#endif
+
return platform_add_devices(au1xxx_platform_devices,
ARRAY_SIZE(au1xxx_platform_devices));
}
diff --git a/arch/mips/alchemy/devboards/db1x00/board_setup.c b/arch/mips/alchemy/devboards/db1x00/board_setup.c
index de30d8e..4d2d32c 100644
--- a/arch/mips/alchemy/devboards/db1x00/board_setup.c
+++ b/arch/mips/alchemy/devboards/db1x00/board_setup.c
@@ -32,6 +32,7 @@
#include <asm/mach-au1x00/au1000.h>
#include <asm/mach-db1x00/db1x00.h>
+#include <asm/mach-au1x00/au1xxx_eth.h>
#include <prom.h>
@@ -134,6 +135,22 @@ void __init board_setup(void)
printk(KERN_INFO "AMD Alchemy Au1100/Db1100 Board\n");
#endif
#ifdef CONFIG_MIPS_BOSPORUS
+ struct au1000_eth_platform_data eth0_pdata;
+
+ /*
+ * Micrel/Kendin 5 port switch attached to MAC0,
+ * MAC0 is associated with PHY address 5 (== WAN port)
+ * MAC1 is not associated with any PHY, since it's connected directly
+ * to the switch.
+ * no interrupts are used
+ */
+ eth0_pdata.phy1_search_mac0 = 0;
+ eth0_pdata.phy_static_config = 1;
+ eth0_pdata.phy_addr = 5;
+ eth0_pdata.phy_busid = 0;
+
+ au1xxx_override_eth0_cfg(ð0_pdata);
+
printk(KERN_INFO "AMD Alchemy Bosporus Board\n");
#endif
#ifdef CONFIG_MIPS_MIRAGE
diff --git a/arch/mips/include/asm/mach-au1x00/au1xxx_eth.h b/arch/mips/include/asm/mach-au1x00/au1xxx_eth.h
new file mode 100644
index 0000000..876187e
--- /dev/null
+++ b/arch/mips/include/asm/mach-au1x00/au1xxx_eth.h
@@ -0,0 +1,17 @@
+#ifndef __AU1X00_ETH_DATA_H
+#define __AU1X00_ETH_DATA_H
+
+/* Platform specific PHY configuration passed to the MAC driver */
+struct au1000_eth_platform_data {
+ int phy_static_config;
+ int phy_search_highest_addr;
+ int phy1_search_mac0;
+ int phy_addr;
+ int phy_busid;
+ int phy_irq;
+};
+
+void __init au1xxx_override_eth0_cfg(struct au1000_eth_platform_data *eth_data);
+
+#endif /* __AU1X00_ETH_DATA_H */
+
--
1.6.3.rc3
^ permalink raw reply related
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