* [PATCH net-next] virtio-net: avoid unnecessary sg initialzation
From: Jason Wang @ 2015-08-27 6:53 UTC (permalink / raw)
To: mst, virtualization, netdev, linux-kernel
Usually an skb does not have up to MAX_SKB_FRAGS frags. So no need to
initialize the unuse part of sg. This patch initialize the sg based on
the real number it will used:
- during xmit, it could be inferred from nr_frags and can_push.
- for small receive buffer, it will also be 2.
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 53f5660..c006ae4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -540,7 +540,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
skb_put(skb, GOOD_PACKET_LEN);
hdr = skb_vnet_hdr(skb);
- sg_init_table(rq->sg, MAX_SKB_FRAGS + 2);
+ sg_init_table(rq->sg, 2);
sg_set_buf(rq->sg, hdr, vi->hdr_len);
skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
@@ -893,7 +893,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
if (vi->mergeable_rx_bufs)
hdr->num_buffers = 0;
- sg_init_table(sq->sg, MAX_SKB_FRAGS + 2);
+ sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
if (can_push) {
__skb_push(skb, hdr_len);
num_sg = skb_to_sgvec(skb, sq->sg, 0, skb->len);
--
2.1.4
^ permalink raw reply related
* A question about vring operation
From: Wang, Wei W @ 2015-08-27 1:03 UTC (permalink / raw)
To: Michael S. Tsirkin, Rusty Russell, Paolo Bonzini,
virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
[-- Attachment #1.1: Type: text/plain, Size: 533 bytes --]
Hi all,
I have a question about the vring_avail:
It only includes an idx (equivalent to the ring tail), which is used by the frontend (virtio_net) to fill bufs. The backend (e.g. vhost_net) maintains the ring head (last_avail_idx) by itself. The frontend checks if the ring is full or empty via a counter (vq->num_free).
My question is why can't we include the ring head in the vring_avail struct, so that the vq->num_free is not needed, and the backend can directly use it without maintaining its own copy?
Thanks,
Wei
[-- Attachment #1.2: Type: text/html, Size: 2670 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: BUG kmalloc-64 (Not tainted): Poison overwritten
From: Cong Wang @ 2015-08-25 21:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <CAHA+R7OtN5AyGS-hXwfAj4cfediu0i6+L6K+Z_DcnYaQJj6bhg@mail.gmail.com>
On Tue, Aug 25, 2015 at 2:11 PM, Cong Wang <cwang@twopensource.com> wrote:
> Hi, Michael
>
> I just got the following kernel bug while working on Dave's net tree
> in a KVM guest. It looks like a bug in virtio.
>
Hmm, the stack trace is misleading, it could be caused by my own networking
code even though it doesn't show in dmesg. Anyway I can't reproduce it on the
clean -net tree.
I will let you know if it is really a bug.
Thanks.
^ permalink raw reply
* BUG kmalloc-64 (Not tainted): Poison overwritten
From: Cong Wang @ 2015-08-25 21:11 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel@vger.kernel.org, virtualization
Hi, Michael
I just got the following kernel bug while working on Dave's net tree
in a KVM guest. It looks like a bug in virtio.
Let me know if you need more information.
[ 69.816089] BUG kmalloc-64 (Not tainted): Poison overwritten
[ 69.816089] -----------------------------------------------------------------------------
[ 69.816089]
[ 69.816089] Disabling lock debugging due to kernel taint
[ 69.816089] INFO: 0xffff8800d480c7c0-0xffff8800d480c7c0. First byte
0x6a instead of 0x6b
[ 69.816089] INFO: Allocated in virtqueue_add+0x6b/0x2a2 age=423 cpu=3 pid=128
[ 69.816089] __slab_alloc+0x44b/0x4d2
[ 69.816089] __kmalloc+0xa3/0x14e
[ 69.816089] virtqueue_add+0x6b/0x2a2
[ 69.816089] virtqueue_add_sgs+0x78/0x87
[ 69.816089] __virtblk_add_req+0x139/0x14b
[ 69.816089] virtio_queue_rq+0x14e/0x1f0
[ 69.816089] __blk_mq_run_hw_queue+0x1ac/0x2b9
[ 69.816089] blk_mq_run_hw_queue+0x59/0xb8
[ 69.816089] blk_mq_insert_requests+0x136/0x1ab
[ 69.816089] blk_mq_flush_plug_list+0xd4/0xe3
[ 69.816089] blk_flush_plug_list+0x9b/0x1b9
[ 69.816089] blk_finish_plug+0x24/0x33
[ 69.816089] generic_writepages+0x4c/0x59
[ 69.816089] do_writepages+0x21/0x2f
[ 69.816089] __writeback_single_inode+0xd6/0x5ca
[ 69.816089] writeback_sb_inodes+0x28c/0x458
[ 69.816089] INFO: Freed in detach_buf+0x3d/0x6e age=469 cpu=3 pid=128
[ 69.816089] __slab_free+0x35/0x283
[ 69.816089] kfree+0x153/0x1ac
[ 69.816089] detach_buf+0x3d/0x6e
[ 69.816089] virtqueue_get_buf+0xac/0xdd
[ 69.816089] virtblk_done+0x61/0xcb
[ 69.816089] vring_interrupt+0x2d/0x3c
[ 69.816089] handle_irq_event_percpu+0xbd/0x2c0
[ 69.816089] handle_irq_event+0x4a/0x6e
[ 69.816089] handle_edge_irq+0xc0/0xe3
[ 69.816089] handle_irq+0x11b/0x128
[ 69.816089] do_IRQ+0x4d/0xc1
[ 69.816089] ret_from_intr+0x0/0x1d
[ 70.780063] kmem_cache_free+0xb2/0x248
[ 70.780063] ext4_release_io_end+0x78/0xa9
[ 70.780063] ext4_put_io_end+0x50/0x5f
[ 70.780063] ext4_writepages+0x662/0xb62
[ 70.780063] INFO: Slab 0xffffea0003520300 objects=20 used=20 fp=0x
(null) flags=0x1ffff8000004080
[ 70.780063] INFO: Object 0xffff8800d480c7a8 @offset=1960
fp=0xffff8800d480db90
[ 70.780063]
[ 70.780063] Bytes b4 ffff8800d480c798: 00 00 00 00 00 00 00 00 5a
5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
[ 70.780063] Object ffff8800d480c7a8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 70.780063] Object ffff8800d480c7b8: 6b 6b 6b 6b 6b 6b 6b 6b 6a 6b
6b 6b 6b 6b 6b 6b kkkkkkkkjkkkkkkk
[ 70.780063] Object ffff8800d480c7c8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 70.780063] Object ffff8800d480c7d8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
[ 70.780063] Redzone ffff8800d480c7e8: bb bb bb bb bb bb bb bb
........
[ 70.780063] Padding ffff8800d480c928: 5a 5a 5a 5a 5a 5a 5a 5a
ZZZZZZZZ
[ 70.780063] CPU: 1 PID: 780 Comm: u32_del.sh Tainted: G B
4.2.0-rc7+ #1097
[ 70.780063] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 70.780063] ffff8800d480c7a8 ffff8800d5a47aa8 ffffffff81a64274
0000000000000000
[ 70.780063] ffff88011a007700 ffff8800d5a47ad8 ffffffff81187587
ffff8800d480c7c0
[ 70.780063] 000000000000006b ffff88011a007700 ffff8800d480c7c0
ffff8800d5a47b38
[ 70.780063] Call Trace:
[ 70.780063] [<ffffffff81a64274>] dump_stack+0x4c/0x65
[ 70.780063] [<ffffffff81187587>] print_trailer+0x12b/0x134
[ 70.780063] [<ffffffff81187636>] check_bytes_and_report+0xa6/0xf3
[ 70.780063] [<ffffffff811b8111>] ? alloc_fdmem+0x2b/0x34
[ 70.780063] [<ffffffff81187b5a>] check_object+0x111/0x1ac
[ 70.780063] [<ffffffff811b8111>] ? alloc_fdmem+0x2b/0x34
[ 70.780063] [<ffffffff81a5ef08>] alloc_debug_processing+0x67/0x109
[ 70.780063] [<ffffffff81a5f3f5>] __slab_alloc+0x44b/0x4d2
[ 70.780063] [<ffffffff811b8111>] ? alloc_fdmem+0x2b/0x34
[ 70.780063] [<ffffffff810a34aa>] ? __might_sleep+0x78/0x80
[ 70.780063] [<ffffffff81189388>] __kmalloc+0xa3/0x14e
[ 70.780063] [<ffffffff811b8111>] ? alloc_fdmem+0x2b/0x34
[ 70.780063] [<ffffffff811b8111>] alloc_fdmem+0x2b/0x34
[ 70.780063] [<ffffffff811b81ab>] alloc_fdtable+0x91/0xc2
[ 70.780063] [<ffffffff811b8aca>] dup_fd+0x15d/0x2fc
[ 70.780063] [<ffffffff8107866a>] copy_process.part.33+0x704/0x1837
[ 70.780063] [<ffffffff810a43b4>] ? sched_clock_cpu+0x9e/0xb7
[ 70.780063] [<ffffffff810a44a7>] ? local_clock+0x19/0x22
[ 70.780063] [<ffffffff810e41cb>] ? current_kernel_time+0xe/0x32
[ 70.780063] [<ffffffff8107994f>] _do_fork+0xd3/0x371
[ 70.780063] [<ffffffff810e41cb>] ? current_kernel_time+0xe/0x32
[ 70.780063] [<ffffffff81106d79>] ? __audit_syscall_entry+0xbf/0xe1
[ 70.780063] [<ffffffff8100e658>] ? do_audit_syscall_entry+0x63/0x65
[ 70.780063] [<ffffffff8100f429>] ? syscall_trace_enter_phase1+0x11a/0x125
[ 70.780063] [<ffffffff81079c73>] SyS_clone+0x19/0x1b
[ 70.780063] [<ffffffff81a703d7>] entry_SYSCALL_64_fastpath+0x12/0x6f
[ 70.780063] FIX kmalloc-64: Restoring
0xffff8800d480c7c0-0xffff8800d480c7c0=0x6b
[ 70.780063]
[ 70.780063] FIX kmalloc-64: Marking all objects used
^ permalink raw reply
* Re: udev PATH_ID for virtio devices
From: Kay Sievers @ 2015-08-25 11:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Tom Gundersen, Harald Hoyer, virtualization
In-Reply-To: <20150825145151-mutt-send-email-mst@redhat.com>
On Tue, Aug 25, 2015 at 1:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Aug 25, 2015 at 01:46:49PM +0200, Kay Sievers wrote:
>> On Tue, Aug 25, 2015 at 1:43 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Aug 24, 2015 at 06:10:01PM +0200, Tom Gundersen wrote:
>> >> 1) if there is a guarantee now and in the future (even if only
>> >> restricted to netdevs) that no virtio bus will have a direct sibling
>> >> bus (i.e., with the same parent device); or
>> >
>> > I think this is the case. The virtio bus is an artifact.
>> > There's always a single one behind each pci device.
>> > Is this sufficient?
>>
>> I *think* is not good enough for udev to offer such functionality.
>>
>> We need an authoritative answer that this cannot happen with today's
>> code, and also that there are no plans to ever make multiple virtio
>> devices per parent device.
> But if virtio will make such a promise, will that be sufficient?
Sure. I mean we need to be reasonable sure, that there will never
be more than one virtio device per parent device. Otherwise an
innocent looking kernel change will just break existing userspace
setups which made assumptions which would then not be true anymore.
Kay
^ permalink raw reply
* Re: udev PATH_ID for virtio devices
From: Michael S. Tsirkin @ 2015-08-25 11:52 UTC (permalink / raw)
To: Kay Sievers; +Cc: Tom Gundersen, Harald Hoyer, virtualization
In-Reply-To: <CAPXgP13t+bKEVbQ1M3x7_4DXMk=cKv0Bqy1KD4bKA-JaUg9cmQ@mail.gmail.com>
On Tue, Aug 25, 2015 at 01:46:49PM +0200, Kay Sievers wrote:
> On Tue, Aug 25, 2015 at 1:43 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Aug 24, 2015 at 06:10:01PM +0200, Tom Gundersen wrote:
> >> On Thu, Mar 27, 2014 at 9:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Thu, Mar 27, 2014 at 09:24:10PM +0100, Tom Gundersen wrote:
> >> >> On Thu, Mar 27, 2014 at 8:25 PM, Kay Sievers <kay@vrfy.org> wrote:
> >> >> > On Thu, Mar 27, 2014 at 8:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> >> On Thu, Mar 27, 2014 at 08:06:15PM +0100, Kay Sievers wrote:
> >> >> >>> On Thu, Mar 27, 2014 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> >>> > On Thu, Mar 27, 2014 at 07:43:34PM +0100, Kay Sievers wrote:
> >> >> >>> >> On Thu, Mar 27, 2014 at 6:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> >>> >>
> >> >> >>> >> > If the virtio device is a PCI device, it is really best to
> >> >> >>> >> > treat it like you treat any other PCI function (I guess you mean
> >> >> >>> >> > function and not device, right? We support multifunction
> >> >> >>> >> > devices and some people do pack multiple NICs in a single device)..
> >> >> >>> >> >
> >> >> >>> >> > At the moment many devices in a single pci function can not happen on a
> >> >> >>> >> > PCI system (no multiport) but if we add multiport, we'll follow some
> >> >> >>> >> > existing standard to expose this information to the guest.
> >> >> >>> >>
> >> >> >>> >> This means, that there can currently never multiple devices below one
> >> >> >>> >> and the same virtio parent device?
> >> >> >>>
> >> >> >>> > There's a single virtio device per pci function (you keep saying
> >> >> >>> > device but I hope the distinction is clear and this is
> >> >> >>> > just slip of the tongue).
> >> >> >>>
> >> >> >>> Right, we talks about sysfs directories and they are called "device",
> >> >> >>> we don't really care about the actual bus that is implemented,
> >> >> >>> userspace does not really know much about them. :)
> >> >> >>>
> >> >> >>> > For net devices under a pci function that is also currently the case,
> >> >> >>> > but I can't yet tell you for sure ahead of the time how we'll present
> >> >> >>> > multiport devices if we ever implement them.
> >> >> >>> >
> >> >> >>> > I'm guessing there will be multiple net devices under
> >> >> >>> > a single pci device and we'll present a sysfs attribute with the port
> >> >> >>> > number in this case.
> >> >> >>> >
> >> >> >>> > Hmm maybe we should go ahead and add a place-holder
> >> >> >>> > attribute so that it's future-proof?
> >> >> >>> >
> >> >> >>> > I'll write a patch like that and we'll see how it's accepted.
> >> >> >>>
> >> >> >>> Netdevs with multiple ports are represented with the standard "dev_id"
> >> >> >>> attribute identifying the instance of the driver per parent "device";
> >> >> >>> should all work already from the userspace side, if the virtio side
> >> >> >>> would use that too.
> >> >> >>>
> >> >> >>> Kay
> >> >> >>
> >> >> >> Aha. In that case it's easy - pls assume that if and when we implement
> >> >> >> multiple we'll just follow standards and use dev_id.
> >> >> >> For virtio pci devices specifically virtio<->pci 1:1 mapping is set
> >> >> >> in stone in the spec.
> >> >> >>
> >> >> >> Non pci ones need to be examined separately.
> >> >> >
> >> >> > Nice, thanks. That means we can just "jump over" the "virtio*" device
> >> >> > (device as in sysfs view), and let the pci parent let identify the
> >> >> > device.
> >> >> >
> >> >> > Background: The logic for device naming generally refuses to touch
> >> >> > devices with unknown parent devices in the chain (the directories you
> >> >> > see for: ls -l /sys/class/net/), because the parents *could* offer
> >> >> > their own bus logic that exposes multiple devices below and we would
> >> >> > create clashing names for them. That's why the virt vs. non-virt case
> >> >> > needs to be handled explicitly (seems in this case by just skipping
> >> >> > it) and it is not necessarily by default the same behaviour.
> >> >>
> >> >> So Kay and I discussed this a bit more, and found that we likely
> >> >> cannot handle virtio nic's nicely. As we would name them based on the
> >> >> pci geo, we rely on this being stable between reboots and when
> >> >> adding/removing hardware. Is there some way to make this work with
> >> >> virtio, or will the 'fake' pci busses simply be enumerated as they are
> >> >> created?
> >> >>
> >> >> Cheers,
> >> >>
> >> >> Tom
> >> >
> >> > I think it's standard PCI thing.
> >> > Some pci bridges have a slot id register - I'm assuming you
> >> > are using these if present?
> >> >
> >> > If not you'll either need to rely on firmware enumerating buses
> >> > consistently, or use the mac of the NIC.
> >>
> >> Hi Michael,
> >>
> >> Sorry to resurrect such an old thread, but this suddenly became
> >> relevant again and I see that we never really reached a conclusion.
> >> Rereading the discussion it is still not really clear to me what the
> >> API promise is, so let me try to rephrase what we need and hopefully
> >> you can help me understand where we are at.
> >>
> >> My understanding is that the enumeration of virtioX buses is global
> >> rather than local to the direct parent device of a given bus. That
> >> means that we cannot base deterministic naming of child devices
> >> (functions) on virtio enumeration, as the names would then depend on
> >> the presence or absence of unrelated virtio buses on the system.
> >>
> >> We have two options that would allow us to get around this:
> >>
> >> 1) if there is a guarantee now and in the future (even if only
> >> restricted to netdevs) that no virtio bus will have a direct sibling
> >> bus (i.e., with the same parent device); or
> >
> > I think this is the case. The virtio bus is an artifact.
> > There's always a single one behind each pci device.
> > Is this sufficient?
>
> I *think* is not good enough for udev to offer such functionality.
>
> We need an authoritative answer that this cannot happen with today's
> code, and also that there are no plans to ever make multiple virtio
> devices per parent device.
>
> Thanks,
> Kay
But if virtio will make such a promise, will that be sufficient?
--
MST
^ permalink raw reply
* Re: udev PATH_ID for virtio devices
From: Kay Sievers @ 2015-08-25 11:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Tom Gundersen, Harald Hoyer, virtualization
In-Reply-To: <20150825144034-mutt-send-email-mst@redhat.com>
On Tue, Aug 25, 2015 at 1:43 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Aug 24, 2015 at 06:10:01PM +0200, Tom Gundersen wrote:
>> On Thu, Mar 27, 2014 at 9:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Thu, Mar 27, 2014 at 09:24:10PM +0100, Tom Gundersen wrote:
>> >> On Thu, Mar 27, 2014 at 8:25 PM, Kay Sievers <kay@vrfy.org> wrote:
>> >> > On Thu, Mar 27, 2014 at 8:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> >> On Thu, Mar 27, 2014 at 08:06:15PM +0100, Kay Sievers wrote:
>> >> >>> On Thu, Mar 27, 2014 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> >>> > On Thu, Mar 27, 2014 at 07:43:34PM +0100, Kay Sievers wrote:
>> >> >>> >> On Thu, Mar 27, 2014 at 6:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> >>> >>
>> >> >>> >> > If the virtio device is a PCI device, it is really best to
>> >> >>> >> > treat it like you treat any other PCI function (I guess you mean
>> >> >>> >> > function and not device, right? We support multifunction
>> >> >>> >> > devices and some people do pack multiple NICs in a single device)..
>> >> >>> >> >
>> >> >>> >> > At the moment many devices in a single pci function can not happen on a
>> >> >>> >> > PCI system (no multiport) but if we add multiport, we'll follow some
>> >> >>> >> > existing standard to expose this information to the guest.
>> >> >>> >>
>> >> >>> >> This means, that there can currently never multiple devices below one
>> >> >>> >> and the same virtio parent device?
>> >> >>>
>> >> >>> > There's a single virtio device per pci function (you keep saying
>> >> >>> > device but I hope the distinction is clear and this is
>> >> >>> > just slip of the tongue).
>> >> >>>
>> >> >>> Right, we talks about sysfs directories and they are called "device",
>> >> >>> we don't really care about the actual bus that is implemented,
>> >> >>> userspace does not really know much about them. :)
>> >> >>>
>> >> >>> > For net devices under a pci function that is also currently the case,
>> >> >>> > but I can't yet tell you for sure ahead of the time how we'll present
>> >> >>> > multiport devices if we ever implement them.
>> >> >>> >
>> >> >>> > I'm guessing there will be multiple net devices under
>> >> >>> > a single pci device and we'll present a sysfs attribute with the port
>> >> >>> > number in this case.
>> >> >>> >
>> >> >>> > Hmm maybe we should go ahead and add a place-holder
>> >> >>> > attribute so that it's future-proof?
>> >> >>> >
>> >> >>> > I'll write a patch like that and we'll see how it's accepted.
>> >> >>>
>> >> >>> Netdevs with multiple ports are represented with the standard "dev_id"
>> >> >>> attribute identifying the instance of the driver per parent "device";
>> >> >>> should all work already from the userspace side, if the virtio side
>> >> >>> would use that too.
>> >> >>>
>> >> >>> Kay
>> >> >>
>> >> >> Aha. In that case it's easy - pls assume that if and when we implement
>> >> >> multiple we'll just follow standards and use dev_id.
>> >> >> For virtio pci devices specifically virtio<->pci 1:1 mapping is set
>> >> >> in stone in the spec.
>> >> >>
>> >> >> Non pci ones need to be examined separately.
>> >> >
>> >> > Nice, thanks. That means we can just "jump over" the "virtio*" device
>> >> > (device as in sysfs view), and let the pci parent let identify the
>> >> > device.
>> >> >
>> >> > Background: The logic for device naming generally refuses to touch
>> >> > devices with unknown parent devices in the chain (the directories you
>> >> > see for: ls -l /sys/class/net/), because the parents *could* offer
>> >> > their own bus logic that exposes multiple devices below and we would
>> >> > create clashing names for them. That's why the virt vs. non-virt case
>> >> > needs to be handled explicitly (seems in this case by just skipping
>> >> > it) and it is not necessarily by default the same behaviour.
>> >>
>> >> So Kay and I discussed this a bit more, and found that we likely
>> >> cannot handle virtio nic's nicely. As we would name them based on the
>> >> pci geo, we rely on this being stable between reboots and when
>> >> adding/removing hardware. Is there some way to make this work with
>> >> virtio, or will the 'fake' pci busses simply be enumerated as they are
>> >> created?
>> >>
>> >> Cheers,
>> >>
>> >> Tom
>> >
>> > I think it's standard PCI thing.
>> > Some pci bridges have a slot id register - I'm assuming you
>> > are using these if present?
>> >
>> > If not you'll either need to rely on firmware enumerating buses
>> > consistently, or use the mac of the NIC.
>>
>> Hi Michael,
>>
>> Sorry to resurrect such an old thread, but this suddenly became
>> relevant again and I see that we never really reached a conclusion.
>> Rereading the discussion it is still not really clear to me what the
>> API promise is, so let me try to rephrase what we need and hopefully
>> you can help me understand where we are at.
>>
>> My understanding is that the enumeration of virtioX buses is global
>> rather than local to the direct parent device of a given bus. That
>> means that we cannot base deterministic naming of child devices
>> (functions) on virtio enumeration, as the names would then depend on
>> the presence or absence of unrelated virtio buses on the system.
>>
>> We have two options that would allow us to get around this:
>>
>> 1) if there is a guarantee now and in the future (even if only
>> restricted to netdevs) that no virtio bus will have a direct sibling
>> bus (i.e., with the same parent device); or
>
> I think this is the case. The virtio bus is an artifact.
> There's always a single one behind each pci device.
> Is this sufficient?
I *think* is not good enough for udev to offer such functionality.
We need an authoritative answer that this cannot happen with today's
code, and also that there are no plans to ever make multiple virtio
devices per parent device.
Thanks,
Kay
^ permalink raw reply
* Re: udev PATH_ID for virtio devices
From: Michael S. Tsirkin @ 2015-08-25 11:43 UTC (permalink / raw)
To: Tom Gundersen; +Cc: Kay Sievers, Harald Hoyer, virtualization
In-Reply-To: <CAG-2HqUzkk6DE776K7gH=Z5_RkdhZ90nGHVUwm7MYB-tgrj0AA@mail.gmail.com>
On Mon, Aug 24, 2015 at 06:10:01PM +0200, Tom Gundersen wrote:
> On Thu, Mar 27, 2014 at 9:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Mar 27, 2014 at 09:24:10PM +0100, Tom Gundersen wrote:
> >> On Thu, Mar 27, 2014 at 8:25 PM, Kay Sievers <kay@vrfy.org> wrote:
> >> > On Thu, Mar 27, 2014 at 8:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> On Thu, Mar 27, 2014 at 08:06:15PM +0100, Kay Sievers wrote:
> >> >>> On Thu, Mar 27, 2014 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >>> > On Thu, Mar 27, 2014 at 07:43:34PM +0100, Kay Sievers wrote:
> >> >>> >> On Thu, Mar 27, 2014 at 6:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >>> >>
> >> >>> >> > If the virtio device is a PCI device, it is really best to
> >> >>> >> > treat it like you treat any other PCI function (I guess you mean
> >> >>> >> > function and not device, right? We support multifunction
> >> >>> >> > devices and some people do pack multiple NICs in a single device)..
> >> >>> >> >
> >> >>> >> > At the moment many devices in a single pci function can not happen on a
> >> >>> >> > PCI system (no multiport) but if we add multiport, we'll follow some
> >> >>> >> > existing standard to expose this information to the guest.
> >> >>> >>
> >> >>> >> This means, that there can currently never multiple devices below one
> >> >>> >> and the same virtio parent device?
> >> >>>
> >> >>> > There's a single virtio device per pci function (you keep saying
> >> >>> > device but I hope the distinction is clear and this is
> >> >>> > just slip of the tongue).
> >> >>>
> >> >>> Right, we talks about sysfs directories and they are called "device",
> >> >>> we don't really care about the actual bus that is implemented,
> >> >>> userspace does not really know much about them. :)
> >> >>>
> >> >>> > For net devices under a pci function that is also currently the case,
> >> >>> > but I can't yet tell you for sure ahead of the time how we'll present
> >> >>> > multiport devices if we ever implement them.
> >> >>> >
> >> >>> > I'm guessing there will be multiple net devices under
> >> >>> > a single pci device and we'll present a sysfs attribute with the port
> >> >>> > number in this case.
> >> >>> >
> >> >>> > Hmm maybe we should go ahead and add a place-holder
> >> >>> > attribute so that it's future-proof?
> >> >>> >
> >> >>> > I'll write a patch like that and we'll see how it's accepted.
> >> >>>
> >> >>> Netdevs with multiple ports are represented with the standard "dev_id"
> >> >>> attribute identifying the instance of the driver per parent "device";
> >> >>> should all work already from the userspace side, if the virtio side
> >> >>> would use that too.
> >> >>>
> >> >>> Kay
> >> >>
> >> >> Aha. In that case it's easy - pls assume that if and when we implement
> >> >> multiple we'll just follow standards and use dev_id.
> >> >> For virtio pci devices specifically virtio<->pci 1:1 mapping is set
> >> >> in stone in the spec.
> >> >>
> >> >> Non pci ones need to be examined separately.
> >> >
> >> > Nice, thanks. That means we can just "jump over" the "virtio*" device
> >> > (device as in sysfs view), and let the pci parent let identify the
> >> > device.
> >> >
> >> > Background: The logic for device naming generally refuses to touch
> >> > devices with unknown parent devices in the chain (the directories you
> >> > see for: ls -l /sys/class/net/), because the parents *could* offer
> >> > their own bus logic that exposes multiple devices below and we would
> >> > create clashing names for them. That's why the virt vs. non-virt case
> >> > needs to be handled explicitly (seems in this case by just skipping
> >> > it) and it is not necessarily by default the same behaviour.
> >>
> >> So Kay and I discussed this a bit more, and found that we likely
> >> cannot handle virtio nic's nicely. As we would name them based on the
> >> pci geo, we rely on this being stable between reboots and when
> >> adding/removing hardware. Is there some way to make this work with
> >> virtio, or will the 'fake' pci busses simply be enumerated as they are
> >> created?
> >>
> >> Cheers,
> >>
> >> Tom
> >
> > I think it's standard PCI thing.
> > Some pci bridges have a slot id register - I'm assuming you
> > are using these if present?
> >
> > If not you'll either need to rely on firmware enumerating buses
> > consistently, or use the mac of the NIC.
>
> Hi Michael,
>
> Sorry to resurrect such an old thread, but this suddenly became
> relevant again and I see that we never really reached a conclusion.
> Rereading the discussion it is still not really clear to me what the
> API promise is, so let me try to rephrase what we need and hopefully
> you can help me understand where we are at.
>
> My understanding is that the enumeration of virtioX buses is global
> rather than local to the direct parent device of a given bus. That
> means that we cannot base deterministic naming of child devices
> (functions) on virtio enumeration, as the names would then depend on
> the presence or absence of unrelated virtio buses on the system.
>
> We have two options that would allow us to get around this:
>
> 1) if there is a guarantee now and in the future (even if only
> restricted to netdevs) that no virtio bus will have a direct sibling
> bus (i.e., with the same parent device); or
I think this is the case. The virtio bus is an artifact.
There's always a single one behind each pci device.
Is this sufficient?
> 2) if the enumeration of virtio buses was extended with a "port"
> numebr which was guaranteed to be made local to their direct parent
> device and moreover be made deterministic. I.e., if there are no
> virtio sibling buses (but there may be other unrelated virtio buses)
> the name would always be virtioX.0, and if there are two buses with
> the same parent their names would always be virtioX.0 and virtioX.1
> and always in the same order.
>
> Is either of those things something that is already guaranteed or
> something that could be made working?
>
> Cheers,
>
> Tom
^ permalink raw reply
* Re: udev PATH_ID for virtio devices
From: Tom Gundersen @ 2015-08-24 16:10 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Kay Sievers, Harald Hoyer, virtualization
In-Reply-To: <20140327205827.GA4440@redhat.com>
On Thu, Mar 27, 2014 at 9:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Mar 27, 2014 at 09:24:10PM +0100, Tom Gundersen wrote:
>> On Thu, Mar 27, 2014 at 8:25 PM, Kay Sievers <kay@vrfy.org> wrote:
>> > On Thu, Mar 27, 2014 at 8:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> On Thu, Mar 27, 2014 at 08:06:15PM +0100, Kay Sievers wrote:
>> >>> On Thu, Mar 27, 2014 at 7:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >>> > On Thu, Mar 27, 2014 at 07:43:34PM +0100, Kay Sievers wrote:
>> >>> >> On Thu, Mar 27, 2014 at 6:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >>> >>
>> >>> >> > If the virtio device is a PCI device, it is really best to
>> >>> >> > treat it like you treat any other PCI function (I guess you mean
>> >>> >> > function and not device, right? We support multifunction
>> >>> >> > devices and some people do pack multiple NICs in a single device)..
>> >>> >> >
>> >>> >> > At the moment many devices in a single pci function can not happen on a
>> >>> >> > PCI system (no multiport) but if we add multiport, we'll follow some
>> >>> >> > existing standard to expose this information to the guest.
>> >>> >>
>> >>> >> This means, that there can currently never multiple devices below one
>> >>> >> and the same virtio parent device?
>> >>>
>> >>> > There's a single virtio device per pci function (you keep saying
>> >>> > device but I hope the distinction is clear and this is
>> >>> > just slip of the tongue).
>> >>>
>> >>> Right, we talks about sysfs directories and they are called "device",
>> >>> we don't really care about the actual bus that is implemented,
>> >>> userspace does not really know much about them. :)
>> >>>
>> >>> > For net devices under a pci function that is also currently the case,
>> >>> > but I can't yet tell you for sure ahead of the time how we'll present
>> >>> > multiport devices if we ever implement them.
>> >>> >
>> >>> > I'm guessing there will be multiple net devices under
>> >>> > a single pci device and we'll present a sysfs attribute with the port
>> >>> > number in this case.
>> >>> >
>> >>> > Hmm maybe we should go ahead and add a place-holder
>> >>> > attribute so that it's future-proof?
>> >>> >
>> >>> > I'll write a patch like that and we'll see how it's accepted.
>> >>>
>> >>> Netdevs with multiple ports are represented with the standard "dev_id"
>> >>> attribute identifying the instance of the driver per parent "device";
>> >>> should all work already from the userspace side, if the virtio side
>> >>> would use that too.
>> >>>
>> >>> Kay
>> >>
>> >> Aha. In that case it's easy - pls assume that if and when we implement
>> >> multiple we'll just follow standards and use dev_id.
>> >> For virtio pci devices specifically virtio<->pci 1:1 mapping is set
>> >> in stone in the spec.
>> >>
>> >> Non pci ones need to be examined separately.
>> >
>> > Nice, thanks. That means we can just "jump over" the "virtio*" device
>> > (device as in sysfs view), and let the pci parent let identify the
>> > device.
>> >
>> > Background: The logic for device naming generally refuses to touch
>> > devices with unknown parent devices in the chain (the directories you
>> > see for: ls -l /sys/class/net/), because the parents *could* offer
>> > their own bus logic that exposes multiple devices below and we would
>> > create clashing names for them. That's why the virt vs. non-virt case
>> > needs to be handled explicitly (seems in this case by just skipping
>> > it) and it is not necessarily by default the same behaviour.
>>
>> So Kay and I discussed this a bit more, and found that we likely
>> cannot handle virtio nic's nicely. As we would name them based on the
>> pci geo, we rely on this being stable between reboots and when
>> adding/removing hardware. Is there some way to make this work with
>> virtio, or will the 'fake' pci busses simply be enumerated as they are
>> created?
>>
>> Cheers,
>>
>> Tom
>
> I think it's standard PCI thing.
> Some pci bridges have a slot id register - I'm assuming you
> are using these if present?
>
> If not you'll either need to rely on firmware enumerating buses
> consistently, or use the mac of the NIC.
Hi Michael,
Sorry to resurrect such an old thread, but this suddenly became
relevant again and I see that we never really reached a conclusion.
Rereading the discussion it is still not really clear to me what the
API promise is, so let me try to rephrase what we need and hopefully
you can help me understand where we are at.
My understanding is that the enumeration of virtioX buses is global
rather than local to the direct parent device of a given bus. That
means that we cannot base deterministic naming of child devices
(functions) on virtio enumeration, as the names would then depend on
the presence or absence of unrelated virtio buses on the system.
We have two options that would allow us to get around this:
1) if there is a guarantee now and in the future (even if only
restricted to netdevs) that no virtio bus will have a direct sibling
bus (i.e., with the same parent device); or
2) if the enumeration of virtio buses was extended with a "port"
numebr which was guaranteed to be made local to their direct parent
device and moreover be made deterministic. I.e., if there are no
virtio sibling buses (but there may be other unrelated virtio buses)
the name would always be virtioX.0, and if there are two buses with
the same parent their names would always be virtioX.0 and virtioX.1
and always in the same order.
Is either of those things something that is already guaranteed or
something that could be made working?
Cheers,
Tom
^ permalink raw reply
* [PATCH] virtio-blk: use VIRTIO_BLK_F_WCE and VIRTIO_BLK_F_CONFIG_WCE in virtio1
From: Paolo Bonzini @ 2015-08-21 22:53 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: open list:VIRTIO CORE, NET AND BLOCK DRIVERS, Michael S. Tsirkin
VIRTIO_BLK_F_CONFIG_WCE is important in order to achieve good performance
(up to 2x, though more realistically +30-40%) in latency-bound workloads.
However, it was removed by mistake together with VIRTIO_BLK_F_FLUSH.
It will be restored in the next revision of the virtio 1.0 standard, so
do the same in Linux.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/block/virtio_blk.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index d4d05f064d39..ea2c17c66dfb 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -478,8 +478,7 @@ static int virtblk_get_cache_mode(struct virtio_device *vdev)
struct virtio_blk_config, wce,
&writeback);
if (err)
- writeback = virtio_has_feature(vdev, VIRTIO_BLK_F_WCE) ||
- virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
+ writeback = virtio_has_feature(vdev, VIRTIO_BLK_F_WCE);
return writeback;
}
@@ -840,7 +839,7 @@ static unsigned int features_legacy[] = {
static unsigned int features[] = {
VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
- VIRTIO_BLK_F_TOPOLOGY,
+ VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
VIRTIO_BLK_F_MQ,
};
--
2.4.3
^ permalink raw reply related
* Re: [PATCH v2 2/2] virtio-net: add default_mtu configuration field
From: Flavio Leitner @ 2015-08-20 19:31 UTC (permalink / raw)
To: Victor Kaplansky; +Cc: virtio-dev, netdev, virtualization, mst
In-Reply-To: <1439732494-29765-3-git-send-email-victork@redhat.com>
On Sun, Aug 16, 2015 at 04:42:25PM +0300, Victor Kaplansky wrote:
> Sometimes it is essential for libvirt to be able to configure MTU
> on guest's NICs to a value different from 1500.
>
> The change adds a new field to configuration area of network
> devices. It will be used to pass initial MTU from the device to
> the driver, and to pass modified MTU from driver to the device
> when a new MTU is assigned by the guest OS.
>
> In addition, in order to support backward and forward
> compatibility, we introduce a new feature bit called
> VIRTIO_NET_F_DEFAULT_MTU.
>
> Added conformance statements for a device and a driver.
>
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
>
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
> content.tex | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 342183b..439d005 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3078,6 +3078,12 @@ features.
>
> \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> channel.
> +
> +\item[VIRTIO_NET_F_DEFAULT_MTU(24)] Default MTU is supported. If
> + offered by the device, device advises driver about initial MTU to
> + be used. If negotiated, the driver uses \field{default_mtu} as
> + an initial value and reports MTU changes to the device.
> +
> \end{description}
>
> \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -3128,6 +3134,7 @@ struct virtio_net_config {
> u8 mac[6];
> le16 status;
> le16 max_virtqueue_pairs;
> + le16 default_mtu;
> };
> \end{lstlisting}
>
> @@ -3158,6 +3165,15 @@ by the driver after negotiation.
> \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
> set and can be read by the driver.
>
> +\item [\field{default_mtu}] is a hint to the driver set by the
> + device. It is valid during feature negotiation only if
> + VIRTIO_NET_F_DEFAULT_MTU is offered and holds the initial value
> + of MTU to be used by the driver. If VIRTIO_NET_F_DEFAULT_MTU is
> + negotiated, the driver uses the \field{default_mtu} as an initial
> + value, and also reports MTU changes to the device by writes to
> + \field{default_mtu}. Such reporting can be used for debugging,
As already said, it's better to change to 'mtu' since changes can
be reported back by writing to the field.
fbl
> + or it can be used for tunning MTU along the network.
> +
> \end{description}
>
> \devicenormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
> @@ -3165,6 +3181,9 @@ by the driver after negotiation.
> The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
> if it offers VIRTIO_NET_F_MQ.
>
> +The device MUST set \field{default_mtu} to between 68 and 65535
> +inclusive, if it offers VIRTIO_NET_F_DEFAULT_MTU.
> +
> \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>
> A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> @@ -3177,6 +3196,12 @@ If the driver does not negotiate the VIRTIO_NET_F_STATUS feature, it SHOULD
> assume the link is active, otherwise it SHOULD read the link status from
> the bottom bit of \field{status}.
>
> +A driver SHOULD negotiate VIRTIO_NET_F_DEFAULT_MTU if the device
> +offers it. If the driver negotiates the VIRTIO_NET_F_DEFAULT_MTU
> +feature, the driver MUST use \field{default_mtu} as an initial value
> +for MTU and the driver MUST report the value of MTU to
> +\field{default_mtu} when MTU is modified by the guest.
> +
> \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
> \label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
> When using the legacy interface, transitional devices and drivers
> --
> --Victor
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-net: add default_mtu configuration field
From: Jason Wang @ 2015-08-20 2:48 UTC (permalink / raw)
To: Victor Kaplansky; +Cc: virtio-dev, netdev, virtualization, mst
In-Reply-To: <20150819140836-mutt-send-email-victork@redhat.com>
On 08/19/2015 07:31 PM, Victor Kaplansky wrote:
> On Mon, Aug 17, 2015 at 11:07:15AM +0800, Jason Wang wrote:
>>
>> On 08/16/2015 09:42 PM, Victor Kaplansky wrote:
>>> @@ -3128,6 +3134,7 @@ struct virtio_net_config {
>>> u8 mac[6];
>>> le16 status;
>>> le16 max_virtqueue_pairs;
>>> + le16 default_mtu;
>> Looks like "mtu" is ok, consider we use "mac" instead of "default_mac".
> Good point. I'll change the name in the next version of the patch.
>
>>> };
>>> \end{lstlisting}
>>>
>>> @@ -3158,6 +3165,15 @@ by the driver after negotiation.
>>> \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
>>> set and can be read by the driver.
>>>
>>> +\item [\field{default_mtu}] is a hint to the driver set by the
>>> + device. It is valid during feature negotiation only if
>>> + VIRTIO_NET_F_DEFAULT_MTU is offered and holds the initial value
>>> + of MTU to be used by the driver. If VIRTIO_NET_F_DEFAULT_MTU is
>>> + negotiated, the driver uses the \field{default_mtu} as an initial
>>> + value, and also reports MTU changes to the device by writes to
>>> + \field{default_mtu}. Such reporting can be used for debugging,
>>> + or it can be used for tunning MTU along the network.
>>> +
>> I vaguely remember that config is read only in some arch or transport
>> and that's why we introduce another vq cmd to confirm the announcement.
>> Probably we should do same for this?
> If so, we need to add one more feature bit to confirm the ability
> of the driver to report MTU, or we can weaken the requirement in
> conformance statement and write "the driver may report the MTU".
> What do you say?
>
> -- Victor
Either looks good for me. (Need confirm the read only issue with the
editors of other transport or arch)
Thanks
^ permalink raw reply
* Re: [PATCH v2 1/2] virtio-net: rephrase devconf fields description
From: Jason Wang @ 2015-08-20 2:46 UTC (permalink / raw)
To: Victor Kaplansky; +Cc: virtio-dev, netdev, virtualization, mst
In-Reply-To: <20150819143550-mutt-send-email-victork@redhat.com>
On 08/19/2015 07:54 PM, Victor Kaplansky wrote:
> On Mon, Aug 17, 2015 at 10:43:46AM +0800, Jason Wang wrote:
>>
>> On 08/16/2015 09:42 PM, Victor Kaplansky wrote:
>>> Clarify general description of the mac, status and
>>> max_virtqueue_pairs fields. Specifically, the old description is
>>> vague about configuration layout and fields offsets when some of
>>> the fields are non valid.
>>>
>>> Also clarify that validity of two status bits depends on two
>>> different feature flags.
>>>
>>> Signed-off-by: Victor Kaplansky <victork@redhat.com>
>>> ---
>>> +
>>> +\item [\field{max_virtqueue_pairs}] tells the driver the maximum
>>> + number of each of virtqueues (receiveq1\ldots receiveqN and
>>> + transmitq1\ldots transmitqN respectively) that can be configured
>>> + on the device once VIRTIO_NET_F_MQ is negotiated.
>>> + \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
>>> + set and can be read by the driver.
>>> +
>>
>> I don't get the point that adding "can be read by the driver". Looks
>> like it's hard for hypervisor to detect this?
> AFAIU, if the device sets VIRTIO_NET_F_MQ, the device also sets
> the value of 'max_virtqueue_pairs' even before driver negotiated
> VIRTIO_NET_F_MQ. If so, the driver can read the value of
> 'max_virtqueue_pairs' during negotiation and potentially this
> value can even affect negotiation decision of the driver.
Looks not, it's legal to negotiate VIRTIO_NET_F_MQ with
max_virtqueue_pairs = 1.
>
> If above is correct, I'll change the description to make this
> point more clear.
>
> Thanks,
> -- Victor
Driver also sets 'mac' before driver negotiated VIRTIO_NET_F_MAC, so I'm
not sure this is really needed.
Thanks
^ permalink raw reply
* [PATCH 2/2] virtio_balloon: do not change memory amount visible via /proc/meminfo
From: Denis V. Lunev @ 2015-08-19 21:49 UTC (permalink / raw)
Cc: Denis V. Lunev, Michael S. Tsirkin, virtualization
In-Reply-To: <1440020989-10824-1-git-send-email-den@openvz.org>
Balloon device is frequently used as a mean of cooperative memory control
in between guest and host to manage memory overcommitment. This is the
typical case for any hosting workload when KVM guest is provided for
end-user.
Though there is a problem in this setup. The end-user and hosting provider
have signed SLA agreement in which some amount of memory is guaranted for
the guest. The good thing is that this memory will be given to the guest
when the guest will really need it (f.e. with OOM in guest and with
VIRTIO_BALLOON_F_DEFLATE_ON_OOM configuration flag set). The bad thing
is that end-user does not know this.
Balloon by default reduce the amount of memory exposed to the end-user
each time when the page is stolen from guest or returned back by using
adjust_managed_page_count and thus /proc/meminfo shows reduced amount
of memory.
Fortunately the solution is simple, we should just avoid to call
adjust_managed_page_count with VIRTIO_BALLOON_F_DEFLATE_ON_OOM set.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_balloon.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8543c9a..7efc329 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -157,7 +157,9 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
}
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
- adjust_managed_page_count(page, -1);
+ if (!virtio_has_feature(vb->vdev,
+ VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
+ adjust_managed_page_count(page, -1);
}
/* Did we get any? */
@@ -173,7 +175,9 @@ static void release_pages_balloon(struct virtio_balloon *vb)
/* Find pfns pointing at start of each page, get pages and free them. */
for (i = 0; i < vb->num_pfns; i += VIRTIO_BALLOON_PAGES_PER_PAGE) {
struct page *page = balloon_pfn_to_page(vb->pfns[i]);
- adjust_managed_page_count(page, 1);
+ if (!virtio_has_feature(vb->vdev,
+ VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
+ adjust_managed_page_count(page, 1);
put_page(page); /* balloon reference */
}
}
--
2.1.4
^ permalink raw reply related
* [PATCH 1/2] virtio_ballon: change stub of release_pages_by_pfn
From: Denis V. Lunev @ 2015-08-19 21:49 UTC (permalink / raw)
Cc: Denis V. Lunev, Michael S. Tsirkin, virtualization
In-Reply-To: <1440020989-10824-1-git-send-email-den@openvz.org>
and rename it to release_pages_balloon. The function originally takes
arrays of pfns and now it takes pointer to struct virtio_ballon.
This change is necessary to conditionally call adjust_managed_page_count
in the next patch.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_balloon.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 82e80e0..8543c9a 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -166,13 +166,13 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
mutex_unlock(&vb->balloon_lock);
}
-static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
+static void release_pages_balloon(struct virtio_balloon *vb)
{
unsigned int i;
/* Find pfns pointing at start of each page, get pages and free them. */
- for (i = 0; i < num; i += VIRTIO_BALLOON_PAGES_PER_PAGE) {
- struct page *page = balloon_pfn_to_page(pfns[i]);
+ for (i = 0; i < vb->num_pfns; i += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+ struct page *page = balloon_pfn_to_page(vb->pfns[i]);
adjust_managed_page_count(page, 1);
put_page(page); /* balloon reference */
}
@@ -206,7 +206,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
if (vb->num_pfns != 0)
tell_host(vb, vb->deflate_vq);
mutex_unlock(&vb->balloon_lock);
- release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ release_pages_balloon(vb);
return num_freed_pages;
}
--
2.1.4
^ permalink raw reply related
* [PATCH 0/2] virtio_balloon: do not change memory amount visible via /proc/meminfo
From: Denis V. Lunev @ 2015-08-19 21:49 UTC (permalink / raw)
Cc: Denis V. Lunev, Michael S. Tsirkin, virtualization
Though there is a problem in this setup. The end-user and hosting provider
have signed SLA agreement in which some amount of memory is guaranted for
the guest. The good thing is that this memory will be given to the guest
when the guest will really need it (f.e. with OOM in guest and with
VIRTIO_BALLOON_F_DEFLATE_ON_OOM configuration flag set). The bad thing
is that end-user does not know this.
Balloon by default reduce the amount of memory exposed to the end-user
each time when the page is stolen from guest or returned back by using
adjust_managed_page_count and thus /proc/meminfo shows reduced amount
of memory.
Fortunately the solution is simple, we should just avoid to call
adjust_managed_page_count with VIRTIO_BALLOON_F_DEFLATE_ON_OOM set.
Please note that neither VMWare ballon nor HyperV balloon do not care
about proper handling of adjust_managed_page_count at all.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
^ permalink raw reply
* Re: [PATCH v2 1/2] virtio-net: rephrase devconf fields description
From: Victor Kaplansky @ 2015-08-19 11:54 UTC (permalink / raw)
To: Jason Wang; +Cc: virtio-dev, netdev, virtualization, mst
In-Reply-To: <55D14A62.2060408@redhat.com>
On Mon, Aug 17, 2015 at 10:43:46AM +0800, Jason Wang wrote:
>
>
> On 08/16/2015 09:42 PM, Victor Kaplansky wrote:
> > Clarify general description of the mac, status and
> > max_virtqueue_pairs fields. Specifically, the old description is
> > vague about configuration layout and fields offsets when some of
> > the fields are non valid.
> >
> > Also clarify that validity of two status bits depends on two
> > different feature flags.
> >
> > Signed-off-by: Victor Kaplansky <victork@redhat.com>
> > ---
> > +
> > +\item [\field{max_virtqueue_pairs}] tells the driver the maximum
> > + number of each of virtqueues (receiveq1\ldots receiveqN and
> > + transmitq1\ldots transmitqN respectively) that can be configured
> > + on the device once VIRTIO_NET_F_MQ is negotiated.
> > + \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
> > + set and can be read by the driver.
> > +
>
>
> I don't get the point that adding "can be read by the driver". Looks
> like it's hard for hypervisor to detect this?
AFAIU, if the device sets VIRTIO_NET_F_MQ, the device also sets
the value of 'max_virtqueue_pairs' even before driver negotiated
VIRTIO_NET_F_MQ. If so, the driver can read the value of
'max_virtqueue_pairs' during negotiation and potentially this
value can even affect negotiation decision of the driver.
If above is correct, I'll change the description to make this
point more clear.
Thanks,
-- Victor
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-net: add default_mtu configuration field
From: Victor Kaplansky @ 2015-08-19 11:31 UTC (permalink / raw)
To: Jason Wang; +Cc: virtio-dev, netdev, virtualization, mst
In-Reply-To: <55D14FE3.8040307@redhat.com>
On Mon, Aug 17, 2015 at 11:07:15AM +0800, Jason Wang wrote:
>
>
> On 08/16/2015 09:42 PM, Victor Kaplansky wrote:
> > @@ -3128,6 +3134,7 @@ struct virtio_net_config {
> > u8 mac[6];
> > le16 status;
> > le16 max_virtqueue_pairs;
> > + le16 default_mtu;
>
> Looks like "mtu" is ok, consider we use "mac" instead of "default_mac".
Good point. I'll change the name in the next version of the patch.
>
> > };
> > \end{lstlisting}
> >
> > @@ -3158,6 +3165,15 @@ by the driver after negotiation.
> > \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
> > set and can be read by the driver.
> >
> > +\item [\field{default_mtu}] is a hint to the driver set by the
> > + device. It is valid during feature negotiation only if
> > + VIRTIO_NET_F_DEFAULT_MTU is offered and holds the initial value
> > + of MTU to be used by the driver. If VIRTIO_NET_F_DEFAULT_MTU is
> > + negotiated, the driver uses the \field{default_mtu} as an initial
> > + value, and also reports MTU changes to the device by writes to
> > + \field{default_mtu}. Such reporting can be used for debugging,
> > + or it can be used for tunning MTU along the network.
> > +
>
> I vaguely remember that config is read only in some arch or transport
> and that's why we introduce another vq cmd to confirm the announcement.
> Probably we should do same for this?
If so, we need to add one more feature bit to confirm the ability
of the driver to report MTU, or we can weaken the requirement in
conformance statement and write "the driver may report the MTU".
What do you say?
-- Victor
^ permalink raw reply
* Re: [PATCH 1/4] virtio_net: use DECLARE_EWMA
From: Johannes Berg @ 2015-08-19 7:48 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: virtualization, Michael S. Tsirkin
In-Reply-To: <1439970382-6170-2-git-send-email-johannes@sipsolutions.net>
Sorry, forgot to Cc Michael and the virt list - patch reproduced below
in full.
johannes
From 22500fbcf722748fe3471b2e4c6156db47aade15 Mon Sep 17 00:00:00 2001
From: Johannes Berg <johannes.berg@intel.com>
Date: Wed, 19 Aug 2015 09:25:18 +0200
Subject: [PATCH] virtio_net: use DECLARE_EWMA
Instead of using the out-of-line EWMA calculation, use DECLARE_EWMA()
to create static inlines. On x86/64 this results in no change in code
size for me, but reduces the struct receive_queue size by the two
unsigned long values that store the parameters.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
drivers/net/Kconfig | 1 -
drivers/net/virtio_net.c | 22 +++++++++++-----------
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index e58468b02987..f50373645ab4 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -282,7 +282,6 @@ config VETH
config VIRTIO_NET
tristate "Virtio network driver"
depends on VIRTIO
- select AVERAGE
---help---
This is the virtual network driver for virtio. It can be used with
lguest or QEMU based VMMs (like KVM or Xen). Say Y or M.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 546b669fbfdd..9b950f2db836 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -40,12 +40,12 @@ module_param(gso, bool, 0444);
#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
#define GOOD_COPY_LEN 128
-/* Weight used for the RX packet size EWMA. The average packet size is used to
- * determine the packet buffer size when refilling RX rings. As the entire RX
- * ring may be refilled at once, the weight is chosen so that the EWMA will be
- * insensitive to short-term, transient changes in packet size.
+/* RX packet size EWMA. The average packet size is used to determine the packet
+ * buffer size when refilling RX rings. As the entire RX ring may be refilled
+ * at once, the weight is chosen so that the EWMA will be insensitive to short-
+ * term, transient changes in packet size.
*/
-#define RECEIVE_AVG_WEIGHT 64
+DECLARE_EWMA(pkt_len, 1, 64)
/* Minimum alignment for mergeable packet buffers. */
#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, 256)
@@ -85,7 +85,7 @@ struct receive_queue {
struct page *pages;
/* Average packet length for mergeable receive buffers. */
- struct ewma mrg_avg_pkt_len;
+ struct ewma_pkt_len mrg_avg_pkt_len;
/* Page frag for packet buffer allocation. */
struct page_frag alloc_frag;
@@ -407,7 +407,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
}
- ewma_add(&rq->mrg_avg_pkt_len, head_skb->len);
+ ewma_pkt_len_add(&rq->mrg_avg_pkt_len, head_skb->len);
return head_skb;
err_skb:
@@ -600,12 +600,12 @@ static int add_recvbuf_big(struct virtnet_info *vi, struct receive_queue *rq,
return err;
}
-static unsigned int get_mergeable_buf_len(struct ewma *avg_pkt_len)
+static unsigned int get_mergeable_buf_len(struct ewma_pkt_len *avg_pkt_len)
{
const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
unsigned int len;
- len = hdr_len + clamp_t(unsigned int, ewma_read(avg_pkt_len),
+ len = hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
GOOD_PACKET_LEN, PAGE_SIZE - hdr_len);
return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
}
@@ -1615,7 +1615,7 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
napi_hash_add(&vi->rq[i].napi);
sg_init_table(vi->rq[i].sg, ARRAY_SIZE(vi->rq[i].sg));
- ewma_init(&vi->rq[i].mrg_avg_pkt_len, 1, RECEIVE_AVG_WEIGHT);
+ ewma_pkt_len_init(&vi->rq[i].mrg_avg_pkt_len);
sg_init_table(vi->sq[i].sg, ARRAY_SIZE(vi->sq[i].sg));
}
@@ -1658,7 +1658,7 @@ static ssize_t mergeable_rx_buffer_size_show(struct netdev_rx_queue *queue,
{
struct virtnet_info *vi = netdev_priv(queue->dev);
unsigned int queue_index = get_netdev_rx_queue_index(queue);
- struct ewma *avg;
+ struct ewma_pkt_len *avg;
BUG_ON(queue_index >= vi->max_queue_pairs);
avg = &vi->rq[queue_index].mrg_avg_pkt_len;
^ permalink raw reply related
* Re: [PATCH v2 2/2] virtio-net: add default_mtu configuration field
From: Jason Wang @ 2015-08-17 3:07 UTC (permalink / raw)
To: Victor Kaplansky, virtio-dev; +Cc: netdev, virtualization, mst
In-Reply-To: <1439732494-29765-3-git-send-email-victork@redhat.com>
On 08/16/2015 09:42 PM, Victor Kaplansky wrote:
> Sometimes it is essential for libvirt to be able to configure MTU
> on guest's NICs to a value different from 1500.
>
> The change adds a new field to configuration area of network
> devices. It will be used to pass initial MTU from the device to
> the driver, and to pass modified MTU from driver to the device
> when a new MTU is assigned by the guest OS.
>
> In addition, in order to support backward and forward
> compatibility, we introduce a new feature bit called
> VIRTIO_NET_F_DEFAULT_MTU.
>
> Added conformance statements for a device and a driver.
>
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
>
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
> content.tex | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 342183b..439d005 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3078,6 +3078,12 @@ features.
>
> \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> channel.
> +
> +\item[VIRTIO_NET_F_DEFAULT_MTU(24)] Default MTU is supported. If
> + offered by the device, device advises driver about initial MTU to
> + be used. If negotiated, the driver uses \field{default_mtu} as
> + an initial value and reports MTU changes to the device.
> +
> \end{description}
>
> \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -3128,6 +3134,7 @@ struct virtio_net_config {
> u8 mac[6];
> le16 status;
> le16 max_virtqueue_pairs;
> + le16 default_mtu;
Looks like "mtu" is ok, consider we use "mac" instead of "default_mac".
> };
> \end{lstlisting}
>
> @@ -3158,6 +3165,15 @@ by the driver after negotiation.
> \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
> set and can be read by the driver.
>
> +\item [\field{default_mtu}] is a hint to the driver set by the
> + device. It is valid during feature negotiation only if
> + VIRTIO_NET_F_DEFAULT_MTU is offered and holds the initial value
> + of MTU to be used by the driver. If VIRTIO_NET_F_DEFAULT_MTU is
> + negotiated, the driver uses the \field{default_mtu} as an initial
> + value, and also reports MTU changes to the device by writes to
> + \field{default_mtu}. Such reporting can be used for debugging,
> + or it can be used for tunning MTU along the network.
> +
I vaguely remember that config is read only in some arch or transport
and that's why we introduce another vq cmd to confirm the announcement.
Probably we should do same for this?
^ permalink raw reply
* Re: [PATCH v2 1/2] virtio-net: rephrase devconf fields description
From: Jason Wang @ 2015-08-17 2:43 UTC (permalink / raw)
To: Victor Kaplansky, virtio-dev; +Cc: netdev, virtualization, mst
In-Reply-To: <1439732494-29765-2-git-send-email-victork@redhat.com>
On 08/16/2015 09:42 PM, Victor Kaplansky wrote:
> Clarify general description of the mac, status and
> max_virtqueue_pairs fields. Specifically, the old description is
> vague about configuration layout and fields offsets when some of
> the fields are non valid.
>
> Also clarify that validity of two status bits depends on two
> different feature flags.
>
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
> content.tex | 42 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index d989d98..342183b 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3115,23 +3115,14 @@ were needed.
> \subsection{Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout}
> \label{sec:Device Types / Block Device / Feature bits / Device configuration layout}
>
> -Three driver-read-only configuration fields are currently defined. The \field{mac} address field
> -always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
> -\field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
> -read-only bits (for the driver) are currently defined for the status field:
> -VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> +
> +The following configuration fields are currently defined:
>
> \begin{lstlisting}
> #define VIRTIO_NET_S_LINK_UP 1
> #define VIRTIO_NET_S_ANNOUNCE 2
> \end{lstlisting}
>
> -The following driver-read-only field, \field{max_virtqueue_pairs} only exists if
> -VIRTIO_NET_F_MQ is set. This field specifies the maximum number
> -of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
> -and transmitq1\ldots transmitqN respectively) that can be configured once VIRTIO_NET_F_MQ
> -is negotiated.
> -
> \begin{lstlisting}
> struct virtio_net_config {
> u8 mac[6];
> @@ -3140,6 +3131,35 @@ struct virtio_net_config {
> };
> \end{lstlisting}
>
> +\field{mac}, \field{status}, \field{max_virtqueue_pairs} are
> +driver-read-only. \field{default_mtu} can also be written
> +by the driver after negotiation.
> +
> +\begin{description}
> +\item [\field{mac}] is MAC address assigned by the device and is valid
> + only if VIRTIO_NET_F_MAC is offered.
> +
> +\item [\field{status}] consists of two driver-read-only status bits:
> +
> +\begin{description}
> +
> +\item VIRTIO_NET_S_LINK_UP is valid if VIRTIO_NET_F_STATUS is
> + offered.
> +
> +\item VIRTIO_NET_S_ANNOUNCE is valid if
> + VIRTIO_NET_F_GUEST_ANNOUNCE is offered.
> +
> +\end{description}
> +
> +\item [\field{max_virtqueue_pairs}] tells the driver the maximum
> + number of each of virtqueues (receiveq1\ldots receiveqN and
> + transmitq1\ldots transmitqN respectively) that can be configured
> + on the device once VIRTIO_NET_F_MQ is negotiated.
> + \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
> + set and can be read by the driver.
> +
Hi:
I don't get the point that adding "can be read by the driver". Looks
like it's hard for hypervisor to detect this?
Thanks
^ permalink raw reply
* [PATCH v2 2/2] virtio-net: add default_mtu configuration field
From: Victor Kaplansky @ 2015-08-16 13:42 UTC (permalink / raw)
To: virtio-dev; +Cc: netdev, virtualization, mst
In-Reply-To: <1439732494-29765-1-git-send-email-victork@redhat.com>
Sometimes it is essential for libvirt to be able to configure MTU
on guest's NICs to a value different from 1500.
The change adds a new field to configuration area of network
devices. It will be used to pass initial MTU from the device to
the driver, and to pass modified MTU from driver to the device
when a new MTU is assigned by the guest OS.
In addition, in order to support backward and forward
compatibility, we introduce a new feature bit called
VIRTIO_NET_F_DEFAULT_MTU.
Added conformance statements for a device and a driver.
Signed-off-by: Victor Kaplansky <victork@redhat.com>
Signed-off-by: Victor Kaplansky <victork@redhat.com>
---
content.tex | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/content.tex b/content.tex
index 342183b..439d005 100644
--- a/content.tex
+++ b/content.tex
@@ -3078,6 +3078,12 @@ features.
\item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
channel.
+
+\item[VIRTIO_NET_F_DEFAULT_MTU(24)] Default MTU is supported. If
+ offered by the device, device advises driver about initial MTU to
+ be used. If negotiated, the driver uses \field{default_mtu} as
+ an initial value and reports MTU changes to the device.
+
\end{description}
\subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
@@ -3128,6 +3134,7 @@ struct virtio_net_config {
u8 mac[6];
le16 status;
le16 max_virtqueue_pairs;
+ le16 default_mtu;
};
\end{lstlisting}
@@ -3158,6 +3165,15 @@ by the driver after negotiation.
\field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
set and can be read by the driver.
+\item [\field{default_mtu}] is a hint to the driver set by the
+ device. It is valid during feature negotiation only if
+ VIRTIO_NET_F_DEFAULT_MTU is offered and holds the initial value
+ of MTU to be used by the driver. If VIRTIO_NET_F_DEFAULT_MTU is
+ negotiated, the driver uses the \field{default_mtu} as an initial
+ value, and also reports MTU changes to the device by writes to
+ \field{default_mtu}. Such reporting can be used for debugging,
+ or it can be used for tunning MTU along the network.
+
\end{description}
\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
@@ -3165,6 +3181,9 @@ by the driver after negotiation.
The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
if it offers VIRTIO_NET_F_MQ.
+The device MUST set \field{default_mtu} to between 68 and 65535
+inclusive, if it offers VIRTIO_NET_F_DEFAULT_MTU.
+
\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
@@ -3177,6 +3196,12 @@ If the driver does not negotiate the VIRTIO_NET_F_STATUS feature, it SHOULD
assume the link is active, otherwise it SHOULD read the link status from
the bottom bit of \field{status}.
+A driver SHOULD negotiate VIRTIO_NET_F_DEFAULT_MTU if the device
+offers it. If the driver negotiates the VIRTIO_NET_F_DEFAULT_MTU
+feature, the driver MUST use \field{default_mtu} as an initial value
+for MTU and the driver MUST report the value of MTU to
+\field{default_mtu} when MTU is modified by the guest.
+
\subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
\label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
When using the legacy interface, transitional devices and drivers
--
--Victor
^ permalink raw reply related
* [PATCH v2 1/2] virtio-net: rephrase devconf fields description
From: Victor Kaplansky @ 2015-08-16 13:42 UTC (permalink / raw)
To: virtio-dev; +Cc: netdev, virtualization, mst
In-Reply-To: <1439732494-29765-1-git-send-email-victork@redhat.com>
Clarify general description of the mac, status and
max_virtqueue_pairs fields. Specifically, the old description is
vague about configuration layout and fields offsets when some of
the fields are non valid.
Also clarify that validity of two status bits depends on two
different feature flags.
Signed-off-by: Victor Kaplansky <victork@redhat.com>
---
content.tex | 42 +++++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/content.tex b/content.tex
index d989d98..342183b 100644
--- a/content.tex
+++ b/content.tex
@@ -3115,23 +3115,14 @@ were needed.
\subsection{Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout}
\label{sec:Device Types / Block Device / Feature bits / Device configuration layout}
-Three driver-read-only configuration fields are currently defined. The \field{mac} address field
-always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
-\field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
-read-only bits (for the driver) are currently defined for the status field:
-VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
+
+The following configuration fields are currently defined:
\begin{lstlisting}
#define VIRTIO_NET_S_LINK_UP 1
#define VIRTIO_NET_S_ANNOUNCE 2
\end{lstlisting}
-The following driver-read-only field, \field{max_virtqueue_pairs} only exists if
-VIRTIO_NET_F_MQ is set. This field specifies the maximum number
-of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
-and transmitq1\ldots transmitqN respectively) that can be configured once VIRTIO_NET_F_MQ
-is negotiated.
-
\begin{lstlisting}
struct virtio_net_config {
u8 mac[6];
@@ -3140,6 +3131,35 @@ struct virtio_net_config {
};
\end{lstlisting}
+\field{mac}, \field{status}, \field{max_virtqueue_pairs} are
+driver-read-only. \field{default_mtu} can also be written
+by the driver after negotiation.
+
+\begin{description}
+\item [\field{mac}] is MAC address assigned by the device and is valid
+ only if VIRTIO_NET_F_MAC is offered.
+
+\item [\field{status}] consists of two driver-read-only status bits:
+
+\begin{description}
+
+\item VIRTIO_NET_S_LINK_UP is valid if VIRTIO_NET_F_STATUS is
+ offered.
+
+\item VIRTIO_NET_S_ANNOUNCE is valid if
+ VIRTIO_NET_F_GUEST_ANNOUNCE is offered.
+
+\end{description}
+
+\item [\field{max_virtqueue_pairs}] tells the driver the maximum
+ number of each of virtqueues (receiveq1\ldots receiveqN and
+ transmitq1\ldots transmitqN respectively) that can be configured
+ on the device once VIRTIO_NET_F_MQ is negotiated.
+ \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
+ set and can be read by the driver.
+
+\end{description}
+
\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
--
--Victor
^ permalink raw reply related
* [PATCH v2 0/2] virtio-net: default_mtu - new conf. field
From: Victor Kaplansky @ 2015-08-16 13:42 UTC (permalink / raw)
To: virtio-dev; +Cc: netdev, virtualization, mst
This set of two patches adds a new field called default_mtu to
the configuration area of network devices. The motivation is to
allow libvirt to set initial MTU different from 1500 on guests
virtual NICs. We also propose to use this new field to report
MTU changes by the guest OS to the device to facilitate
debugging and mtu tunning.
The first patch just clarify the definition of existing fields in
network device configuration are. The second one adds default_mtu
and its description.
v2 changes:
* address comments by Michael S. Tsirkin:
- mac is driver-read-only.
- two status bits depend on different feature flags.
- use should/must only in conformance statements.
- rephrase "is set" -> "is offered" for features.
- other cosmetic changes.
- added conformance statements for a device and for a driver
regarding new 'default_mtu" field.
Victor Kaplansky (2):
virtio-net: rephrase devconf fields description
virtio-net: add default_mtu configuration field
content.tex | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 56 insertions(+), 11 deletions(-)
--
--Victor
^ permalink raw reply
* Re: [PATCH 2/4] mm/compaction: enable mobile-page migration
From: Minchan Kim @ 2015-08-10 7:19 UTC (permalink / raw)
To: Gioh Kim
Cc: aquini, mst, linux-api, linux-kernel, dri-devel, virtualization,
bfields, linux-mm, Gioh Kim, viro, akpm, linux-fsdevel, jlayton,
koct9i, iamjoonsoo.kim, vbabka
In-Reply-To: <20150731104309.GA22158@bbox>
On Fri, Jul 31, 2015 at 07:43:09PM +0900, Minchan Kim wrote:
> Hi Gioh,
>
> On Mon, Jul 13, 2015 at 05:35:17PM +0900, Gioh Kim wrote:
> > From: Gioh Kim <gurugio@hanmail.net>
> >
> > Add framework to register callback functions and check page mobility.
> > There are some modes for page isolation so that isolate interface
> > has arguments of page address and isolation mode while putback
> > interface has only page address as argument.
> >
> > Signed-off-by: Gioh Kim <gioh.kim@lge.com>
> > Acked-by: Rafael Aquini <aquini@redhat.com>
> > ---
> > fs/proc/page.c | 3 ++
> > include/linux/compaction.h | 80 ++++++++++++++++++++++++++++++++++
> > include/linux/fs.h | 2 +
> > include/linux/page-flags.h | 19 ++++++++
> > include/uapi/linux/kernel-page-flags.h | 1 +
> > 5 files changed, 105 insertions(+)
> >
> > diff --git a/fs/proc/page.c b/fs/proc/page.c
> > index 7eee2d8..a4f5a00 100644
> > --- a/fs/proc/page.c
> > +++ b/fs/proc/page.c
> > @@ -146,6 +146,9 @@ u64 stable_page_flags(struct page *page)
> > if (PageBalloon(page))
> > u |= 1 << KPF_BALLOON;
> >
> > + if (PageMobile(page))
> > + u |= 1 << KPF_MOBILE;
> > +
>
> Need a new page flag for non-LRU page migration?
> I am worry that we don't have empty slot for page flag on 32-bit.
>
> Aha, see SetPageMobile below. You use _mapcount.
> It seems to be work for non-LRU pages but unfortunately, zsmalloc
> already have used that field as own purpose so there is no room
> for it.
Gioh, I just sent a patch which zsmalloc doesn't use page->mapping
and _mapcount so I think zsmalloc could support compaction with your
patchset. Although zsmalloc can support compaction with it, I still
don't think it's a good that driver have to mark pages mobile via
_mapcount.
I hope you can find another way.
Thanks.
>
>
> > u |= kpf_copy_bit(k, KPF_LOCKED, PG_locked);
> >
> > u |= kpf_copy_bit(k, KPF_SLAB, PG_slab);
> > diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> > index aa8f61c..f693072 100644
> > --- a/include/linux/compaction.h
> > +++ b/include/linux/compaction.h
> > @@ -1,6 +1,9 @@
> > #ifndef _LINUX_COMPACTION_H
> > #define _LINUX_COMPACTION_H
> >
> > +#include <linux/page-flags.h>
> > +#include <linux/pagemap.h>
> > +
> > /* Return values for compact_zone() and try_to_compact_pages() */
> > /* compaction didn't start as it was deferred due to past failures */
> > #define COMPACT_DEFERRED 0
> > @@ -51,6 +54,70 @@ extern void compaction_defer_reset(struct zone *zone, int order,
> > bool alloc_success);
> > extern bool compaction_restarting(struct zone *zone, int order);
> >
> > +static inline bool mobile_page(struct page *page)
> > +{
> > + return page->mapping && (PageMobile(page) || PageBalloon(page));
>
>
> What's the locking rule to test mobile_page?
> Why we need such many checking?
>
> Why we need PageBalloon check?
> You are trying to make generic abstraction for non-LRU page to migrate
> but PageBalloon check in here breaks your goal.
>
> > +}
> > +
> > +static inline bool isolate_mobilepage(struct page *page, isolate_mode_t mode)
> > +{
> > + bool ret = false;
> > +
> > + /*
> > + * Avoid burning cycles with pages that are yet under __free_pages(),
> > + * or just got freed under us.
> > + *
> > + * In case we 'win' a race for a mobile page being freed under us and
> > + * raise its refcount preventing __free_pages() from doing its job
> > + * the put_page() at the end of this block will take care of
> > + * release this page, thus avoiding a nasty leakage.
> > + */
>
> Good.
>
> > + if (unlikely(!get_page_unless_zero(page)))
> > + goto out;
> > +
> > + /*
> > + * As mobile pages are not isolated from LRU lists, concurrent
> > + * compaction threads can race against page migration functions
> > + * as well as race against the releasing a page.
>
> How can it race with releasing a page?
> We already get refcount above.
>
> Do you mean page->mapping tearing race?
> If so, zsmalloc should be chaned to hold a lock.
>
>
> > + *
> > + * In order to avoid having an already isolated mobile page
> > + * being (wrongly) re-isolated while it is under migration,
> > + * or to avoid attempting to isolate pages being released,
> > + * lets be sure we have the page lock
> > + * before proceeding with the mobile page isolation steps.
> > + */
> > + if (unlikely(!trylock_page(page)))
> > + goto out_putpage;
> > +
> > + if (!(mobile_page(page) && page->mapping->a_ops->isolatepage))
> > + goto out_not_isolated;
>
> I cannot know how isolate_mobilepage is called by upper layer
> because this patch doesn't include it.
> Anyway, why we need such many checks to prove it's mobile page?
>
> mobile_page() {
> page->mapping && (PageMobile(page) || PageBalloon(page));
> }
>
> Additionally, added page->mapping->a_ops->isolatepage check
>
> Is it possible that a non-LRU page's address space provides
> only page->maping->migratepage without isolate/putback?
>
> Couldn't we make it simple like this?
>
> page->mapping && page->mapping->migratepage
>
> So, couldn't we make mobile_page like this
>
> PageMobile(struct page *page)
> {
> VM_BUG_ON_PAGE(!PageLocked(page), page);
> VM_BUG_ON_PAGE(PageLRU(page), page);
>
> return page->mapping && page->mapping->migratepage;
> }
>
> It will save _mapcount of struct page.
>
> > + ret = page->mapping->a_ops->isolatepage(page, mode);
> > + if (!ret)
> > + goto out_not_isolated;
> > + unlock_page(page);
> > + return ret;
> > +
> > +out_not_isolated:
> > + unlock_page(page);
> > +out_putpage:
> > + put_page(page);
> > +out:
> > + return ret;
> > +}
> > +
> > +static inline void putback_mobilepage(struct page *page)
> > +{
> > + /*
> > + * 'lock_page()' stabilizes the page and prevents races against
>
> What does it mean 'stabilize'?
> Clear comment is always welcome rather than vague word.
>
> > + * concurrent isolation threads attempting to re-isolate it.
> > + */
> > + lock_page(page);
> > + if (page->mapping && page->mapping->a_ops->putbackpage)
> > + page->mapping->a_ops->putbackpage(page);
> > + unlock_page(page);
> > + /* drop the extra ref count taken for mobile page isolation */
> > + put_page(page);
> > +}
> > #else
> > static inline unsigned long try_to_compact_pages(gfp_t gfp_mask,
> > unsigned int order, int alloc_flags,
> > @@ -83,6 +150,19 @@ static inline bool compaction_deferred(struct zone *zone, int order)
> > return true;
> > }
> >
> > +static inline bool mobile_page(struct page *page)
> > +{
> > + return false;
> > +}
> > +
> > +static inline bool isolate_mobilepage(struct page *page, isolate_mode_t mode)
> > +{
> > + return false;
> > +}
> > +
> > +static inline void putback_mobilepage(struct page *page)
> > +{
> > +}
> > #endif /* CONFIG_COMPACTION */
> >
> > #if defined(CONFIG_COMPACTION) && defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index a0653e5..2cc4b24 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -396,6 +396,8 @@ struct address_space_operations {
> > */
> > int (*migratepage) (struct address_space *,
> > struct page *, struct page *, enum migrate_mode);
> > + bool (*isolatepage) (struct page *, isolate_mode_t);
> > + void (*putbackpage) (struct page *);
> > int (*launder_page) (struct page *);
> > int (*is_partially_uptodate) (struct page *, unsigned long,
> > unsigned long);
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index f34e040..abef145 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -582,6 +582,25 @@ static inline void __ClearPageBalloon(struct page *page)
> > atomic_set(&page->_mapcount, -1);
> > }
> >
> > +#define PAGE_MOBILE_MAPCOUNT_VALUE (-255)
> > +
> > +static inline int PageMobile(struct page *page)
> > +{
> > + return atomic_read(&page->_mapcount) == PAGE_MOBILE_MAPCOUNT_VALUE;
> > +}
> > +
> > +static inline void __SetPageMobile(struct page *page)
> > +{
> > + VM_BUG_ON_PAGE(atomic_read(&page->_mapcount) != -1, page);
> > + atomic_set(&page->_mapcount, PAGE_MOBILE_MAPCOUNT_VALUE);
> > +}
> > +
> > +static inline void __ClearPageMobile(struct page *page)
> > +{
> > + VM_BUG_ON_PAGE(!PageMobile(page), page);
> > + atomic_set(&page->_mapcount, -1);
> > +}
> > +
> > /*
> > * If network-based swap is enabled, sl*b must keep track of whether pages
> > * were allocated from pfmemalloc reserves.
> > diff --git a/include/uapi/linux/kernel-page-flags.h b/include/uapi/linux/kernel-page-flags.h
> > index a6c4962..d50d9e8 100644
> > --- a/include/uapi/linux/kernel-page-flags.h
> > +++ b/include/uapi/linux/kernel-page-flags.h
> > @@ -33,6 +33,7 @@
> > #define KPF_THP 22
> > #define KPF_BALLOON 23
> > #define KPF_ZERO_PAGE 24
> > +#define KPF_MOBILE 25
> >
> >
> > #endif /* _UAPILINUX_KERNEL_PAGE_FLAGS_H */
> > --
> > 2.1.4
> >
^ 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