* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Alexander Duyck @ 2017-12-04 16:30 UTC (permalink / raw)
To: achiad shochat
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala,
Michael S. Tsirkin, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <CAEHy93LEr5h81KGfJBr5Z4ZKaeWEWwQL1LYfZNXoVF_HB6TQGw@mail.gmail.com>
On Mon, Dec 4, 2017 at 1:51 AM, achiad shochat
<achiad.mellanox@gmail.com> wrote:
> On 3 December 2017 at 19:35, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>> On Sun, 3 Dec 2017 11:14:37 +0200
>> achiad shochat <achiad.mellanox@gmail.com> wrote:
>>
>>> On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> > On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
>>> >> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
>>> >> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>>> >> > > Re. problem #2:
>>> >> > > Indeed the best way to address it seems to be to enslave the VF driver
>>> >> > > netdev under a persistent anchor netdev.
>>> >> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
>>> >> > > netdev to work in conjunction.
>>> >> > > And it's indeed desired that this enslavement logic work out-of-the box.
>>> >> > > But in case of PV+VF some configurable policies must be in place (and
>>> >> > > they'd better be generic rather than differ per PV technology).
>>> >> > > For example - based on which characteristics should the PV+VF coupling
>>> >> > > be done? netvsc uses MAC address, but that might not always be the
>>> >> > > desire.
>>> >> >
>>> >> > It's a policy but not guest userspace policy.
>>> >> >
>>> >> > The hypervisor certainly knows.
>>> >> >
>>> >> > Are you concerned that someone might want to create two devices with the
>>> >> > same MAC for an unrelated reason? If so, hypervisor could easily set a
>>> >> > flag in the virtio device to say "this is a backup, use MAC to find
>>> >> > another device".
>>> >>
>>> >> This is something I was going to suggest: a flag or other configuration on
>>> >> the virtio device to help control how this new feature is used. I can
>>> >> imagine this might be useful to control from either the hypervisor side or
>>> >> the VM side.
>>> >>
>>> >> The hypervisor might want to (1) disable it (force it off), (2) enable it
>>> >> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
>>> >> able to chose whether it wants to make use of the feature, or stick with the
>>> >> bonding solution.
>>> >>
>>> >> Either way, the kernel is making a feature available, and the user (VM or
>>> >> hypervisor) is able to control it by selecting the feature based on the
>>> >> policy desired.
>>> >>
>>> >> sln
>>> >
>>> > I'm not sure what's the feature that is available here.
>>> >
>>> > I saw this as a flag that says "this device shares backend with another
>>> > network device which can be found using MAC, and that backend should be
>>> > preferred". kernel then forces configuration which uses that other
>>> > backend - as long as it exists.
>>> >
>>> > However, please Cc virtio-dev mailing list if we are doing this since
>>> > this is a spec extension.
>>> >
>>> > --
>>> > MST
>>>
>>>
>>> Can someone please explain why assume a virtio device is there at all??
>>> I specified a case where there isn't any.
Migrating without any virtual device is going to be extremely
challenging, especially in any kind of virtualization setup where the
hosts are not homogeneous. By providing a virtio interface you can
guarantee that at least 1 network interface is available on any given
host, and then fail over to that as the least common denominator for
any migration.
>>> I second Jacob - having a netdev of one device driver enslave a netdev
>>> of another device driver is an awkward a-symmetric model.
>>> Regardless of whether they share the same backend device.
>>> Only I am not sure the Linux Bond is the right choice.
>>> e.g one may well want to use the virtio device also when the
>>> pass-through device is available, e.g for multicasts, east-west
>>> traffic, etc.
>>> I'm not sure the Linux Bond fits that functionality.
>>> And, as I hear in this thread, it is hard to make it work out of the box.
>>> So I think the right thing would be to write a new dedicated module
>>> for this purpose.
This part I can sort of agree with. What if we were to look at
providing a way to somehow advertise that the two devices were meant
to be boded for virtualization purposes? For now lets call it a
"virt-bond". Basically we could look at providing a means for virtio
and VF drivers to advertise that they want this sort of bond. Then it
would just be a matter of providing some sort of side channel to
indicate where you want things like multicast/broadcast/east-west
traffic to go.
>>> Re policy -
>>> Indeed the HV can request a policy from the guest but that's not a
>>> claim for the virtio device enslaving the pass-through device.
>>> Any policy can be queried by the upper enslaving device.
>>>
>>> Bottom line - I do not see a single reason to have the virtio netdev
>>> (nor netvsc or any other PV netdev) enslave another netdev by itself.
>>> If we'd do it right with netvsc from the beginning we wouldn't need
>>> this discussion at all...
>>
>> There are several issues with transparent migration.
>> The first is that the SR-IOV device needs to be shut off for earlier
>> in the migration process.
>
> That's not a given fact.
> It's due to the DMA and it should be solve anyway.
> Please read my first reply in this thread.
For now it is a fact. We would need to do a drastic rewrite of the DMA
API in the guest/host/QEMU/IOMMU in order to avoid it for now. So as a
first step I would say we should look at using this bonding type
solution. Being able to defer the VF eviction could be a next step for
all this as it would allow for much better performance, but we still
have too many cases where the VF might not be there after a migration.
>> Next, the SR-IOV device in the migrated go guest environment maybe different.
>> It might not exist at all, it might be at a different PCI address, or it
>> could even be a different vendor/speed/model.
>> Keeping a virtual network device around allows persisting the connectivity,
>> during the process.
>
> Right, but that virtual device must not relate to any para-virt
> specific technology (not netvsc, nor virtio).
> Again, it seems you did not read my first reply.
I would agree with the need to make this agnostic. Maybe we could look
at the current netvsc solution and find a way to make it generic so it
could be applied to any combination of paravirtual interface and PF.
^ permalink raw reply
* [PULL] vhost: cleanups and fixes
From: Michael S. Tsirkin @ 2017-12-04 13:25 UTC (permalink / raw)
To: Linus Torvalds
Cc: mhocko, kvm, mst, penguin-kernel, netdev, linux-kernel, stable,
virtualization, zhangweiping, marcandre.lureau, jstancek
The following changes since commit c1d0c3f623ada808904dec676da0126f5b800630:
fw_cfg: fix the command line module name (2017-11-14 23:57:40 +0200)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to d9e427f6ab8142d6868eb719e6a7851aafea56b6:
virtio_balloon: fix increment of vb->num_pfns in fill_balloon() (2017-12-01 16:55:45 +0200)
----------------------------------------------------------------
virtio,qemu: bugfixes
A couple of bugfixes that just became ready.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Jan Stancek (1):
virtio_balloon: fix increment of vb->num_pfns in fill_balloon()
Marc-André Lureau (1):
fw_cfg: fix driver remove
weiping zhang (1):
virtio: release virtio index when fail to device_register
drivers/firmware/qemu_fw_cfg.c | 3 ++-
drivers/virtio/virtio.c | 2 ++
drivers/virtio/virtio_balloon.c | 3 +--
3 files changed, 5 insertions(+), 3 deletions(-)
^ permalink raw reply
* Re: [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
From: Cornelia Huck @ 2017-12-04 10:24 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <20171201175140.GA19140@localhost.didichuxing.com>
On Sat, 2 Dec 2017 01:51:40 +0800
weiping zhang <zwp10758@gmail.com> wrote:
> cleanup all resource allocated by virtio_mmio_probe.
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 74dc717..3fd0e66 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> return -EBUSY;
>
> vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
> - if (!vm_dev)
> - return -ENOMEM;
> + if (!vm_dev) {
> + rc = -ENOMEM;
Touching this would be a good time to remove the extra space in front
of the -ENOMEM :)
> + goto free_mem;
> + }
>
> vm_dev->vdev.dev.parent = &pdev->dev;
> vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
(...)
> @@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, vm_dev);
>
> - return register_virtio_device(&vm_dev->vdev);
> + rc = register_virtio_device(&vm_dev->vdev);
> + if (rc)
> + goto unmap;
> + return 0;
> +unmap:
> + iounmap(vm_dev->base);
> +free_mem:
> + devm_release_mem_region(&pdev->dev, mem->start,
> + resource_size(mem));
> +free_vmdev:
> + devm_kfree(&pdev->dev, vm_dev);
I think this is problematic as vm_dev embeds a struct device (via
embedding a struct virtio_device). I think the right way to do this is
- call this only if register_virtio_device() has not been called
- put the devm_kfree() into the ->release callback for the
virtio_device (IOW, replace virtio_mmio_release_dev_empty() with a
function that calls this)
- do a put_device() if register_virtio_device() failed
I might be missing some interaction between the usual driver model
handling and devm resources, though.
> + return rc;
> }
>
> static int virtio_mmio_remove(struct platform_device *pdev)
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: achiad shochat @ 2017-12-04 9:51 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala,
Michael S. Tsirkin, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <20171203093524.26491cd0@xeon-e3>
On 3 December 2017 at 19:35, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Sun, 3 Dec 2017 11:14:37 +0200
> achiad shochat <achiad.mellanox@gmail.com> wrote:
>
>> On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
>> >> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
>> >> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>> >> > > Re. problem #2:
>> >> > > Indeed the best way to address it seems to be to enslave the VF driver
>> >> > > netdev under a persistent anchor netdev.
>> >> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
>> >> > > netdev to work in conjunction.
>> >> > > And it's indeed desired that this enslavement logic work out-of-the box.
>> >> > > But in case of PV+VF some configurable policies must be in place (and
>> >> > > they'd better be generic rather than differ per PV technology).
>> >> > > For example - based on which characteristics should the PV+VF coupling
>> >> > > be done? netvsc uses MAC address, but that might not always be the
>> >> > > desire.
>> >> >
>> >> > It's a policy but not guest userspace policy.
>> >> >
>> >> > The hypervisor certainly knows.
>> >> >
>> >> > Are you concerned that someone might want to create two devices with the
>> >> > same MAC for an unrelated reason? If so, hypervisor could easily set a
>> >> > flag in the virtio device to say "this is a backup, use MAC to find
>> >> > another device".
>> >>
>> >> This is something I was going to suggest: a flag or other configuration on
>> >> the virtio device to help control how this new feature is used. I can
>> >> imagine this might be useful to control from either the hypervisor side or
>> >> the VM side.
>> >>
>> >> The hypervisor might want to (1) disable it (force it off), (2) enable it
>> >> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
>> >> able to chose whether it wants to make use of the feature, or stick with the
>> >> bonding solution.
>> >>
>> >> Either way, the kernel is making a feature available, and the user (VM or
>> >> hypervisor) is able to control it by selecting the feature based on the
>> >> policy desired.
>> >>
>> >> sln
>> >
>> > I'm not sure what's the feature that is available here.
>> >
>> > I saw this as a flag that says "this device shares backend with another
>> > network device which can be found using MAC, and that backend should be
>> > preferred". kernel then forces configuration which uses that other
>> > backend - as long as it exists.
>> >
>> > However, please Cc virtio-dev mailing list if we are doing this since
>> > this is a spec extension.
>> >
>> > --
>> > MST
>>
>>
>> Can someone please explain why assume a virtio device is there at all??
>> I specified a case where there isn't any.
>>
>> I second Jacob - having a netdev of one device driver enslave a netdev
>> of another device driver is an awkward a-symmetric model.
>> Regardless of whether they share the same backend device.
>> Only I am not sure the Linux Bond is the right choice.
>> e.g one may well want to use the virtio device also when the
>> pass-through device is available, e.g for multicasts, east-west
>> traffic, etc.
>> I'm not sure the Linux Bond fits that functionality.
>> And, as I hear in this thread, it is hard to make it work out of the box.
>> So I think the right thing would be to write a new dedicated module
>> for this purpose.
>>
>> Re policy -
>> Indeed the HV can request a policy from the guest but that's not a
>> claim for the virtio device enslaving the pass-through device.
>> Any policy can be queried by the upper enslaving device.
>>
>> Bottom line - I do not see a single reason to have the virtio netdev
>> (nor netvsc or any other PV netdev) enslave another netdev by itself.
>> If we'd do it right with netvsc from the beginning we wouldn't need
>> this discussion at all...
>
> There are several issues with transparent migration.
> The first is that the SR-IOV device needs to be shut off for earlier
> in the migration process.
That's not a given fact.
It's due to the DMA and it should be solve anyway.
Please read my first reply in this thread.
> Next, the SR-IOV device in the migrated go guest environment maybe different.
> It might not exist at all, it might be at a different PCI address, or it
> could even be a different vendor/speed/model.
> Keeping a virtual network device around allows persisting the connectivity,
> during the process.
Right, but that virtual device must not relate to any para-virt
specific technology (not netvsc, nor virtio).
Again, it seems you did not read my first reply.
^ permalink raw reply
* Re: [PATCH] virtio: release virtio index when fail to device_register
From: Cornelia Huck @ 2017-12-04 9:38 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <20171201165539.GA11865@localhost.didichuxing.com>
On Sat, 2 Dec 2017 00:55:39 +0800
weiping zhang <zwp10758@gmail.com> wrote:
> On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:
> > We hold an extra reference to the struct device, even after a failed
> > register, and it is the responsibility of the caller to give up that
> > reference once no longer needed. As callers toregister_virtio_device()
> > embed the struct virtio_device, it needs to be their responsibility.
> > Looking at the existing callers,
> >
> > - ccw does a put_device
> > - pci, mmio and remoteproc do nothing, causing a leak
> > - vop does a free on the embedding structure, which is a big no-no
> >
> > Thoughts?
> Sorry to relay late and thanks for your review.
> Do you mean the "extra reference to the struct device" caused by the
> following code?
>
> err = device_register(&dev->dev);
> device_add(dev)
> get_device(dev)
> If I'm understand right, I think there is no extra reference if we fail
> virtio_register_device, because if device_register we don't get a
> reference.
The device_initialize() already gives you a reference. If device_add()
fails, it has cleaned up any additional reference it might have
obtained, but the initial reference is still there and needs to be
released by the caller.
^ permalink raw reply
* Re: [PATCH 1/3] vhost: fix skb leak in handle_rx()
From: Jason Wang @ 2017-12-04 7:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: mjrosato, netdev, wexu, linux-kernel, virtualization
In-Reply-To: <20171201163728-mutt-send-email-mst@kernel.org>
On 2017年12月01日 22:37, Michael S. Tsirkin wrote:
> On Fri, Dec 01, 2017 at 03:11:05PM +0800, Jason Wang wrote:
>>
>> On 2017年12月01日 13:54, wexu@redhat.com wrote:
>>> From: Wei Xu <wexu@redhat.com>
>>>
>>> Matthew found a roughly 40% tcp throughput regression with commit
>>> c67df11f(vhost_net: try batch dequing from skb array) as discussed
>>> in the following thread:
>>> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
>>>
>>> Eventually we figured out that it was a skb leak in handle_rx()
>>> when sending packets to the VM. This usually happens when a guest
>>> can not drain out vq as fast as vhost fills in, afterwards it sets
>>> off the traffic jam and leaks skb(s) which occurs as no headcount
>>> to send on the vq from vhost side.
>>>
>>> This can be avoided by making sure we have got enough headcount
>>> before actually consuming a skb from the batched rx array while
>>> transmitting, which is simply done by moving checking the zero
>>> headcount a bit ahead.
>>>
>>> Signed-off-by: Wei Xu <wexu@redhat.com>
>>> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>>> ---
>>> drivers/vhost/net.c | 20 ++++++++++----------
>>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>> index 8d626d7..c7bdeb6 100644
>>> --- a/drivers/vhost/net.c
>>> +++ b/drivers/vhost/net.c
>>> @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
>>> /* On error, stop handling until the next kick. */
>>> if (unlikely(headcount < 0))
>>> goto out;
>>> - if (nvq->rx_array)
>>> - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
>>> - /* On overrun, truncate and discard */
>>> - if (unlikely(headcount > UIO_MAXIOV)) {
>>> - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
>>> - err = sock->ops->recvmsg(sock, &msg,
>>> - 1, MSG_DONTWAIT | MSG_TRUNC);
>>> - pr_debug("Discarded rx packet: len %zd\n", sock_len);
>>> - continue;
>>> - }
>>> /* OK, now we need to know about added descriptors. */
>>> if (!headcount) {
>>> if (unlikely(vhost_enable_notify(&net->dev, vq))) {
>>> @@ -800,6 +790,16 @@ static void handle_rx(struct vhost_net *net)
>>> * they refilled. */
>>> goto out;
>>> }
>>> + if (nvq->rx_array)
>>> + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
>>> + /* On overrun, truncate and discard */
>>> + if (unlikely(headcount > UIO_MAXIOV)) {
>>> + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
>>> + err = sock->ops->recvmsg(sock, &msg,
>>> + 1, MSG_DONTWAIT | MSG_TRUNC);
>>> + pr_debug("Discarded rx packet: len %zd\n", sock_len);
>>> + continue;
>>> + }
>>> /* We don't need to be notified again. */
>>> iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
>>> fixup = msg.msg_iter;
>> I suggest to reorder this patch to 3/3.
>>
>> Thanks
> Why? This doesn't cause any new leaks, does it?
>
It doesn't, just think it can ease the downstream back porting in case
patch 2-3 were missed if somebody did a bisect and just backport patch 1.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v18 10/10] virtio-balloon: don't report free pages when page poisoning is enabled
From: Wei Wang @ 2017-12-04 5:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <20171201173951-mutt-send-email-mst@kernel.org>
On 12/01/2017 11:49 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 29, 2017 at 09:55:26PM +0800, Wei Wang wrote:
>> The guest free pages should not be discarded by the live migration thread
>> when page poisoning is enabled with PAGE_POISONING_NO_SANITY=n, because
>> skipping the transfer of such poisoned free pages will trigger false
>> positive when new pages are allocated and checked on the destination.
>> This patch skips the reporting of free pages in the above case.
>>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 035bd3a..6ac4cff 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -652,7 +652,9 @@ static void report_free_page(struct work_struct *work)
>> /* Start by sending the obtained cmd id to the host with an outbuf */
>> send_one_desc(vb, vb->free_page_vq, virt_to_phys(&vb->start_cmd_id),
>> sizeof(uint32_t), false, true, false);
>> - walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
>> + if (!(page_poisoning_enabled() &&
>> + !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY)))
>> + walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
>> /*
>> * End by sending the stop id to the host with an outbuf. Use the
>> * non-batching mode here to trigger a kick after adding the stop id.
> PAGE_POISONING_ZERO is actually OK.
I think the 0-filled pages still need to be sent. If the host on the
destination doesn't use PAGE_POISONING_ZERO, then the pages it offers to
the guest on the destination may have non-0 values.
>
> But I really would prefer it that we still send pages to host,
> otherwise debugging becomes much harder.
>
> And it does not have to be completely useless, even though
> you can not discard them as they would be zero-filled then.
>
> How about a config field telling host what should be there in the free
> pages? This way even though host can not discard them, host can send
> them out without reading them, still a win.
>
>
OK, but I think we would need two 32-bit config registers:
__u32 page_poison_val; // stores the PAGE_POISON VALUE, but it couldn't
indicate if page poisoning is in use
__u32 special_features; // set bit 0 to indicate page poisoning is in use
#define VIRTIO_BALLOON_SF_PAGE_POISON (1 << 0)
Best,
Wei
^ permalink raw reply
* Re: [PATCH v18 07/10] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wei Wang @ 2017-12-04 3:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <20171201171746-mutt-send-email-mst@kernel.org>
On 12/01/2017 11:38 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 29, 2017 at 09:55:23PM +0800, Wei Wang wrote:
>> +static void send_one_desc(struct virtio_balloon *vb,
>> + struct virtqueue *vq,
>> + uint64_t addr,
>> + uint32_t len,
>> + bool inbuf,
>> + bool batch)
>> +{
>> + int err;
>> + unsigned int size;
>> +
>> + /* Detach all the used buffers from the vq */
>> + while (virtqueue_get_buf(vq, &size))
>> + ;
>> +
>> + err = virtqueue_add_one_desc(vq, addr, len, inbuf, vq);
>> + /*
>> + * This is expected to never fail: there is always at least 1 entry
>> + * available on the vq, because when the vq is full the worker thread
>> + * that adds the desc will be put into sleep until at least 1 entry is
>> + * available to use.
>> + */
>> + BUG_ON(err);
>> +
>> + /* If batching is requested, we batch till the vq is full */
>> + if (!batch || !vq->num_free)
>> + kick_and_wait(vq, vb->acked);
>> +}
>> +
> This internal kick complicates callers. I suggest that instead,
> you move this to callers, just return a "kick required" boolean.
> This way callers do not need to play with num_free at all.
Then in what situation would the function return true of "kick required"?
I think this wouldn't make a difference fundamentally. For example, we
have 257 sgs (batching size=256) to send to host:
while (i < 257) {
kick_required = send_sgs();
if (kick_required)
kick(); // After the 256 sgs have been added, the caller
performs a kick().
}
Do we still need a kick here for the 257th sg as before? Only the caller
knows if the last added sgs need a kick (when the send_sgs receives one
sg, it doesn't know if there are more to come).
There is another approach to checking if the last added sgs haven't been
sync-ed to the host: expose "vring_virtqueue->num_added" to the caller
via a virtio_ring API:
unsigned int virtqueue_num_added(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
return vq->num_added;
}
>> +/*
>> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
>> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
>> + * The page xbitmap is searched for continuous "1" bits, which correspond
>> + * to continuous pages, to chunk into sgs.
>> + *
>> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
>> + * need to be searched.
>> + */
>> +static void tell_host_sgs(struct virtio_balloon *vb,
>> + struct virtqueue *vq,
>> + unsigned long page_xb_start,
>> + unsigned long page_xb_end)
>> +{
>> + unsigned long pfn_start, pfn_end;
>> + uint64_t addr;
>> + uint32_t len, max_len = round_down(UINT_MAX, PAGE_SIZE);
>> +
>> + pfn_start = page_xb_start;
>> + while (pfn_start < page_xb_end) {
>> + pfn_start = xb_find_next_set_bit(&vb->page_xb, pfn_start,
>> + page_xb_end);
>> + if (pfn_start == page_xb_end + 1)
>> + break;
>> + pfn_end = xb_find_next_zero_bit(&vb->page_xb,
>> + pfn_start + 1,
>> + page_xb_end);
>> + addr = pfn_start << PAGE_SHIFT;
>> + len = (pfn_end - pfn_start) << PAGE_SHIFT;
> This assugnment can overflow. Next line compares with UINT_MAX but by
> that time it is too late. I think you should do all math in 64 bit to
> avoid surprises, then truncate to max_len and then it's safe to assign
> to sg.
Sounds reasonable, thanks.
>> +
>> + xb_clear_bit_range(&vb->page_xb, page_xb_start, page_xb_end);
>> +}
>> +
>> +static inline int xb_set_page(struct virtio_balloon *vb,
>> + struct page *page,
>> + unsigned long *pfn_min,
>> + unsigned long *pfn_max)
>> +{
>> + unsigned long pfn = page_to_pfn(page);
>> + int ret;
>> +
>> + *pfn_min = min(pfn, *pfn_min);
>> + *pfn_max = max(pfn, *pfn_max);
>> +
>> + do {
>> + ret = xb_preload_and_set_bit(&vb->page_xb, pfn,
>> + GFP_NOWAIT | __GFP_NOWARN);
>> + } while (unlikely(ret == -EAGAIN));
> what exactly does this loop do? Does this wait
> forever until there is some free memory? why GFP_NOWAIT?
Basically, "-EAGAIN" is returned from xb_set_bit() in the case when the
pre-allocated per-cpu ida_bitmap is NULL. In that case, the caller
re-invokes xb_preload_and_set_bit(), which re-invokes xb_preload to
allocate ida_bitmap. So "-EAGAIN" actually does not indicate a status
about memory allocation. "-ENOMEM" is the one to indicate the failure of
memory allocation, but the loop doesn't re-try on "-ENOMEM".
GFP_NOWAIT is used to avoid memory reclaiming, which could cause the
deadlock issue we discussed before.
> return num_freed_pages;
> }
>
> +/*
> + * The regular leak_balloon() with VIRTIO_BALLOON_F_SG needs memory allocation
> + * for xbitmap, which is not suitable for the oom case. This function does not
> + * use xbitmap to chunk pages, so it can be used by oom notifier to deflate
> + * pages when VIRTIO_BALLOON_F_SG is negotiated.
> + */
> I guess we can live with this for now.
Agree, the patchset has been big. We can get the basic implementation in
first, and leave the following as future work. I can add it in the
commit log.
> Two things to consider
> - adding support for pre-allocating indirect buffers
> - sorting the internal page queue (how?)
Best,
Wei
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Stephen Hemminger @ 2017-12-03 17:35 UTC (permalink / raw)
To: achiad shochat
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala,
Michael S. Tsirkin, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <CAEHy93+yhRtTQub0xk5W1jKRNisrcRBtNexcwRfWdSKPm5a4Gw@mail.gmail.com>
On Sun, 3 Dec 2017 11:14:37 +0200
achiad shochat <achiad.mellanox@gmail.com> wrote:
> On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
> >> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
> >> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
> >> > > Re. problem #2:
> >> > > Indeed the best way to address it seems to be to enslave the VF driver
> >> > > netdev under a persistent anchor netdev.
> >> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
> >> > > netdev to work in conjunction.
> >> > > And it's indeed desired that this enslavement logic work out-of-the box.
> >> > > But in case of PV+VF some configurable policies must be in place (and
> >> > > they'd better be generic rather than differ per PV technology).
> >> > > For example - based on which characteristics should the PV+VF coupling
> >> > > be done? netvsc uses MAC address, but that might not always be the
> >> > > desire.
> >> >
> >> > It's a policy but not guest userspace policy.
> >> >
> >> > The hypervisor certainly knows.
> >> >
> >> > Are you concerned that someone might want to create two devices with the
> >> > same MAC for an unrelated reason? If so, hypervisor could easily set a
> >> > flag in the virtio device to say "this is a backup, use MAC to find
> >> > another device".
> >>
> >> This is something I was going to suggest: a flag or other configuration on
> >> the virtio device to help control how this new feature is used. I can
> >> imagine this might be useful to control from either the hypervisor side or
> >> the VM side.
> >>
> >> The hypervisor might want to (1) disable it (force it off), (2) enable it
> >> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
> >> able to chose whether it wants to make use of the feature, or stick with the
> >> bonding solution.
> >>
> >> Either way, the kernel is making a feature available, and the user (VM or
> >> hypervisor) is able to control it by selecting the feature based on the
> >> policy desired.
> >>
> >> sln
> >
> > I'm not sure what's the feature that is available here.
> >
> > I saw this as a flag that says "this device shares backend with another
> > network device which can be found using MAC, and that backend should be
> > preferred". kernel then forces configuration which uses that other
> > backend - as long as it exists.
> >
> > However, please Cc virtio-dev mailing list if we are doing this since
> > this is a spec extension.
> >
> > --
> > MST
>
>
> Can someone please explain why assume a virtio device is there at all??
> I specified a case where there isn't any.
>
> I second Jacob - having a netdev of one device driver enslave a netdev
> of another device driver is an awkward a-symmetric model.
> Regardless of whether they share the same backend device.
> Only I am not sure the Linux Bond is the right choice.
> e.g one may well want to use the virtio device also when the
> pass-through device is available, e.g for multicasts, east-west
> traffic, etc.
> I'm not sure the Linux Bond fits that functionality.
> And, as I hear in this thread, it is hard to make it work out of the box.
> So I think the right thing would be to write a new dedicated module
> for this purpose.
>
> Re policy -
> Indeed the HV can request a policy from the guest but that's not a
> claim for the virtio device enslaving the pass-through device.
> Any policy can be queried by the upper enslaving device.
>
> Bottom line - I do not see a single reason to have the virtio netdev
> (nor netvsc or any other PV netdev) enslave another netdev by itself.
> If we'd do it right with netvsc from the beginning we wouldn't need
> this discussion at all...
There are several issues with transparent migration.
The first is that the SR-IOV device needs to be shut off for earlier
in the migration process.
Next, the SR-IOV device in the migrated go guest environment maybe different.
It might not exist at all, it might be at a different PCI address, or it
could even be a different vendor/speed/model.
Keeping a virtual network device around allows persisting the connectivity,
during the process.
^ permalink raw reply
* Re: Memory corruption in powerpc guests with virtio_balloon (was Re: [PATCH v3] virtio_balloon: fix deadlock on OOM)
From: Michael Ellerman @ 2017-12-03 10:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michal Hocko, Tetsuo Handa, linux-kernel, virtualization,
linux-mm, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20171201164129-mutt-send-email-mst@kernel.org>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Dec 01, 2017 at 11:31:08PM +1100, Michael Ellerman wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>> > fill_balloon doing memory allocations under balloon_lock
>> > can cause a deadlock when leak_balloon is called from
>> > virtballoon_oom_notify and tries to take same lock.
...
>>
>>
>> Somehow this commit seems to be killing powerpc guests.
...
>
> Thanks for the report!
> A fix was just posted:
> virtio_balloon: fix increment of vb->num_pfns in fill_balloon()
>
> Would appreciate testing.
Yep that fixes it. Thanks.
Tested-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: achiad shochat @ 2017-12-03 9:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala, netdev,
virtualization, Achiad, Peter Waskiewicz Jr, Singhai, Anjali,
Shannon Nelson, Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171203065759-mutt-send-email-mst@kernel.org>
On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
>> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
>> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>> > > Re. problem #2:
>> > > Indeed the best way to address it seems to be to enslave the VF driver
>> > > netdev under a persistent anchor netdev.
>> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
>> > > netdev to work in conjunction.
>> > > And it's indeed desired that this enslavement logic work out-of-the box.
>> > > But in case of PV+VF some configurable policies must be in place (and
>> > > they'd better be generic rather than differ per PV technology).
>> > > For example - based on which characteristics should the PV+VF coupling
>> > > be done? netvsc uses MAC address, but that might not always be the
>> > > desire.
>> >
>> > It's a policy but not guest userspace policy.
>> >
>> > The hypervisor certainly knows.
>> >
>> > Are you concerned that someone might want to create two devices with the
>> > same MAC for an unrelated reason? If so, hypervisor could easily set a
>> > flag in the virtio device to say "this is a backup, use MAC to find
>> > another device".
>>
>> This is something I was going to suggest: a flag or other configuration on
>> the virtio device to help control how this new feature is used. I can
>> imagine this might be useful to control from either the hypervisor side or
>> the VM side.
>>
>> The hypervisor might want to (1) disable it (force it off), (2) enable it
>> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
>> able to chose whether it wants to make use of the feature, or stick with the
>> bonding solution.
>>
>> Either way, the kernel is making a feature available, and the user (VM or
>> hypervisor) is able to control it by selecting the feature based on the
>> policy desired.
>>
>> sln
>
> I'm not sure what's the feature that is available here.
>
> I saw this as a flag that says "this device shares backend with another
> network device which can be found using MAC, and that backend should be
> preferred". kernel then forces configuration which uses that other
> backend - as long as it exists.
>
> However, please Cc virtio-dev mailing list if we are doing this since
> this is a spec extension.
>
> --
> MST
Can someone please explain why assume a virtio device is there at all??
I specified a case where there isn't any.
I second Jacob - having a netdev of one device driver enslave a netdev
of another device driver is an awkward a-symmetric model.
Regardless of whether they share the same backend device.
Only I am not sure the Linux Bond is the right choice.
e.g one may well want to use the virtio device also when the
pass-through device is available, e.g for multicasts, east-west
traffic, etc.
I'm not sure the Linux Bond fits that functionality.
And, as I hear in this thread, it is hard to make it work out of the box.
So I think the right thing would be to write a new dedicated module
for this purpose.
Re policy -
Indeed the HV can request a policy from the guest but that's not a
claim for the virtio device enslaving the pass-through device.
Any policy can be queried by the upper enslaving device.
Bottom line - I do not see a single reason to have the virtio netdev
(nor netvsc or any other PV netdev) enslave another netdev by itself.
If we'd do it right with netvsc from the beginning we wouldn't need
this discussion at all...
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-12-03 5:05 UTC (permalink / raw)
To: Shannon Nelson
Cc: Jakub Kicinski, Hannes Frederic Sowa, achiad shochat,
Sridhar Samudrala, netdev, virtualization, Achiad,
Peter Waskiewicz Jr, Singhai, Anjali, Andy Gospodarek, Or Gerlitz
In-Reply-To: <780cf854-83e1-b2a4-0f07-74372a972ca6@oracle.com>
On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
> > > Re. problem #2:
> > > Indeed the best way to address it seems to be to enslave the VF driver
> > > netdev under a persistent anchor netdev.
> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
> > > netdev to work in conjunction.
> > > And it's indeed desired that this enslavement logic work out-of-the box.
> > > But in case of PV+VF some configurable policies must be in place (and
> > > they'd better be generic rather than differ per PV technology).
> > > For example - based on which characteristics should the PV+VF coupling
> > > be done? netvsc uses MAC address, but that might not always be the
> > > desire.
> >
> > It's a policy but not guest userspace policy.
> >
> > The hypervisor certainly knows.
> >
> > Are you concerned that someone might want to create two devices with the
> > same MAC for an unrelated reason? If so, hypervisor could easily set a
> > flag in the virtio device to say "this is a backup, use MAC to find
> > another device".
>
> This is something I was going to suggest: a flag or other configuration on
> the virtio device to help control how this new feature is used. I can
> imagine this might be useful to control from either the hypervisor side or
> the VM side.
>
> The hypervisor might want to (1) disable it (force it off), (2) enable it
> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
> able to chose whether it wants to make use of the feature, or stick with the
> bonding solution.
>
> Either way, the kernel is making a feature available, and the user (VM or
> hypervisor) is able to control it by selecting the feature based on the
> policy desired.
>
> sln
I'm not sure what's the feature that is available here.
I saw this as a flag that says "this device shares backend with another
network device which can be found using MAC, and that backend should be
preferred". kernel then forces configuration which uses that other
backend - as long as it exists.
However, please Cc virtio-dev mailing list if we are doing this since
this is a spec extension.
--
MST
^ permalink raw reply
* Re: [PATCH net,stable v4 0/3] vhost: fix a few skb leaks
From: David Miller @ 2017-12-03 2:32 UTC (permalink / raw)
To: wexu; +Cc: mjrosato, mst, netdev, linux-kernel, virtualization
In-Reply-To: <1512123038-15773-1-git-send-email-wexu@redhat.com>
From: wexu@redhat.com
Date: Fri, 1 Dec 2017 05:10:35 -0500
> Matthew found a roughly 40% tcp throughput regression with commit
> c67df11f(vhost_net: try batch dequing from skb array) as discussed
> in the following thread:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
Series applied and queued up for -stable.
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Shannon Nelson @ 2017-12-01 20:08 UTC (permalink / raw)
To: Michael S. Tsirkin, achiad shochat
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala, netdev,
virtualization, Achiad, Peter Waskiewicz Jr, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171130155612-mutt-send-email-mst@kernel.org>
On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
> On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>> Re. problem #2:
>> Indeed the best way to address it seems to be to enslave the VF driver
>> netdev under a persistent anchor netdev.
>> And it's indeed desired to allow (but not enforce) PV netdev and VF
>> netdev to work in conjunction.
>> And it's indeed desired that this enslavement logic work out-of-the box.
>> But in case of PV+VF some configurable policies must be in place (and
>> they'd better be generic rather than differ per PV technology).
>> For example - based on which characteristics should the PV+VF coupling
>> be done? netvsc uses MAC address, but that might not always be the
>> desire.
>
> It's a policy but not guest userspace policy.
>
> The hypervisor certainly knows.
>
> Are you concerned that someone might want to create two devices with the
> same MAC for an unrelated reason? If so, hypervisor could easily set a
> flag in the virtio device to say "this is a backup, use MAC to find
> another device".
This is something I was going to suggest: a flag or other configuration
on the virtio device to help control how this new feature is used. I
can imagine this might be useful to control from either the hypervisor
side or the VM side.
The hypervisor might want to (1) disable it (force it off), (2) enable
it for VM choice, or (3) force it on for the VM. In case (2), the VM
might be able to chose whether it wants to make use of the feature, or
stick with the bonding solution.
Either way, the kernel is making a feature available, and the user (VM
or hypervisor) is able to control it by selecting the feature based on
the policy desired.
sln
^ permalink raw reply
* [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-01 19:51 UTC (permalink / raw)
To: linux-kernel
Cc: tglx, kvm, Michael S. Tsirkin, peterz, fweisbec, jiangshanlai,
josh, rostedt, oleg, dhowells, edumazet, netdev,
mathieu.desnoyers, dipankar, akpm, Paul E. McKenney,
virtualization, mingo
In-Reply-To: <20171201195053.GA23494@linux.vnet.ibm.com>
Because READ_ONCE() now implies read_barrier_depends(), the
read_barrier_depends() in next_desc() is now redundant. This commit
therefore removes it and the related comments.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: <kvm@vger.kernel.org>
Cc: <virtualization@lists.linux-foundation.org>
Cc: <netdev@vger.kernel.org>
---
drivers/vhost/vhost.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 33ac2b186b85..78b5940a415a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1877,12 +1877,7 @@ static unsigned next_desc(struct vhost_virtqueue *vq, struct vring_desc *desc)
return -1U;
/* Check they're not leading us off end of descriptors. */
- next = vhost16_to_cpu(vq, desc->next);
- /* Make sure compiler knows to grab that: we don't want it changing! */
- /* We will use the result as an index in an array, so most
- * architectures only need a compiler barrier here. */
- read_barrier_depends();
-
+ next = vhost16_to_cpu(vq, READ_ONCE(desc->next));
return next;
}
--
2.5.2
^ permalink raw reply related
* [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
From: weiping zhang @ 2017-12-01 17:51 UTC (permalink / raw)
To: mst, jasowang, cohuck; +Cc: virtualization
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..3fd0e66 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev)
- return -ENOMEM;
+ if (!vm_dev) {
+ rc = -ENOMEM;
+ goto free_mem;
+ }
vm_dev->vdev.dev.parent = &pdev->dev;
vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
@@ -524,14 +526,17 @@ static int virtio_mmio_probe(struct platform_device *pdev)
spin_lock_init(&vm_dev->lock);
vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL)
- return -EFAULT;
+ if (vm_dev->base == NULL) {
+ rc = -EFAULT;
+ goto free_vmdev;
+ }
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
/* Check device version */
@@ -539,7 +544,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(&pdev->dev, "Version %ld not supported!\n",
vm_dev->version);
- return -ENXIO;
+ rc = -ENXIO;
+ goto unmap;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -548,7 +554,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
* virtio-mmio device with an ID 0 is a (dummy) placeholder
* with no function. End probing now with no error reported.
*/
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
- return register_virtio_device(&vm_dev->vdev);
+ rc = register_virtio_device(&vm_dev->vdev);
+ if (rc)
+ goto unmap;
+ return 0;
+unmap:
+ iounmap(vm_dev->base);
+free_mem:
+ devm_release_mem_region(&pdev->dev, mem->start,
+ resource_size(mem));
+free_vmdev:
+ devm_kfree(&pdev->dev, vm_dev);
+ return rc;
}
static int virtio_mmio_remove(struct platform_device *pdev)
--
2.9.4
^ permalink raw reply related
* Re: [PATCH] virtio: release virtio index when fail to device_register
From: weiping zhang @ 2017-12-01 17:30 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171201165539.GA11865@localhost.didichuxing.com>
2017-12-02 0:55 GMT+08:00 weiping zhang <zwp10758@gmail.com>:
> On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:
>> On Wed, 29 Nov 2017 09:23:01 +0800
>> weiping zhang <zwp10758@gmail.com> wrote:
>>
>> > index can be reused by other virtio device.
>> >
>> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>
>> > ---
>> > drivers/virtio/virtio.c | 2 ++
>> > 1 file changed, 2 insertions(+)
>> >
>> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> > index 48230a5..bf7ff39 100644
>> > --- a/drivers/virtio/virtio.c
>> > +++ b/drivers/virtio/virtio.c
>> > @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
>> > /* device_register() causes the bus infrastructure to look for a
>> > * matching driver. */
>> > err = device_register(&dev->dev);
>> > + if (err)
>> > + ida_simple_remove(&virtio_index_ida, dev->index);
>> > out:
>> > if (err)
>> > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>>
>> I think your patch is correct (giving up the index if we failed to
>> register), but this made me look at our error handling if a virtio
>> device failed to register.
>>
>> We hold an extra reference to the struct device, even after a failed
>> register, and it is the responsibility of the caller to give up that
>> reference once no longer needed. As callers toregister_virtio_device()
>> embed the struct virtio_device, it needs to be their responsibility.
>> Looking at the existing callers,
>>
>> - ccw does a put_device
>> - pci, mmio and remoteproc do nothing, causing a leak
>> - vop does a free on the embedding structure, which is a big no-no
>>
>> Thoughts?
> Sorry to relay late and thanks for your review.
> Do you mean the "extra reference to the struct device" caused by the
> following code?
>
> err = device_register(&dev->dev);
> device_add(dev)
> get_device(dev)
> If I'm understand right, I think there is no extra reference if we fail
> virtio_register_device, because if device_register we don't get a
> reference.
I go through these callers, virtio_mmio_probe should do more cleanup.
I'll sent a patch fix that.
> --
> Thanks
> weiping
^ permalink raw reply
* Re: [PATCH v18 05/10] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-01 17:25 UTC (permalink / raw)
To: Wang, Wei W
Cc: yang.zhang.wz@gmail.com, kvm@vger.kernel.org, mst@redhat.com,
Tetsuo Handa, liliang.opensource@gmail.com, qemu-devel@nongnu.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
mawilcox@microsoft.com, quan.xu@aliyun.com, nilal@redhat.com,
riel@redhat.com, cornelia.huck@de.ibm.com, mhocko@kernel.org,
linux-kernel@vger.kernel.org, amit.sh
In-Reply-To: <286AC319A985734F985F78AFA26841F739376DA1@shsmsx102.ccr.corp.intel.com>
On Fri, Dec 01, 2017 at 03:09:08PM +0000, Wang, Wei W wrote:
> On Friday, December 1, 2017 9:02 PM, Tetsuo Handa wrote:
> > If start == end is legal,
> >
> > for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
> >
> > makes this loop do nothing because 10 < 10 is false.
>
> How about "start <= end "?
Don't ask Tetsuo for his opinion, write some userspace code that uses it.
^ permalink raw reply
* Re: [PATCH] virtio: release virtio index when fail to device_register
From: weiping zhang @ 2017-12-01 16:55 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171129105044.3a4b0ab3.cohuck@redhat.com>
On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:
> On Wed, 29 Nov 2017 09:23:01 +0800
> weiping zhang <zwp10758@gmail.com> wrote:
>
> > index can be reused by other virtio device.
> >
> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> > ---
> > drivers/virtio/virtio.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 48230a5..bf7ff39 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
> > /* device_register() causes the bus infrastructure to look for a
> > * matching driver. */
> > err = device_register(&dev->dev);
> > + if (err)
> > + ida_simple_remove(&virtio_index_ida, dev->index);
> > out:
> > if (err)
> > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>
> I think your patch is correct (giving up the index if we failed to
> register), but this made me look at our error handling if a virtio
> device failed to register.
>
> We hold an extra reference to the struct device, even after a failed
> register, and it is the responsibility of the caller to give up that
> reference once no longer needed. As callers toregister_virtio_device()
> embed the struct virtio_device, it needs to be their responsibility.
> Looking at the existing callers,
>
> - ccw does a put_device
> - pci, mmio and remoteproc do nothing, causing a leak
> - vop does a free on the embedding structure, which is a big no-no
>
> Thoughts?
Sorry to relay late and thanks for your review.
Do you mean the "extra reference to the struct device" caused by the
following code?
err = device_register(&dev->dev);
device_add(dev)
get_device(dev)
If I'm understand right, I think there is no extra reference if we fail
virtio_register_device, because if device_register we don't get a
reference.
--
Thanks
weiping
^ permalink raw reply
* Re: [PATCH v18 10/10] virtio-balloon: don't report free pages when page poisoning is enabled
From: Michael S. Tsirkin @ 2017-12-01 15:49 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1511963726-34070-11-git-send-email-wei.w.wang@intel.com>
On Wed, Nov 29, 2017 at 09:55:26PM +0800, Wei Wang wrote:
> The guest free pages should not be discarded by the live migration thread
> when page poisoning is enabled with PAGE_POISONING_NO_SANITY=n, because
> skipping the transfer of such poisoned free pages will trigger false
> positive when new pages are allocated and checked on the destination.
> This patch skips the reporting of free pages in the above case.
>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> ---
> drivers/virtio/virtio_balloon.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 035bd3a..6ac4cff 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -652,7 +652,9 @@ static void report_free_page(struct work_struct *work)
> /* Start by sending the obtained cmd id to the host with an outbuf */
> send_one_desc(vb, vb->free_page_vq, virt_to_phys(&vb->start_cmd_id),
> sizeof(uint32_t), false, true, false);
> - walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> + if (!(page_poisoning_enabled() &&
> + !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY)))
> + walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> /*
> * End by sending the stop id to the host with an outbuf. Use the
> * non-batching mode here to trigger a kick after adding the stop id.
PAGE_POISONING_ZERO is actually OK.
But I really would prefer it that we still send pages to host,
otherwise debugging becomes much harder.
And it does not have to be completely useless, even though
you can not discard them as they would be zero-filled then.
How about a config field telling host what should be there in the free
pages? This way even though host can not discard them, host can send
them out without reading them, still a win.
> --
> 2.7.4
^ permalink raw reply
* Re: [RFC] virtio-iommu version 0.5
From: Jean-Philippe Brucker @ 2017-12-01 15:46 UTC (permalink / raw)
To: iommu@lists.linux-foundation.org, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: Lorenzo Pieralisi, ashok.raj@intel.com, mst@redhat.com,
Marc Zyngier, Will Deacon, Jayachandran.Nair@cavium.com,
eric.auger@redhat.com, Robin Murphy, eric.auger.pro@gmail.com
In-Reply-To: <20171023093241.20113-1-jean-philippe.brucker@arm.com>
Hello,
Since we still have time to introduce disruptive changes, I'm
considering changing the UNMAP parameters slightly. Behavior stays the
same, but instead of passing virt_addr and size, we pass virt_start and
virt_end:
struct virtio_iommu_req_unmap {
le32 domain;
le64 virt_start;
le64 virt_end;
le32 reserved;
};
And for symmetry, also change MAP:
struct virtio_iommu_req_map {
le32 domain;
le64 phys_start;
le64 virt_start;
le64 virt_end;
le32 flags;
};
This would allow to express the full 64-bit range in MAP and UNMAP
requests. Currently the MAP description dismisses this case with "just
use VIRTIO_IOMMU_F_BYPASS if you need an identity mapping", and I think
it still holds. But for UNMAP it can be useful to send a single request
covering the full address space instead of lots of individual requests.
I had this problem when implementing ATTACH/DETACH in kvmtool, because
VFIO doesn't have an explicit unmap-all command (and kvmtool doesn't
keep track of the mappings). When changing a device's domain, I had to
send two unmap commands, one for each half of the address space. It's
not a huge problem, just a bit inconvenient.
I don't see how unmap-all would be useful for virtio-iommu at the moment
(since detaching the domain unmaps all) but just like it turns out to be
desirable in VFIO, I'm sure someone will need it in virtio-iommu one
day.
Alternatively we could change \field{reserved} of the unmap request into
\field{flags} and add an UNMAP_ALL flag. This is backward-compatible and
less invasive. Introducing a new UNMAP_ALL request would probably be
cleaner though. In any case, I personally prefer start and end
parameter, it looks nicer. The changeset below looks scary, but it's
mostly reformatting.
Please let me know if you have any objection, or other comment for 0.5.
Thanks,
Jean
--- 8< ---
Subject: [PATCH] Change MAP and UNMAP parameters
Passing start/end instead of start/size to MAP and UNMAP offers more
flexibility. UNMAP can now be used to unmap the whole address space in
one go. UNMAP(domain, 0, ~0ULL) should now remove all mappings.
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
device-operations.tex | 124 +++++++++++++++++++++++++-------------------------
1 file changed, 61 insertions(+), 63 deletions(-)
diff --git a/device-operations.tex b/device-operations.tex
index 9b35283..3ecafd3 100644
--- a/device-operations.tex
+++ b/device-operations.tex
@@ -21,15 +21,15 @@ types:
\texttt{attach(device = 0x104, domain = 1)}
\item Create a mapping between a range of guest-virtual and guest-physical
address. \\
- \texttt{map(domain = 1, virt = 0x1000, phys = 0xa000,
- size = 0x1000, flags = READ)}
+ \texttt{map(domain = 1, virt_start = 0x1000, virt_end = 0x1fff,
+ phys = 0xa000, flags = READ)}
Endpoint 0x104, for example a hardware PCI endpoint, can now read at
addresses 0x1000-0x1fff. These accesses are translated into
system-physical addresses by the IOMMU.
\item Remove the mapping.\\
- \texttt{unmap(domain = 1, virt = 0x1000, size = 0x1000)}
+ \texttt{unmap(domain = 1, virt_start= 0x1000, virt_end = 0x1fff)}
Any access to addresses 0x1000-0x1fff by endpoint 0x104 would now be
rejected.
@@ -286,8 +286,9 @@ written to it, the driver SHOULD interpret it as a failure from the device
to parse the request.
If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered, the driver SHOULD
-NOT send requests with \field{virt_addr} less than
-\field{input_range.start} or greater than \field{input_range.end}.
+NOT send requests with \field{virt_start} less than
+\field{input_range.start} or \field{virt_end} greater than
+\field{input_range.end}.
If the VIRTIO_IOMMU_F_DOMAIN_BITS feature is offered, the driver SHOULD
NOT send requests with \field{domain} greater than the size described by
@@ -321,7 +322,7 @@ The device MUST ignore reserved fields of the head and the tail of a
request.
If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered, the device MUST
-truncate the range described by \field{virt_addr} and \field{size} in
+truncate the range described by \field{virt_start} and \field{virt_end} in
requests to fit in the range described by \field{input_range}.
If the VIRTIO_IOMMU_F_DOMAIN_BITS is offered, the device MUST ignore bits
@@ -446,9 +447,9 @@ endpoint cannot access any mapping from that domain.
\begin{lstlisting}
struct virtio_iommu_req_map {
le32 domain;
- le64 phys_addr;
- le64 virt_addr;
- le64 size;
+ le64 phys_start;
+ le64 virt_start;
+ le64 virt_end;
le32 flags;
};
@@ -461,30 +462,21 @@ struct virtio_iommu_req_map {
Map a range of virtually-contiguous addresses to a range of
physically-contiguous addresses of the same size. After the request
succeeds, all endpoints attached to this domain can access memory in the
-range $[phys\_addr; phys\_addr + size[$. For example, if an endpoint
-accesses address $VA \in [virt\_addr; virt\_addr + size[$, the device (or
-the physical IOMMU) translates the address: $PA = VA - virt\_addr +
-phys\_addr$. If the access parameters are compatible with \field{flags}
-(for instance, the access is write and \field{flags} are
-VIRTIO_IOMMU_MAP_F_READ | VIRTIO_IOMMU_MAP_F_WRITE) then the IOMMU allows
-the access to reach $PA$.
-
-The range defined by (\field{virt_addr}, \field{size}) must be within the
-limits specified by \field{input_range}. The range defined by
-(\field{phys_addr}, \field{size}) must be within the guest-physical
-address space. This includes upper and lower limits, as well as any
-carving of guest-physical addresses for use by the host (for instance MSI
-doorbells). Guest physical boundaries are set by the host using a firmware
-mechanism outside the scope of this specification.
-
-\begin{note}
-This format prevents from creating the identity mapping in a single
-request \texttt{[0x0; 0xfff....fff] $\rightarrow$ [0x0; 0xfff...fff]},
-since it would result in a size of zero. Hopefully allowing
-VIRTIO_IOMMU_F_BYPASS eliminates the need for issuing such request. It
-would also be unlikely to conform to the physical range restrictions
-from the previous paragraph.
-\end{note}
+range $[virt\_start; virt\_end]$. For example, if an endpoint accesses
+address $VA \in [virt\_start; virt\_end]$, the device (or the physical
+IOMMU) translates the address: $PA = VA - virt\_start + phys\_start$. If
+the access parameters are compatible with \field{flags} (for instance, the
+access is write and \field{flags} are VIRTIO_IOMMU_MAP_F_READ |
+VIRTIO_IOMMU_MAP_F_WRITE) then the IOMMU allows the access to reach $PA$.
+
+The range defined by \field{virt_start} and \field{virt_end} should be
+within the limits specified by \field{input_range}. Given $phys\_end =
+phys\_start + virt\_end - virt\_start$, the range defined by
+\field{phys_start} and phys_end should be within the guest-physical address
+space. This includes upper and lower limits, as well as any carving of
+guest-physical addresses for use by the host (for instance MSI doorbells).
+Guest physical boundaries are set by the host using a firmware mechanism
+outside the scope of this specification.
\begin{note}
On flags: it is unlikely that all possible combinations of flags will be
@@ -503,11 +495,13 @@ negotiated.
The driver SHOULD set undefined \field{flags} bits to zero.
+\field{virt_end} MUST be strictly greater than \field{virt_start}.
+
\devicenormative{\paragraph}{MAP request}{Device Types / IOMMU Device / Device operations / MAP request}
-If \field{virt_addr}, \field{phys_addr} or \field{size} is not aligned on
-the page granularity, the device SHOULD set the request \field{status} to
-VIRTIO_IOMMU_S_RANGE and SHOULD NOT create the mapping.
+If \field{virt_start}, \field{phys_start} or (\field{virt_end} + 1) is
+not aligned on the page granularity, the device SHOULD set the request
+\field{status} to VIRTIO_IOMMU_S_RANGE and SHOULD NOT create the mapping.
If the device doesn't recognize a \field{flags} bit, it SHOULD set the
request \field{status} to VIRTIO_IOMMU_S_INVAL. In this case the device
@@ -524,45 +518,49 @@ If \field{domain} does not exist, the device SHOULD set the request
\begin{lstlisting}
struct virtio_iommu_req_unmap {
le32 domain;
- le64 virt_addr;
- le64 size;
+ le64 virt_start;
+ le64 virt_end;
le32 reserved;
};
\end{lstlisting}
Unmap a range of addresses mapped with VIRTIO_IOMMU_T_MAP. We define here
a mapping as a virtual region created with a single MAP request. All
-mappings covered by the range $[virt\_addr; virt\_addr + size [$ are
-removed.
+mappings covered by the range $[virt\_start; virt\_end]$ are removed.
-The semantics of unmapping are specified below, and illustrated with the
-following requests, assuming each example sequence starts with a blank
-address space. We define two pseudocode functions \texttt{map(virt\_addr,
-size) -> mapping} and \texttt{unmap(virt\_addr, size)}.
+The semantics of unmapping are specified in \ref{drivernormative:Device
+Types / IOMMU Device / Device operations / UNMAP request} and
+\ref{devicenormative:Device Types / IOMMU Device / Device operations /
+UNMAP request}, and illustrated with the following requests, assuming each
+example sequence starts with a blank address space. We define two
+pseudocode functions \texttt{map(virt_start, virt_end) -> mapping} and
+\texttt{unmap(virt_start, virt_end)}.
\begin{lstlisting}
-(1) unmap(addr=0, size=5) -> succeeds, doesn't unmap anything
+(1) unmap(virt_start=0,
+ virt_end=4) -> succeeds, doesn't unmap anything
-(2) a = map(addr=0, size=10);
- unmap(0, 10) -> succeeds, unmaps a
+(2) a = map(virt_start=0,
+ virt_end=9);
+ unmap(0, 9) -> succeeds, unmaps a
-(3) a = map(0, 5);
- b = map(5, 5);
- unmap(0, 10) -> succeeds, unmaps a and b
+(3) a = map(0, 4);
+ b = map(5, 9);
+ unmap(0, 9) -> succeeds, unmaps a and b
-(4) a = map(0, 10);
- unmap(0, 5) -> faults, doesn't unmap anything
+(4) a = map(0, 9);
+ unmap(0, 4) -> faults, doesn't unmap anything
-(5) a = map(0, 5);
- b = map(5, 5);
- unmap(0, 5) -> succeeds, unmaps a
+(5) a = map(0, 4);
+ b = map(5, 9);
+ unmap(0, 4) -> succeeds, unmaps a
-(6) a = map(0, 5);
- unmap(0, 10) -> succeeds, unmaps a
+(6) a = map(0, 4);
+ unmap(0, 9) -> succeeds, unmaps a
-(7) a = map(0, 5);
- b = map(10, 5);
- unmap(0, 15) -> succeeds, unmaps a and b
+(7) a = map(0, 4);
+ b = map(10, 14);
+ unmap(0, 14) -> succeeds, unmaps a and b
\end{lstlisting}
This request is only available when VIRTIO_IOMMU_F_MAP_UNMAP has been
@@ -572,9 +570,9 @@ negotiated.
The driver SHOULD set the \field{reserved} field to zero.
-The range, defined by \field{virt_addr} and \field{size}, SHOULD cover one
-or more contiguous mappings created with MAP requests. The range MAY spill
-over unmapped virtual addresses.
+The range, defined by \field{virt_start} and \field{virt_end}, SHOULD
+cover one or more contiguous mappings created with MAP requests. The range
+MAY spill over unmapped virtual addresses.
The first address of a range SHOULD either be the first address of a
mapping or be outside any mapping. The last address of a range SHOULD
--
2.14.2
^ permalink raw reply related
* Re: [PATCH v18 07/10] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-12-01 15:38 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1511963726-34070-8-git-send-email-wei.w.wang@intel.com>
On Wed, Nov 29, 2017 at 09:55:23PM +0800, Wei Wang wrote:
> Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer of
> balloon (i.e. inflated/deflated) pages using scatter-gather lists to the
> host. A scatter-gather list is described by a vring desc.
>
> The implementation of the previous virtio-balloon is not very efficient,
> because the balloon pages are transferred to the host by one array each
> time. Here is the breakdown of the time in percentage spent on each step
> of the balloon inflating process (inflating 7GB of an 8GB idle guest).
>
> 1) allocating pages (6.5%)
> 2) sending PFNs to host (68.3%)
> 3) address translation (6.1%)
> 4) madvise (19%)
>
> It takes about 4126ms for the inflating process to complete. The above
> profiling shows that the bottlenecks are stage 2) and stage 4).
>
> This patch optimizes step 2) by transferring pages to host in sgs. An sg
> describes a chunk of guest physically continuous pages. With this
> mechanism, step 4) can also be optimized by doing address translation and
> madvise() in chunks rather than page by page.
>
> With this new feature, the above ballooning process takes ~440ms resulting
> in an improvement of ~89%.
>
> TODO: optimize stage 1) by allocating/freeing a chunk of pages instead of
> a single page each time.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> drivers/virtio/virtio_balloon.c | 230 +++++++++++++++++++++++++++++++++---
> include/uapi/linux/virtio_balloon.h | 1 +
> 2 files changed, 212 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 7960746..2c21c5a 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -32,6 +32,8 @@
> #include <linux/mm.h>
> #include <linux/mount.h>
> #include <linux/magic.h>
> +#include <linux/xbitmap.h>
> +#include <asm/page.h>
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -79,6 +81,9 @@ struct virtio_balloon {
> /* Synchronize access/update to this struct virtio_balloon elements */
> struct mutex balloon_lock;
>
> + /* The xbitmap used to record balloon pages */
> + struct xb page_xb;
> +
> /* The array of pfns we tell the Host about. */
> unsigned int num_pfns;
> __virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
> @@ -141,15 +146,121 @@ static void set_page_pfns(struct virtio_balloon *vb,
> page_to_balloon_pfn(page) + i);
> }
>
> +static void kick_and_wait(struct virtqueue *vq, wait_queue_head_t wq_head)
> +{
> + unsigned int len;
> +
> + virtqueue_kick(vq);
> + wait_event(wq_head, virtqueue_get_buf(vq, &len));
> +}
> +
> +static void send_one_desc(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + uint64_t addr,
> + uint32_t len,
> + bool inbuf,
> + bool batch)
> +{
> + int err;
> + unsigned int size;
> +
> + /* Detach all the used buffers from the vq */
> + while (virtqueue_get_buf(vq, &size))
> + ;
> +
> + err = virtqueue_add_one_desc(vq, addr, len, inbuf, vq);
> + /*
> + * This is expected to never fail: there is always at least 1 entry
> + * available on the vq, because when the vq is full the worker thread
> + * that adds the desc will be put into sleep until at least 1 entry is
> + * available to use.
> + */
> + BUG_ON(err);
> +
> + /* If batching is requested, we batch till the vq is full */
> + if (!batch || !vq->num_free)
> + kick_and_wait(vq, vb->acked);
> +}
> +
This internal kick complicates callers. I suggest that instead,
you move this to callers, just return a "kick required" boolean.
This way callers do not need to play with num_free at all.
> +/*
> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> + * The page xbitmap is searched for continuous "1" bits, which correspond
> + * to continuous pages, to chunk into sgs.
> + *
> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
> + * need to be searched.
> + */
> +static void tell_host_sgs(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + unsigned long page_xb_start,
> + unsigned long page_xb_end)
> +{
> + unsigned long pfn_start, pfn_end;
> + uint64_t addr;
> + uint32_t len, max_len = round_down(UINT_MAX, PAGE_SIZE);
> +
> + pfn_start = page_xb_start;
> + while (pfn_start < page_xb_end) {
> + pfn_start = xb_find_next_set_bit(&vb->page_xb, pfn_start,
> + page_xb_end);
> + if (pfn_start == page_xb_end + 1)
> + break;
> + pfn_end = xb_find_next_zero_bit(&vb->page_xb,
> + pfn_start + 1,
> + page_xb_end);
> + addr = pfn_start << PAGE_SHIFT;
> + len = (pfn_end - pfn_start) << PAGE_SHIFT;
This assugnment can overflow. Next line compares with UINT_MAX but by
that time it is too late. I think you should do all math in 64 bit to
avoid surprises, then truncate to max_len and then it's safe to assign
to sg.
> + while (len > max_len) {
> + send_one_desc(vb, vq, addr, max_len, true, true);
> + addr += max_len;
> + len -= max_len;
> + }
> + send_one_desc(vb, vq, addr, len, true, true);
> + pfn_start = pfn_end + 1;
> + }
> +
> + /*
> + * The last few desc-s may not reach the batch size, but need a kick to
> + * notify the device to handle them.
> + */
> + if (vq->num_free != virtqueue_get_vring_size(vq))
> + kick_and_wait(vq, vb->acked);
the actual trigger for kick is if we did not kick after
the last send_one_desc. If kick is moved out of send_one_desc,
you can test for that explicitly.
> +
> + xb_clear_bit_range(&vb->page_xb, page_xb_start, page_xb_end);
> +}
> +
> +static inline int xb_set_page(struct virtio_balloon *vb,
> + struct page *page,
> + unsigned long *pfn_min,
> + unsigned long *pfn_max)
> +{
> + unsigned long pfn = page_to_pfn(page);
> + int ret;
> +
> + *pfn_min = min(pfn, *pfn_min);
> + *pfn_max = max(pfn, *pfn_max);
> +
> + do {
> + ret = xb_preload_and_set_bit(&vb->page_xb, pfn,
> + GFP_NOWAIT | __GFP_NOWARN);
> + } while (unlikely(ret == -EAGAIN));
what exactly does this loop do? Does this wait
forever until there is some free memory? why GFP_NOWAIT?
> +
> + return ret;
> +}
> +
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> unsigned num_allocated_pages;
> unsigned num_pfns;
> struct page *page;
> LIST_HEAD(pages);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> for (num_pfns = 0; num_pfns < num;
> num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> @@ -172,11 +283,18 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> vb->num_pfns = 0;
>
> while ((page = balloon_page_pop(&pages))) {
> + if (use_sg) {
> + if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) {
> + __free_page(page);
> + break;
> + }
> + } else {
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> + }
> +
> balloon_page_enqueue(&vb->vb_dev_info, page);
>
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> -
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> @@ -185,8 +303,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>
> num_allocated_pages = vb->num_pfns;
> /* Did we get any? */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->inflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->inflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->inflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> return num_allocated_pages;
> @@ -212,9 +334,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> struct page *page;
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> LIST_HEAD(pages);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> - /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + /* Traditionally, we can only do one array worth at a time. */
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> /* We can't release more pages than taken */
> @@ -224,7 +349,14 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> page = balloon_page_dequeue(vb_dev_info);
> if (!page)
> break;
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> + if (use_sg) {
> + if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) {
> + balloon_page_enqueue(&vb->vb_dev_info, page);
> + break;
> + }
> + } else {
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> + }
> list_add(&page->lru, &pages);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> @@ -235,13 +367,55 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->deflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->deflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->deflate_vq);
> + }
> release_pages_balloon(vb, &pages);
> mutex_unlock(&vb->balloon_lock);
> return num_freed_pages;
> }
>
> +/*
> + * The regular leak_balloon() with VIRTIO_BALLOON_F_SG needs memory allocation
> + * for xbitmap, which is not suitable for the oom case. This function does not
> + * use xbitmap to chunk pages, so it can be used by oom notifier to deflate
> + * pages when VIRTIO_BALLOON_F_SG is negotiated.
> + */
I guess we can live with this for now.
Two things to consider
- adding support for pre-allocating indirect buffers
- sorting the internal page queue (how?)
> +static unsigned int leak_balloon_sg_oom(struct virtio_balloon *vb)
> +{
> + unsigned int n;
> + struct page *page;
> + struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> + struct virtqueue *vq = vb->deflate_vq;
> + LIST_HEAD(pages);
> +
> + mutex_lock(&vb->balloon_lock);
> + for (n = 0; n < oom_pages; n++) {
> + page = balloon_page_dequeue(vb_dev_info);
> + if (!page)
> + break;
> +
> + list_add(&page->lru, &pages);
> + vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> + send_one_desc(vb, vq, virt_to_phys(page_address(page)),
> + PAGE_SIZE, true, true);
> + release_pages_balloon(vb, &pages);
> + }
> +
> + /*
> + * The last few sgs may not reach the batch size, but need a kick to
> + * notify the device to handle them.
> + */
> + if (vq->num_free != virtqueue_get_vring_size(vq))
> + kick_and_wait(vq, vb->acked);
> + mutex_unlock(&vb->balloon_lock);
> +
> + return n;
> +}
> +
> static inline void update_stat(struct virtio_balloon *vb, int idx,
> u16 tag, u64 val)
> {
> @@ -381,7 +555,10 @@ static int virtballoon_oom_notify(struct notifier_block *self,
> return NOTIFY_OK;
>
> freed = parm;
> - num_freed_pages = leak_balloon(vb, oom_pages);
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG))
> + num_freed_pages = leak_balloon_sg_oom(vb);
> + else
> + num_freed_pages = leak_balloon(vb, oom_pages);
> update_balloon_size(vb);
> *freed += num_freed_pages;
>
> @@ -478,6 +655,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> {
> struct virtio_balloon *vb = container_of(vb_dev_info,
> struct virtio_balloon, vb_dev_info);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> unsigned long flags;
>
> /*
> @@ -499,16 +677,26 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> vb_dev_info->isolated_pages--;
> __count_vm_event(BALLOON_MIGRATE);
> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, newpage);
> - tell_host(vb, vb->inflate_vq);
> -
> + if (use_sg) {
> + send_one_desc(vb, vb->inflate_vq,
> + virt_to_phys(page_address(newpage)),
> + PAGE_SIZE, true, false);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> + }
> /* balloon's page migration 2nd step -- deflate "page" */
> balloon_page_delete(page);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, page);
> - tell_host(vb, vb->deflate_vq);
> -
> + if (use_sg) {
> + send_one_desc(vb, vb->inflate_vq,
> + virt_to_phys(page_address(page)),
> + PAGE_SIZE, true, false);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> put_page(page); /* balloon reference */
> @@ -567,6 +755,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (err)
> goto out_free_vb;
>
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> + xb_init(&vb->page_xb);
> +
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -683,6 +874,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_MUST_TELL_HOST,
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> + VIRTIO_BALLOON_F_SG,
> };
>
> static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..37780a7 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> --
> 2.7.4
^ permalink raw reply
* RE: [PATCH v18 05/10] xbitmap: add more operations
From: Wang, Wei W @ 2017-12-01 15:09 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz@gmail.com, kvm@vger.kernel.org, mst@redhat.com,
liliang.opensource@gmail.com, qemu-devel@nongnu.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
mawilcox@microsoft.com, willy@infradead.org, quan.xu@aliyun.com,
nilal@redhat.com, riel@redhat.com, cornelia.huck@de.ibm.com,
mhocko@kernel.org, linux-kernel@vger.kernel.org, amit.shah
In-Reply-To: <201712012202.BDE13557.MJFQLtOOHVOFSF@I-love.SAKURA.ne.jp>
On Friday, December 1, 2017 9:02 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
> > On 11/30/2017 06:34 PM, Tetsuo Handa wrote:
> > > Wei Wang wrote:
> > >> + * @start: the start of the bit range, inclusive
> > >> + * @end: the end of the bit range, inclusive
> > >> + *
> > >> + * This function is used to clear a bit in the xbitmap. If all the
> > >> +bits of the
> > >> + * bitmap are 0, the bitmap will be freed.
> > >> + */
> > >> +void xb_clear_bit_range(struct xb *xb, unsigned long start,
> > >> +unsigned long end) {
> > >> + struct radix_tree_root *root = &xb->xbrt;
> > >> + struct radix_tree_node *node;
> > >> + void **slot;
> > >> + struct ida_bitmap *bitmap;
> > >> + unsigned int nbits;
> > >> +
> > >> + for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
> > >> + unsigned long index = start / IDA_BITMAP_BITS;
> > >> + unsigned long bit = start % IDA_BITMAP_BITS;
> > >> +
> > >> + bitmap = __radix_tree_lookup(root, index, &node, &slot);
> > >> + if (radix_tree_exception(bitmap)) {
> > >> + unsigned long ebit = bit + 2;
> > >> + unsigned long tmp = (unsigned long)bitmap;
> > >> +
> > >> + nbits = min(end - start + 1, BITS_PER_LONG - ebit);
> > > "nbits = min(end - start + 1," seems to expect that start == end is
> > > legal for clearing only 1 bit. But this function is no-op if start == end.
> > > Please clarify what "inclusive" intended.
> >
> > If xb_clear_bit_range(xb,10,10), then it is effectively the same as
> > xb_clear_bit(10). Why would it be illegal?
> >
> > "@start inclusive" means that the @start will also be included to be
> > cleared.
>
> If start == end is legal,
>
> for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
>
> makes this loop do nothing because 10 < 10 is false.
How about "start <= end "?
Best,
Wei
^ permalink raw reply
* Re: [PATCH net,stable v4 0/3] vhost: fix a few skb leaks
From: Michael S. Tsirkin @ 2017-12-01 14:58 UTC (permalink / raw)
To: Matthew Rosato; +Cc: netdev, linux-kernel, virtualization, wexu, David Miller
In-Reply-To: <9ef31370-4042-b701-e983-90b8f20a2a39@linux.vnet.ibm.com>
On Fri, Dec 01, 2017 at 09:54:02AM -0500, Matthew Rosato wrote:
> On 12/01/2017 09:47 AM, Michael S. Tsirkin wrote:
> > On Fri, Dec 01, 2017 at 05:10:35AM -0500, wexu@redhat.com wrote:
> >> From: Wei Xu <wexu@redhat.com>
> >>
> >> Matthew found a roughly 40% tcp throughput regression with commit
> >> c67df11f(vhost_net: try batch dequing from skb array) as discussed
> >> in the following thread:
> >> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
> >
> > Series
> >
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Tested this series on a z13 (s390x) on top of net-next using 4GB/4vcpu
> guests. Verified that both the reported TCP throughput regression and
> memory leak are resolved.
>
> net-next: 19.83 Gb/s
> net-next+: 35.02 Gb/s
>
> Thanks all!
>
> Matt
So we can also add
Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Dave, pls take this through the net tree as most changes are
in tun/tap.
Thanks!
> >
> >
> >> v4:
> >> - fix zero iov iterator count in tap/tap_do_read()(Jason)
> >> - don't put tun in case of EBADFD(Jason)
> >> - Replace msg->msg_control with new 'skb' when calling tun/tap_do_read()
> >>
> >> v3:
> >> - move freeing skb from vhost to tun/tap recvmsg() to not
> >> confuse the callers.
> >>
> >> v2:
> >> - add Matthew as the reporter, thanks matthew.
> >> - moving zero headcount check ahead instead of defer consuming skb
> >> due to jason and mst's comment.
> >> - add freeing skb in favor of recvmsg() fails.
> >>
> >> Wei Xu (3):
> >> vhost: fix skb leak in handle_rx()
> >> tun: free skb in early errors
> >> tap: free skb if flags error
> >>
> >> drivers/net/tap.c | 14 ++++++++++----
> >> drivers/net/tun.c | 24 ++++++++++++++++++------
> >> drivers/vhost/net.c | 20 ++++++++++----------
> >> 3 files changed, 38 insertions(+), 20 deletions(-)
> >>
> >> --
> >> 1.8.3.1
> >
^ permalink raw reply
* Re: [PATCH net,stable v4 0/3] vhost: fix a few skb leaks
From: Matthew Rosato @ 2017-12-01 14:54 UTC (permalink / raw)
To: Michael S. Tsirkin, wexu; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20171201164731-mutt-send-email-mst@kernel.org>
On 12/01/2017 09:47 AM, Michael S. Tsirkin wrote:
> On Fri, Dec 01, 2017 at 05:10:35AM -0500, wexu@redhat.com wrote:
>> From: Wei Xu <wexu@redhat.com>
>>
>> Matthew found a roughly 40% tcp throughput regression with commit
>> c67df11f(vhost_net: try batch dequing from skb array) as discussed
>> in the following thread:
>> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
>
> Series
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
Tested this series on a z13 (s390x) on top of net-next using 4GB/4vcpu
guests. Verified that both the reported TCP throughput regression and
memory leak are resolved.
net-next: 19.83 Gb/s
net-next+: 35.02 Gb/s
Thanks all!
Matt
>
>
>> v4:
>> - fix zero iov iterator count in tap/tap_do_read()(Jason)
>> - don't put tun in case of EBADFD(Jason)
>> - Replace msg->msg_control with new 'skb' when calling tun/tap_do_read()
>>
>> v3:
>> - move freeing skb from vhost to tun/tap recvmsg() to not
>> confuse the callers.
>>
>> v2:
>> - add Matthew as the reporter, thanks matthew.
>> - moving zero headcount check ahead instead of defer consuming skb
>> due to jason and mst's comment.
>> - add freeing skb in favor of recvmsg() fails.
>>
>> Wei Xu (3):
>> vhost: fix skb leak in handle_rx()
>> tun: free skb in early errors
>> tap: free skb if flags error
>>
>> drivers/net/tap.c | 14 ++++++++++----
>> drivers/net/tun.c | 24 ++++++++++++++++++------
>> drivers/vhost/net.c | 20 ++++++++++----------
>> 3 files changed, 38 insertions(+), 20 deletions(-)
>>
>> --
>> 1.8.3.1
>
^ 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