* Re: [PATCH v2 1/1] virtio_balloon: include buffers and cached memory statistics
From: Tomáš Golembiovský @ 2017-10-31 12:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, kvm, Gal Hammer, qemu-devel, virtualization, linux-mm,
Shaohua Li, Huang Ying
In-Reply-To: <20171022200557.02558e37@fiorina>
ping
+Gil, +Amnon... could you maybe aid in reviewing the patch, please?
Tomas
On Sun, 22 Oct 2017 20:05:57 +0200
Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> On Thu, 19 Oct 2017 16:12:20 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Thu, Sep 21, 2017 at 02:55:41PM +0200, Tomáš Golembiovský wrote:
> > > Add a new fields, VIRTIO_BALLOON_S_BUFFERS and VIRTIO_BALLOON_S_CACHED,
> > > to virtio_balloon memory statistics protocol. The values correspond to
> > > 'Buffers' and 'Cached' in /proc/meminfo.
> > >
> > > To be able to compute the value of 'Cached' memory it is necessary to
> > > export total_swapcache_pages() to modules.
> > >
> > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> >
> > Does 'Buffers' actually make sense? It's a temporary storage -
> > wouldn't it be significantly out of date by the time
> > host receives it?
>
> That would be best answered by somebody from kernel. But my personal
> opinion is that it would not be out of date. The amount of memory
> dedicated to Buffers does not seem to fluctuate too much.
>
> Tomas
>
>
> > > ---
> > > drivers/virtio/virtio_balloon.c | 11 +++++++++++
> > > include/uapi/linux/virtio_balloon.h | 4 +++-
> > > mm/swap_state.c | 1 +
> > > 3 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > index f0b3a0b9d42f..c2558ec47a62 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -244,12 +244,19 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> > > struct sysinfo i;
> > > unsigned int idx = 0;
> > > long available;
> > > + long cached;
> > >
> > > all_vm_events(events);
> > > si_meminfo(&i);
> > >
> > > available = si_mem_available();
> > >
> > > + cached = global_node_page_state(NR_FILE_PAGES) -
> > > + total_swapcache_pages() - i.bufferram;
> > > + if (cached < 0)
> > > + cached = 0;
> > > +
> > > +
> > > #ifdef CONFIG_VM_EVENT_COUNTERS
> > > update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> > > pages_to_bytes(events[PSWPIN]));
> > > @@ -264,6 +271,10 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> > > pages_to_bytes(i.totalram));
> > > update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
> > > pages_to_bytes(available));
> > > + update_stat(vb, idx++, VIRTIO_BALLOON_S_BUFFERS,
> > > + pages_to_bytes(i.bufferram));
> > > + update_stat(vb, idx++, VIRTIO_BALLOON_S_CACHED,
> > > + pages_to_bytes(cached));
> > >
> > > return idx;
> > > }
> > > diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> > > index 343d7ddefe04..d5dc8a56a497 100644
> > > --- a/include/uapi/linux/virtio_balloon.h
> > > +++ b/include/uapi/linux/virtio_balloon.h
> > > @@ -52,7 +52,9 @@ struct virtio_balloon_config {
> > > #define VIRTIO_BALLOON_S_MEMFREE 4 /* Total amount of free memory */
> > > #define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
> > > #define VIRTIO_BALLOON_S_AVAIL 6 /* Available memory as in /proc */
> > > -#define VIRTIO_BALLOON_S_NR 7
> > > +#define VIRTIO_BALLOON_S_BUFFERS 7 /* Buffers memory as in /proc */
> > > +#define VIRTIO_BALLOON_S_CACHED 8 /* Cached memory as in /proc */
> > > +#define VIRTIO_BALLOON_S_NR 9
> > >
> > > /*
> > > * Memory statistics structure.
> > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > index 71ce2d1ccbf7..f3a4ff7d6c52 100644
> > > --- a/mm/swap_state.c
> > > +++ b/mm/swap_state.c
> > > @@ -95,6 +95,7 @@ unsigned long total_swapcache_pages(void)
> > > rcu_read_unlock();
> > > return ret;
> > > }
> > > +EXPORT_SYMBOL_GPL(total_swapcache_pages);
> > >
> > > static atomic_t swapin_readahead_hits = ATOMIC_INIT(4);
> >
> > Need an ack from MM crowd on that.
> >
> > > --
> > > 2.14.1
>
>
> --
> Tomáš Golembiovský <tgolembi@redhat.com>
--
Tomáš Golembiovský <tgolembi@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 1/1] virtio_balloon: include buffers and cached memory statistics
From: Michael S. Tsirkin @ 2017-10-31 16:15 UTC (permalink / raw)
To: Tomáš Golembiovský
Cc: virtio-dev, riel, kvm, Gal Hammer, qemu-devel, virtualization,
linux-mm, Shaohua Li, Huang Ying
In-Reply-To: <20171031132019.76197945@fiorina>
On Tue, Oct 31, 2017 at 01:20:19PM +0100, Tomáš Golembiovský wrote:
> ping
>
> +Gil, +Amnon... could you maybe aid in reviewing the patch, please?
>
>
> Tomas
>
> On Sun, 22 Oct 2017 20:05:57 +0200
> Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> > On Thu, 19 Oct 2017 16:12:20 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Thu, Sep 21, 2017 at 02:55:41PM +0200, Tomáš Golembiovský wrote:
> > > > Add a new fields, VIRTIO_BALLOON_S_BUFFERS and VIRTIO_BALLOON_S_CACHED,
> > > > to virtio_balloon memory statistics protocol. The values correspond to
> > > > 'Buffers' and 'Cached' in /proc/meminfo.
> > > >
> > > > To be able to compute the value of 'Cached' memory it is necessary to
> > > > export total_swapcache_pages() to modules.
> > > >
> > > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > >
> > > Does 'Buffers' actually make sense? It's a temporary storage -
> > > wouldn't it be significantly out of date by the time
> > > host receives it?
> >
> > That would be best answered by somebody from kernel. But my personal
> > opinion is that it would not be out of date. The amount of memory
> > dedicated to Buffers does not seem to fluctuate too much.
> >
> > Tomas
> >
I would be inclined to say, just report
global_node_page_state(NR_FILE_PAGES).
Maybe subtract buffer ram.
It's not clear host cares about the distinction,
it's all memory that can shrink in response to
memory pressure such as inflating the balloon.
This statistic is portable as well I think, most guests have
storage cache.
> > > > ---
> > > > drivers/virtio/virtio_balloon.c | 11 +++++++++++
> > > > include/uapi/linux/virtio_balloon.h | 4 +++-
> > > > mm/swap_state.c | 1 +
> > > > 3 files changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > > index f0b3a0b9d42f..c2558ec47a62 100644
> > > > --- a/drivers/virtio/virtio_balloon.c
> > > > +++ b/drivers/virtio/virtio_balloon.c
> > > > @@ -244,12 +244,19 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> > > > struct sysinfo i;
> > > > unsigned int idx = 0;
> > > > long available;
> > > > + long cached;
> > > >
> > > > all_vm_events(events);
> > > > si_meminfo(&i);
> > > >
> > > > available = si_mem_available();
> > > >
> > > > + cached = global_node_page_state(NR_FILE_PAGES) -
> > > > + total_swapcache_pages() - i.bufferram;
> > > > + if (cached < 0)
> > > > + cached = 0;
> > > > +
> > > > +
> > > > #ifdef CONFIG_VM_EVENT_COUNTERS
> > > > update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> > > > pages_to_bytes(events[PSWPIN]));
> > > > @@ -264,6 +271,10 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> > > > pages_to_bytes(i.totalram));
> > > > update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
> > > > pages_to_bytes(available));
> > > > + update_stat(vb, idx++, VIRTIO_BALLOON_S_BUFFERS,
> > > > + pages_to_bytes(i.bufferram));
> > > > + update_stat(vb, idx++, VIRTIO_BALLOON_S_CACHED,
> > > > + pages_to_bytes(cached));
> > > >
> > > > return idx;
> > > > }
> > > > diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> > > > index 343d7ddefe04..d5dc8a56a497 100644
> > > > --- a/include/uapi/linux/virtio_balloon.h
> > > > +++ b/include/uapi/linux/virtio_balloon.h
> > > > @@ -52,7 +52,9 @@ struct virtio_balloon_config {
> > > > #define VIRTIO_BALLOON_S_MEMFREE 4 /* Total amount of free memory */
> > > > #define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
> > > > #define VIRTIO_BALLOON_S_AVAIL 6 /* Available memory as in /proc */
> > > > -#define VIRTIO_BALLOON_S_NR 7
> > > > +#define VIRTIO_BALLOON_S_BUFFERS 7 /* Buffers memory as in /proc */
> > > > +#define VIRTIO_BALLOON_S_CACHED 8 /* Cached memory as in /proc */
> > > > +#define VIRTIO_BALLOON_S_NR 9
> > > >
> > > > /*
> > > > * Memory statistics structure.
> > > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > > index 71ce2d1ccbf7..f3a4ff7d6c52 100644
> > > > --- a/mm/swap_state.c
> > > > +++ b/mm/swap_state.c
> > > > @@ -95,6 +95,7 @@ unsigned long total_swapcache_pages(void)
> > > > rcu_read_unlock();
> > > > return ret;
> > > > }
> > > > +EXPORT_SYMBOL_GPL(total_swapcache_pages);
> > > >
> > > > static atomic_t swapin_readahead_hits = ATOMIC_INIT(4);
> > >
> > > Need an ack from MM crowd on that.
> > >
> > > > --
> > > > 2.14.1
> >
> >
> > --
> > Tomáš Golembiovský <tgolembi@redhat.com>
>
>
> --
> Tomáš Golembiovský <tgolembi@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: conditionally enable tx polling
From: Michael S. Tsirkin @ 2017-10-31 16:36 UTC (permalink / raw)
To: Jason Wang
Cc: Matthew Rosato, kvm, netdev, linux-kernel, virtualization, Wei Xu
In-Reply-To: <1509445640-4085-1-git-send-email-jasowang@redhat.com>
On Tue, Oct 31, 2017 at 06:27:20PM +0800, Jason Wang wrote:
> We always poll tx for socket, this is sub optimal since:
>
> - we only want to be notified when sndbuf is available
> - this will slightly increase the waitqueue traversing time and more
> important, vhost could not benefit from commit
> commit 9e641bdcfa4e
> ("net-tun: restructure tun_do_read for better sleep/wakeup efficiency")
> even if we've stopped rx polling during handle_rx() since tx poll
> were still left in the waitqueue.
>
> Pktgen from a remote host to VM over mlx4 shows 5.5% improvements on
> rx PPS. (from 1.27Mpps to 1.34Mpps)
>
> Cc: Wei Xu <wexu@redhat.com>
> Cc: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
Now that vhost_poll_stop happens on data path
a lot, I'd say
if (poll->wqh)
there should be unlikely().
> drivers/vhost/net.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 68677d9..286c3e4 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -471,6 +471,7 @@ static void handle_tx(struct vhost_net *net)
> goto out;
>
> vhost_disable_notify(&net->dev, vq);
> + vhost_net_disable_vq(net, vq);
>
> hdr_size = nvq->vhost_hlen;
> zcopy = nvq->ubufs;
> @@ -556,6 +557,8 @@ static void handle_tx(struct vhost_net *net)
> % UIO_MAXIOV;
> }
> vhost_discard_vq_desc(vq, 1);
> + if (err == -EAGAIN)
> + vhost_net_enable_vq(net, vq);
> break;
> }
> if (err != len)
I would probably just enable it unconditionally here. Why not?
> @@ -1145,9 +1148,11 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> r = vhost_vq_init_access(vq);
> if (r)
> goto err_used;
> - r = vhost_net_enable_vq(n, vq);
> - if (r)
> - goto err_used;
> + if (index == VHOST_NET_VQ_RX) {
> + r = vhost_net_enable_vq(n, vq);
> + if (r)
> + goto err_used;
> + }
>
> oldubufs = nvq->ubufs;
> nvq->ubufs = ubufs;
This last chunk seems questionable. If queue has stuff in it
when we connect the backend, we'll miss a wakeup.
I suspect this can happen during migration.
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: conditionally enable tx polling
From: Jason Wang @ 2017-11-01 12:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Matthew Rosato, kvm, netdev, linux-kernel, virtualization, Wei Xu
In-Reply-To: <20171031182637-mutt-send-email-mst@kernel.org>
On 2017年11月01日 00:36, Michael S. Tsirkin wrote:
> On Tue, Oct 31, 2017 at 06:27:20PM +0800, Jason Wang wrote:
>> We always poll tx for socket, this is sub optimal since:
>>
>> - we only want to be notified when sndbuf is available
>> - this will slightly increase the waitqueue traversing time and more
>> important, vhost could not benefit from commit
>> commit 9e641bdcfa4e
>> ("net-tun: restructure tun_do_read for better sleep/wakeup efficiency")
>> even if we've stopped rx polling during handle_rx() since tx poll
>> were still left in the waitqueue.
>>
>> Pktgen from a remote host to VM over mlx4 shows 5.5% improvements on
>> rx PPS. (from 1.27Mpps to 1.34Mpps)
>>
>> Cc: Wei Xu <wexu@redhat.com>
>> Cc: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
> Now that vhost_poll_stop happens on data path
> a lot, I'd say
> if (poll->wqh)
> there should be unlikely().
It has been there since 8241a1e466cd ("vhost_net: stop polling socket
during rx processing"). So it will be used for rx path too which
unlikely() does not work as well as the case in tx.
>
>
>> drivers/vhost/net.c | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 68677d9..286c3e4 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -471,6 +471,7 @@ static void handle_tx(struct vhost_net *net)
>> goto out;
>>
>> vhost_disable_notify(&net->dev, vq);
>> + vhost_net_disable_vq(net, vq);
>>
>> hdr_size = nvq->vhost_hlen;
>> zcopy = nvq->ubufs;
>> @@ -556,6 +557,8 @@ static void handle_tx(struct vhost_net *net)
>> % UIO_MAXIOV;
>> }
>> vhost_discard_vq_desc(vq, 1);
>> + if (err == -EAGAIN)
>> + vhost_net_enable_vq(net, vq);
>> break;
>> }
>> if (err != len)
> I would probably just enable it unconditionally here. Why not?
>
I thought we only care about the case of tun_sock_write_space() and for
the errors other than -EAGAIN, they have nothing to do with polling.
>> @@ -1145,9 +1148,11 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>> r = vhost_vq_init_access(vq);
>> if (r)
>> goto err_used;
>> - r = vhost_net_enable_vq(n, vq);
>> - if (r)
>> - goto err_used;
>> + if (index == VHOST_NET_VQ_RX) {
>> + r = vhost_net_enable_vq(n, vq);
>> + if (r)
>> + goto err_used;
>> + }
>>
>> oldubufs = nvq->ubufs;
>> nvq->ubufs = ubufs;
> This last chunk seems questionable. If queue has stuff in it
> when we connect the backend, we'll miss a wakeup.
> I suspect this can happen during migration.
Unless qemu pass a tap which s already had pending tx packets.
I can remove this chuck, but if guest does not transmit any packet, rx
can't benefit from this.
Thanks
>
>
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: Virtio BoF minutes from KVM Forum 2017
From: Michael S. Tsirkin @ 2017-11-01 14:59 UTC (permalink / raw)
To: Jens Freimann; +Cc: virtio-dev, virtio-comment, virtualization
In-Reply-To: <20171029125225.lx4ezpkbvjjsjv3x@localhost.localdomain>
On Sun, Oct 29, 2017 at 01:52:25PM +0100, Jens Freimann wrote:
> Ilya: - you might have more completions than descriptors available
> - partial descriptor chains are a problem for hardware because you might have
> to read a bunch of conscriptors twice - how would you do deal with a big
> buffer that cointains a large number of
> small packets with respect to completions?
> - is one bit for completion enough? right now it means descriptor was actually
> used. how to we signal when it was completed?
I am not sure I understand the difference. Under virtio, driver makes a
descriptor available, then device reads/writes memory depending on
descriptor type, then marks it as used.
What does completed mean?
> - concerned about not being able to do scatter/gatter with the ring layout.
> Network drivers heavily using indirect buffers. - for a hardware
> implementation a completion ring is a very convenient form for
> some use cases, so we want an efficient implementation for them. If we had an
> inline descriptor then a completion ring is just a normal ring and we won't
> need another ring type.
> - doesn't like the fact that we need to do a linear scan to find the length of
> a descriptor chain. It would be nice if we could have the length of the chain
> in the first descriptor (i.e. the number of chained descriptors, not the number
> of posted descriptors which can be deduced from the id field)
Not responding to rest of points since I don't understand the basic
assumption above yet.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: conditionally enable tx polling
From: Michael S. Tsirkin @ 2017-11-01 15:03 UTC (permalink / raw)
To: Jason Wang
Cc: Matthew Rosato, kvm, netdev, linux-kernel, virtualization, Wei Xu
In-Reply-To: <c69accc4-4a35-0501-8a99-ab7821365cd8@redhat.com>
On Wed, Nov 01, 2017 at 08:51:36PM +0800, Jason Wang wrote:
>
>
> On 2017年11月01日 00:36, Michael S. Tsirkin wrote:
> > On Tue, Oct 31, 2017 at 06:27:20PM +0800, Jason Wang wrote:
> > > We always poll tx for socket, this is sub optimal since:
> > >
> > > - we only want to be notified when sndbuf is available
> > > - this will slightly increase the waitqueue traversing time and more
> > > important, vhost could not benefit from commit
> > > commit 9e641bdcfa4e
> > > ("net-tun: restructure tun_do_read for better sleep/wakeup efficiency")
> > > even if we've stopped rx polling during handle_rx() since tx poll
> > > were still left in the waitqueue.
> > >
> > > Pktgen from a remote host to VM over mlx4 shows 5.5% improvements on
> > > rx PPS. (from 1.27Mpps to 1.34Mpps)
> > >
> > > Cc: Wei Xu <wexu@redhat.com>
> > > Cc: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > Now that vhost_poll_stop happens on data path
> > a lot, I'd say
> > if (poll->wqh)
> > there should be unlikely().
>
> It has been there since 8241a1e466cd ("vhost_net: stop polling socket during
> rx processing"). So it will be used for rx path too which unlikely() does
> not work as well as the case in tx.
Worth testing, does not have to block this patch.
>
> >
> >
> > > drivers/vhost/net.c | 11 ++++++++---
> > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 68677d9..286c3e4 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -471,6 +471,7 @@ static void handle_tx(struct vhost_net *net)
> > > goto out;
> > > vhost_disable_notify(&net->dev, vq);
> > > + vhost_net_disable_vq(net, vq);
> > > hdr_size = nvq->vhost_hlen;
> > > zcopy = nvq->ubufs;
> > > @@ -556,6 +557,8 @@ static void handle_tx(struct vhost_net *net)
> > > % UIO_MAXIOV;
> > > }
> > > vhost_discard_vq_desc(vq, 1);
> > > + if (err == -EAGAIN)
> > > + vhost_net_enable_vq(net, vq);
> > > break;
> > > }
> > > if (err != len)
> > I would probably just enable it unconditionally here. Why not?
> >
>
> I thought we only care about the case of tun_sock_write_space() and for the
> errors other than -EAGAIN, they have nothing to do with polling.
We could thinkably get ENOMEM I guess. If we miss a code things
get stuck - It's just easier not to add extra code IMHO.
> > > @@ -1145,9 +1148,11 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> > > r = vhost_vq_init_access(vq);
> > > if (r)
> > > goto err_used;
> > > - r = vhost_net_enable_vq(n, vq);
> > > - if (r)
> > > - goto err_used;
> > > + if (index == VHOST_NET_VQ_RX) {
> > > + r = vhost_net_enable_vq(n, vq);
> > > + if (r)
> > > + goto err_used;
> > > + }
> > > oldubufs = nvq->ubufs;
> > > nvq->ubufs = ubufs;
> > This last chunk seems questionable. If queue has stuff in it
> > when we connect the backend, we'll miss a wakeup.
> > I suspect this can happen during migration.
>
> Unless qemu pass a tap which s already had pending tx packets.
>
> I can remove this chuck, but if guest does not transmit any packet, rx can't
> benefit from this.
>
> Thanks
Not sure I understand the last sentence. vhost will stay
polling the socket - why is that a problem?
> >
> >
> > > --
> > > 2.7.4
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] x86/paravirt: Add kernel parameter to choose paravirt lock type
From: Waiman Long @ 2017-11-01 15:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jonathan Corbet
Cc: Juergen Gross, Rusty Russell, kvm, Radim Krčmář,
Peter Zijlstra, x86, linux-kernel, virtualization, Waiman Long,
Paolo Bonzini, xen-devel, Alok Kataria, Boris Ostrovsky
Currently, there are 3 different lock types that can be chosen for
the x86 architecture:
- qspinlock
- pvqspinlock
- unfair lock
One of the above lock types will be chosen at boot time depending on
a number of different factors.
Ideally, the hypervisors should be able to pick the best performing
lock type for the current VM configuration. That is not currently
the case as the performance of each lock type are affected by many
different factors like the number of vCPUs in the VM, the amount vCPU
overcommitment, the CPU type and so on.
Generally speaking, unfair lock performs well for VMs with a small
number of vCPUs. Native qspinlock may perform better than pvqspinlock
if there is vCPU pinning and there is no vCPU over-commitment.
This patch adds a new kernel parameter to allow administrator to
choose the paravirt spinlock type to be used. VM administrators can
experiment with the different lock types and choose one that can best
suit their need, if they want to. Hypervisor developers can also use
that to experiment with different lock types so that they can come
up with a better algorithm to pick the best lock type.
The hypervisor paravirt spinlock code will override this new parameter
in determining if pvqspinlock should be used. The parameter, however,
will override Xen's xen_nopvspin in term of disabling unfair lock.
Signed-off-by: Waiman Long <longman@redhat.com>
---
Documentation/admin-guide/kernel-parameters.txt | 7 +++++
arch/x86/include/asm/paravirt.h | 9 ++++++
arch/x86/kernel/kvm.c | 4 +++
arch/x86/kernel/paravirt.c | 40 ++++++++++++++++++++++++-
arch/x86/xen/spinlock.c | 6 ++--
5 files changed, 62 insertions(+), 4 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index f7df49d..c98d9c7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3275,6 +3275,13 @@
[KNL] Number of legacy pty's. Overwrites compiled-in
default number.
+ pvlock_type= [X86,PV_OPS]
+ Specify the paravirt spinlock type to be used.
+ Options are:
+ queued - native queued spinlock
+ pv - paravirt queued spinlock
+ unfair - simple TATAS unfair lock
+
quiet [KNL] Disable most log messages
r128= [HW,DRM]
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 12deec7..941a046 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -690,6 +690,15 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
#endif /* SMP && PARAVIRT_SPINLOCKS */
+enum pv_spinlock_type {
+ locktype_auto,
+ locktype_queued,
+ locktype_paravirt,
+ locktype_unfair,
+};
+
+extern enum pv_spinlock_type pv_spinlock_type;
+
#ifdef CONFIG_X86_32
#define PV_SAVE_REGS "pushl %ecx; pushl %edx;"
#define PV_RESTORE_REGS "popl %edx; popl %ecx;"
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 8bb9594..3a5d3ec4 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -646,6 +646,10 @@ void __init kvm_spinlock_init(void)
if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
return;
+ if ((pv_spinlock_type == locktype_queued) ||
+ (pv_spinlock_type == locktype_unfair))
+ return;
+
__pv_init_lock_hash();
pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 041096b..ca35cd3 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -115,11 +115,48 @@ unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
return 5;
}
+/*
+ * The kernel argument "pvlock_type=" can be used to explicitly specify
+ * which type of spinlocks to be used. Currently, there are 3 options:
+ * 1) queued - the native queued spinlock
+ * 2) pv - the paravirt queued spinlock (if CONFIG_PARAVIRT_SPINLOCKS)
+ * 3) unfair - the simple TATAS unfair lock
+ *
+ * If this argument is not specified, the kernel will automatically choose
+ * an appropriate one depending on X86_FEATURE_HYPERVISOR and hypervisor
+ * specific settings.
+ */
+enum pv_spinlock_type __read_mostly pv_spinlock_type = locktype_auto;
+
+static int __init pvlock_setup(char *s)
+{
+ if (!s)
+ return -EINVAL;
+
+ if (!strcmp(s, "queued"))
+ pv_spinlock_type = locktype_queued;
+ else if (!strcmp(s, "pv"))
+ pv_spinlock_type = locktype_paravirt;
+ else if (!strcmp(s, "unfair"))
+ pv_spinlock_type = locktype_unfair;
+ else
+ return -EINVAL;
+
+ pr_info("PV lock type = %s (%d)\n", s, pv_spinlock_type);
+ return 0;
+}
+
+early_param("pvlock_type", pvlock_setup);
+
DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
void __init native_pv_lock_init(void)
{
- if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ if (pv_spinlock_type == locktype_unfair)
+ return;
+
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
+ (pv_spinlock_type != locktype_auto))
static_branch_disable(&virt_spin_lock_key);
}
@@ -473,3 +510,4 @@ struct pv_mmu_ops pv_mmu_ops __ro_after_init = {
EXPORT_SYMBOL (pv_mmu_ops);
EXPORT_SYMBOL_GPL(pv_info);
EXPORT_SYMBOL (pv_irq_ops);
+EXPORT_SYMBOL (pv_spinlock_type);
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 1e1462d..9fc8eab 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -82,7 +82,7 @@ void xen_init_lock_cpu(int cpu)
char *name;
if (!xen_pvspin) {
- if (cpu == 0)
+ if ((cpu == 0) && (pv_spinlock_type == locktype_auto))
static_branch_disable(&virt_spin_lock_key);
return;
}
@@ -130,8 +130,8 @@ void xen_uninit_lock_cpu(int cpu)
*/
void __init xen_init_spinlocks(void)
{
-
- if (!xen_pvspin) {
+ if (!xen_pvspin || (pv_spinlock_type == locktype_queued) ||
+ (pv_spinlock_type == locktype_unfair)) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
return;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] x86/paravirt: Add kernel parameter to choose paravirt lock type
From: Juergen Gross @ 2017-11-01 15:51 UTC (permalink / raw)
To: Waiman Long, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Jonathan Corbet
Cc: Rusty Russell, kvm, Radim Krčmář, Peter Zijlstra,
x86, linux-kernel, virtualization, Paolo Bonzini, xen-devel,
Alok Kataria, Boris Ostrovsky
In-Reply-To: <1509550367-19255-1-git-send-email-longman@redhat.com>
On 01/11/17 16:32, Waiman Long wrote:
> Currently, there are 3 different lock types that can be chosen for
> the x86 architecture:
>
> - qspinlock
> - pvqspinlock
> - unfair lock
>
> One of the above lock types will be chosen at boot time depending on
> a number of different factors.
>
> Ideally, the hypervisors should be able to pick the best performing
> lock type for the current VM configuration. That is not currently
> the case as the performance of each lock type are affected by many
> different factors like the number of vCPUs in the VM, the amount vCPU
> overcommitment, the CPU type and so on.
>
> Generally speaking, unfair lock performs well for VMs with a small
> number of vCPUs. Native qspinlock may perform better than pvqspinlock
> if there is vCPU pinning and there is no vCPU over-commitment.
>
> This patch adds a new kernel parameter to allow administrator to
> choose the paravirt spinlock type to be used. VM administrators can
> experiment with the different lock types and choose one that can best
> suit their need, if they want to. Hypervisor developers can also use
> that to experiment with different lock types so that they can come
> up with a better algorithm to pick the best lock type.
>
> The hypervisor paravirt spinlock code will override this new parameter
> in determining if pvqspinlock should be used. The parameter, however,
> will override Xen's xen_nopvspin in term of disabling unfair lock.
Hmm, I'm not sure we need pvlock_type _and_ xen_nopvspin. What do others
think?
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 7 +++++
> arch/x86/include/asm/paravirt.h | 9 ++++++
> arch/x86/kernel/kvm.c | 4 +++
> arch/x86/kernel/paravirt.c | 40 ++++++++++++++++++++++++-
> arch/x86/xen/spinlock.c | 6 ++--
> 5 files changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f7df49d..c98d9c7 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3275,6 +3275,13 @@
> [KNL] Number of legacy pty's. Overwrites compiled-in
> default number.
>
> + pvlock_type= [X86,PV_OPS]
> + Specify the paravirt spinlock type to be used.
> + Options are:
> + queued - native queued spinlock
> + pv - paravirt queued spinlock
> + unfair - simple TATAS unfair lock
> +
> quiet [KNL] Disable most log messages
>
> r128= [HW,DRM]
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index 12deec7..941a046 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -690,6 +690,15 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
>
> #endif /* SMP && PARAVIRT_SPINLOCKS */
>
> +enum pv_spinlock_type {
> + locktype_auto,
> + locktype_queued,
> + locktype_paravirt,
> + locktype_unfair,
> +};
> +
> +extern enum pv_spinlock_type pv_spinlock_type;
> +
> #ifdef CONFIG_X86_32
> #define PV_SAVE_REGS "pushl %ecx; pushl %edx;"
> #define PV_RESTORE_REGS "popl %edx; popl %ecx;"
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8bb9594..3a5d3ec4 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -646,6 +646,10 @@ void __init kvm_spinlock_init(void)
> if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
> return;
>
> + if ((pv_spinlock_type == locktype_queued) ||
> + (pv_spinlock_type == locktype_unfair))
> + return;
> +
> __pv_init_lock_hash();
> pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
> pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index 041096b..ca35cd3 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -115,11 +115,48 @@ unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
> return 5;
> }
>
> +/*
> + * The kernel argument "pvlock_type=" can be used to explicitly specify
> + * which type of spinlocks to be used. Currently, there are 3 options:
> + * 1) queued - the native queued spinlock
> + * 2) pv - the paravirt queued spinlock (if CONFIG_PARAVIRT_SPINLOCKS)
> + * 3) unfair - the simple TATAS unfair lock
> + *
> + * If this argument is not specified, the kernel will automatically choose
> + * an appropriate one depending on X86_FEATURE_HYPERVISOR and hypervisor
> + * specific settings.
> + */
> +enum pv_spinlock_type __read_mostly pv_spinlock_type = locktype_auto;
> +
> +static int __init pvlock_setup(char *s)
> +{
> + if (!s)
> + return -EINVAL;
> +
> + if (!strcmp(s, "queued"))
> + pv_spinlock_type = locktype_queued;
> + else if (!strcmp(s, "pv"))
> + pv_spinlock_type = locktype_paravirt;
> + else if (!strcmp(s, "unfair"))
> + pv_spinlock_type = locktype_unfair;
> + else
> + return -EINVAL;
> +
> + pr_info("PV lock type = %s (%d)\n", s, pv_spinlock_type);
> + return 0;
> +}
> +
> +early_param("pvlock_type", pvlock_setup);
> +
> DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
>
> void __init native_pv_lock_init(void)
> {
> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> + if (pv_spinlock_type == locktype_unfair)
> + return;
> +
> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
> + (pv_spinlock_type != locktype_auto))
> static_branch_disable(&virt_spin_lock_key);
Really? I don't think locktype_paravirt should disable the static key.
Juergen
^ permalink raw reply
* RE: Virtio BoF minutes from KVM Forum 2017
From: Ilya Lesokhin @ 2017-11-01 15:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org,
virtio-comment@lists.oasis-open.org
In-Reply-To: <20171101162357-mutt-send-email-mst@kernel.org>
On Wednesday, November 01, 2017 4:59 PM, Michael S. Tsirkin wrote:
> On Sun, Oct 29, 2017 at 01:52:25PM +0100, Jens Freimann wrote:
> > Ilya: - you might have more completions than descriptors available
> > - partial descriptor chains are a problem for hardware because you
> > might have to read a bunch of conscriptors twice - how would you do
> > deal with a big buffer that cointains a large number of small packets
> > with respect to completions?
> > - is one bit for completion enough? right now it means descriptor was
> > actually used. how to we signal when it was completed?
>
> I am not sure I understand the difference. Under virtio, driver makes a
> descriptor available, then device reads/writes memory depending on descriptor
> type, then marks it as used.
>
> What does completed mean?
>
During the BOF, someone raised the point that there is no indication that the HW has
Read the descriptor. I think after some discussion we've agreed that it's not a useful indication.
My issues with the current completion or used notifications are as follows:
1. There is no room for extra metadata such as checksum or flow tag.
You could put that in the descriptor payload but it's somewhat inconvenient.
You have to either use and additional descriptor for metadata per chain.
Or putting it in one of the buffers and forcing the lifetime of the metadata and data to be the same.
2. Current format assumes 1-1 corresponds between descriptors and completions.
You did offer a skipping optimization for many descriptors -> 1 completion.
But it is somewhat inefficient.
And you didn't offer a solution for 1 descriptor -> multiple completions.
Mellanox has a feature called striding RQ where you post a large buffer and
The NIC fills it with multiple back to back packets with padding.
Each packet generates its own completion.
3. There is a usage model where you have multiple produce rings
And a single completion ring.
You could implement the completion ring using an additional virtio ring, but
The current model will require an extra indirection as it force you to write into
The buffers the descriptor in the completion ring point to. Rather than writing the
Completion into the ring itself.
Additionally the device is still required to write to the original producer ring
in addition to the completion ring.
I think the best and most flexible design is to have variable size descriptor that
start with a dword header.
The dword header will include - an ownership bit, an opcode and descriptor length.
The opcode and the "length" dwords following the header will be device specific.
The owner bit meaning changes on each ring wrap around so the device doesn't
Need to update.
Each device (or device class) can choose whether completions are reported directly inside
the descriptors in that ring or in a separate completion ring.
completions rings can be implemented in an efficient manner with this design.
The driver will initialize a dedicated completion ring with empty completion sized descriptors.
And the device will write the completions directly into the ring.
^ permalink raw reply
* Re: [PATCH] x86/paravirt: Add kernel parameter to choose paravirt lock type
From: Waiman Long @ 2017-11-01 16:28 UTC (permalink / raw)
To: Juergen Gross, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Jonathan Corbet
Cc: Rusty Russell, kvm, Radim Krčmář, Peter Zijlstra,
x86, linux-kernel, virtualization, Paolo Bonzini, xen-devel,
Alok Kataria, Boris Ostrovsky
In-Reply-To: <404a9fab-03ff-174e-4ece-79932a6f302c@suse.com>
On 11/01/2017 11:51 AM, Juergen Gross wrote:
> On 01/11/17 16:32, Waiman Long wrote:
>> Currently, there are 3 different lock types that can be chosen for
>> the x86 architecture:
>>
>> - qspinlock
>> - pvqspinlock
>> - unfair lock
>>
>> One of the above lock types will be chosen at boot time depending on
>> a number of different factors.
>>
>> Ideally, the hypervisors should be able to pick the best performing
>> lock type for the current VM configuration. That is not currently
>> the case as the performance of each lock type are affected by many
>> different factors like the number of vCPUs in the VM, the amount vCPU
>> overcommitment, the CPU type and so on.
>>
>> Generally speaking, unfair lock performs well for VMs with a small
>> number of vCPUs. Native qspinlock may perform better than pvqspinlock
>> if there is vCPU pinning and there is no vCPU over-commitment.
>>
>> This patch adds a new kernel parameter to allow administrator to
>> choose the paravirt spinlock type to be used. VM administrators can
>> experiment with the different lock types and choose one that can best
>> suit their need, if they want to. Hypervisor developers can also use
>> that to experiment with different lock types so that they can come
>> up with a better algorithm to pick the best lock type.
>>
>> The hypervisor paravirt spinlock code will override this new parameter
>> in determining if pvqspinlock should be used. The parameter, however,
>> will override Xen's xen_nopvspin in term of disabling unfair lock.
> Hmm, I'm not sure we need pvlock_type _and_ xen_nopvspin. What do others
> think?
I don't think we need xen_nopvspin, but I don't want to remove that
without agreement from the community.
>> DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
>>
>> void __init native_pv_lock_init(void)
>> {
>> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>> + if (pv_spinlock_type == locktype_unfair)
>> + return;
>> +
>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
>> + (pv_spinlock_type != locktype_auto))
>> static_branch_disable(&virt_spin_lock_key);
> Really? I don't think locktype_paravirt should disable the static key.
With paravirt spinlock, it doesn't matter if the static key is disabled
or not. Without CONFIG_PARAVIRT_SPINLOCKS, however, it does degenerate
into the native qspinlock. So you are right, I should check for paravirt
type as well.
Cheers,
Longman
^ permalink raw reply
* Re: Virtio BoF minutes from KVM Forum 2017
From: Michael S. Tsirkin @ 2017-11-01 17:35 UTC (permalink / raw)
To: Ilya Lesokhin
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org,
virtio-comment@lists.oasis-open.org
In-Reply-To: <AM4PR0501MB2723D56ED113A7B98D3785F0D45F0@AM4PR0501MB2723.eurprd05.prod.outlook.com>
On Wed, Nov 01, 2017 at 03:52:12PM +0000, Ilya Lesokhin wrote:
> On Wednesday, November 01, 2017 4:59 PM, Michael S. Tsirkin wrote:
>
> > On Sun, Oct 29, 2017 at 01:52:25PM +0100, Jens Freimann wrote:
> > > Ilya: - you might have more completions than descriptors available
> > > - partial descriptor chains are a problem for hardware because you
> > > might have to read a bunch of conscriptors twice - how would you do
> > > deal with a big buffer that cointains a large number of small packets
> > > with respect to completions?
> > > - is one bit for completion enough? right now it means descriptor was
> > > actually used. how to we signal when it was completed?
> >
> > I am not sure I understand the difference. Under virtio, driver makes a
> > descriptor available, then device reads/writes memory depending on descriptor
> > type, then marks it as used.
> >
> > What does completed mean?
> >
>
> During the BOF, someone raised the point that there is no indication that the HW has
> Read the descriptor. I think after some discussion we've agreed that it's not a useful indication.
>
> My issues with the current completion or used notifications are as follows:
> 1. There is no room for extra metadata such as checksum or flow tag.
> You could put that in the descriptor payload but it's somewhat inconvenient.
> You have to either use and additional descriptor for metadata per chain.
> Or putting it in one of the buffers and forcing the lifetime of the metadata and data to be the same.
That's true. It would be easy to make descriptors e.g. 32 bytes each, so
you can add extra data in there. Or if we can live with wasting some
bytes per descriptor, we could add a descriptor flag that marks the
address field as meta-data. You could then chain it with a regular
descriptor for data. However all in all the simplest option is probably
in the virtio header which can be linear with the packet.
> 2. Current format assumes 1-1 corresponds between descriptors and completions.
> You did offer a skipping optimization for many descriptors -> 1 completion.
Yes - that's because the claim was that if using descriptors in-order,
device is doing many unnecessary writes.
> But it is somewhat inefficient.
It seems to outperform other options on a micro-benchmark but sure,
what do you propose?
> And you didn't offer a solution for 1 descriptor -> multiple completions.
> Mellanox has a feature called striding RQ where you post a large buffer and
> The NIC fills it with multiple back to back packets with padding.
> Each packet generates its own completion.
I suspect a good way to do this would be to just pass offsets within the
buffer back and forth. I agree sticking such small messages in a
separate buffer is not ideal. How about an option of replacing PA
with this data?
> 3. There is a usage model where you have multiple produce rings
> And a single completion ring.
What good is it though? It seems to perform worse than combining
producer and consumer in my testing.
> You could implement the completion ring using an additional virtio ring, but
> The current model will require an extra indirection as it force you to write into
> The buffers the descriptor in the completion ring point to. Rather than writing the
> Completion into the ring itself.
> Additionally the device is still required to write to the original producer ring
> in addition to the completion ring.
>
> I think the best and most flexible design is to have variable size descriptor that
> start with a dword header.
> The dword header will include - an ownership bit, an opcode and descriptor length.
> The opcode and the "length" dwords following the header will be device specific.
This means that device needs to do two reads just to decode the
descriptor fully. This conflicts with feedback Intel has been giving on
list which is to try and reduce number of reads. With header linear with
the packet, you need two reads to start transmitting the packet.
> The owner bit meaning changes on each ring wrap around so the device doesn't
> Need to update.
Seems to look like the avail bit in the kvm forum presentation.
> Each device (or device class) can choose whether completions are reported directly inside
> the descriptors in that ring or in a separate completion ring.
>
> completions rings can be implemented in an efficient manner with this design.
> The driver will initialize a dedicated completion ring with empty completion sized descriptors.
> And the device will write the completions directly into the ring.
I assume when you say completion you mean used entries, if I'm wrong
please correct me. In fact with the proposal in the kvm forum
presentation it is possible to write used entries in a separate address
as opposed to overlapping the available entries. If you are going to
support skipping writing back some used descriptors then accounting
would have to change slightly since driver won't be able to reset used
flags then. But in the past in all tests I've written this separate
ring underperforms a shared ring.
>
>
>
>
>
>
>
>
>
^ permalink raw reply
* RE: Virtio BoF minutes from KVM Forum 2017
From: Ilya Lesokhin @ 2017-11-01 18:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <20171101190219-mutt-send-email-mst@kernel.org>
On Wednesday, November 1, 2017 7:35 PM, Michael S. Tsirkin wrote:
> > You have to either use and additional descriptor for metadata per chain.
> > Or putting it in one of the buffers and forcing the lifetime of the metadata
> and data to be the same.
>
> That's true. It would be easy to make descriptors e.g. 32 bytes each, so
> you can add extra data in there. Or if we can live with wasting some
> bytes per descriptor, we could add a descriptor flag that marks the
> address field as meta-data. You could then chain it with a regular
> descriptor for data. However all in all the simplest option is probably
> in the virtio header which can be linear with the packet.
[I.L] In the current proposal descriptor size == SGE (scatter gather entry) size.
I'm not sure that's a good idea.
For example we are considering having an RX ring were you just post a list
of PFN's so a sge is only 8 bytes.
I might be wrong here, so please correct me if that's the not the case.
but I've gotten the impression that due to DPDK limitations you've focused
on the use case where you have 1 SGE.
I'm not sure that a representative workload for network devices,
As LSO is an important offload.
And the storage guys also complained about this issue.
> I suspect a good way to do this would be to just pass offsets within the
> buffer back and forth. I agree sticking such small messages in a
> separate buffer is not ideal. How about an option of replacing PA
> with this data?
[I.L] PA?
>
>
>
> > 3. There is a usage model where you have multiple produce rings
> > And a single completion ring.
>
> What good is it though? It seems to perform worse than combining
> producer and consumer in my testing.
>
[I.L] It might be that for virtio-net a single ring is better
But are you really going to argue that its better in all possible use cases?
>
> > You could implement the completion ring using an additional virtio ring,
> but
> > The current model will require an extra indirection as it force you to write
> into
> > The buffers the descriptor in the completion ring point to. Rather than
> writing the
> > Completion into the ring itself.
> > Additionally the device is still required to write to the original producer ring
> > in addition to the completion ring.
> >
> > I think the best and most flexible design is to have variable size descriptor
> that
> > start with a dword header.
> > The dword header will include - an ownership bit, an opcode and
> descriptor length.
> > The opcode and the "length" dwords following the header will be device
> specific.
>
> This means that device needs to do two reads just to decode the
> descriptor fully. This conflicts with feedback Intel has been giving on
> list which is to try and reduce number of reads. With header linear with
> the packet, you need two reads to start transmitting the packet.
>
[I.L] The device can do a single large read and do the parsing afterword's.
You could also use the doorbell to tell the device how much to read.
> Seems to look like the avail bit in the kvm forum presentation.
>
[I.L] I don't want to argue over the name. The main difference in my
proposal is that the device doesn't need to write to the descriptor.
If it wants to you can define a separate bit for that.
> > Each device (or device class) can choose whether completions are reported
> directly inside
> > the descriptors in that ring or in a separate completion ring.
> >
> > completions rings can be implemented in an efficient manner with this
> design.
> > The driver will initialize a dedicated completion ring with empty completion
> sized descriptors.
> > And the device will write the completions directly into the ring.
>
> I assume when you say completion you mean used entries, if I'm wrong
> please correct me. In fact with the proposal in the kvm forum
> presentation it is possible to write used entries in a separate address
> as opposed to overlapping the available entries. If you are going to
> support skipping writing back some used descriptors then accounting
> would have to change slightly since driver won't be able to reset used
> flags then. But in the past in all tests I've written this separate
> ring underperforms a shared ring.
>
[I.L] A completion is a used entry + device specific metadata.
I don't remember seeing an option to write used entries in a separate address,
I'll appreciate it if you can point me to the right direction.
Regarding the shared ring vs separate ring, I can't really argue with you
as I haven't done the relevant measurement.
I'm just saying it might not always be optimal in all use case,
So you should consider leaving both options open.
Its entirely possible that for virtio-net you want a single ring,
Where for PV-RDMA you want separate rings.
^ permalink raw reply
* Re: [PATCH] x86/paravirt: Add kernel parameter to choose paravirt lock type
From: Boris Ostrovsky @ 2017-11-01 19:01 UTC (permalink / raw)
To: Waiman Long, Juergen Gross, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Jonathan Corbet
Cc: Rusty Russell, kvm, Radim Krčmář, Peter Zijlstra,
x86, linux-kernel, virtualization, Paolo Bonzini, xen-devel,
Alok Kataria
In-Reply-To: <409ede92-5243-0d9b-7893-b596f3f353aa@redhat.com>
On 11/01/2017 12:28 PM, Waiman Long wrote:
> On 11/01/2017 11:51 AM, Juergen Gross wrote:
>> On 01/11/17 16:32, Waiman Long wrote:
>>> Currently, there are 3 different lock types that can be chosen for
>>> the x86 architecture:
>>>
>>> - qspinlock
>>> - pvqspinlock
>>> - unfair lock
>>>
>>> One of the above lock types will be chosen at boot time depending on
>>> a number of different factors.
>>>
>>> Ideally, the hypervisors should be able to pick the best performing
>>> lock type for the current VM configuration. That is not currently
>>> the case as the performance of each lock type are affected by many
>>> different factors like the number of vCPUs in the VM, the amount vCPU
>>> overcommitment, the CPU type and so on.
>>>
>>> Generally speaking, unfair lock performs well for VMs with a small
>>> number of vCPUs. Native qspinlock may perform better than pvqspinlock
>>> if there is vCPU pinning and there is no vCPU over-commitment.
>>>
>>> This patch adds a new kernel parameter to allow administrator to
>>> choose the paravirt spinlock type to be used. VM administrators can
>>> experiment with the different lock types and choose one that can best
>>> suit their need, if they want to. Hypervisor developers can also use
>>> that to experiment with different lock types so that they can come
>>> up with a better algorithm to pick the best lock type.
>>>
>>> The hypervisor paravirt spinlock code will override this new parameter
>>> in determining if pvqspinlock should be used. The parameter, however,
>>> will override Xen's xen_nopvspin in term of disabling unfair lock.
>> Hmm, I'm not sure we need pvlock_type _and_ xen_nopvspin. What do others
>> think?
> I don't think we need xen_nopvspin, but I don't want to remove that
> without agreement from the community.
I also don't think xen_nopvspin will be needed after pvlock_type is
introduced.
-boris
^ permalink raw reply
* Re: [PATCH] x86/paravirt: Add kernel parameter to choose paravirt lock type
From: Waiman Long @ 2017-11-01 19:42 UTC (permalink / raw)
To: Boris Ostrovsky, Juergen Gross, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Jonathan Corbet
Cc: Rusty Russell, kvm, Radim Krčmář, Peter Zijlstra,
x86, linux-kernel, virtualization, Paolo Bonzini, xen-devel,
Alok Kataria
In-Reply-To: <5539b95d-eac4-cc09-2cfc-1452d2751fa0@oracle.com>
On 11/01/2017 03:01 PM, Boris Ostrovsky wrote:
> On 11/01/2017 12:28 PM, Waiman Long wrote:
>> On 11/01/2017 11:51 AM, Juergen Gross wrote:
>>> On 01/11/17 16:32, Waiman Long wrote:
>>>> Currently, there are 3 different lock types that can be chosen for
>>>> the x86 architecture:
>>>>
>>>> - qspinlock
>>>> - pvqspinlock
>>>> - unfair lock
>>>>
>>>> One of the above lock types will be chosen at boot time depending on
>>>> a number of different factors.
>>>>
>>>> Ideally, the hypervisors should be able to pick the best performing
>>>> lock type for the current VM configuration. That is not currently
>>>> the case as the performance of each lock type are affected by many
>>>> different factors like the number of vCPUs in the VM, the amount vCPU
>>>> overcommitment, the CPU type and so on.
>>>>
>>>> Generally speaking, unfair lock performs well for VMs with a small
>>>> number of vCPUs. Native qspinlock may perform better than pvqspinlock
>>>> if there is vCPU pinning and there is no vCPU over-commitment.
>>>>
>>>> This patch adds a new kernel parameter to allow administrator to
>>>> choose the paravirt spinlock type to be used. VM administrators can
>>>> experiment with the different lock types and choose one that can best
>>>> suit their need, if they want to. Hypervisor developers can also use
>>>> that to experiment with different lock types so that they can come
>>>> up with a better algorithm to pick the best lock type.
>>>>
>>>> The hypervisor paravirt spinlock code will override this new parameter
>>>> in determining if pvqspinlock should be used. The parameter, however,
>>>> will override Xen's xen_nopvspin in term of disabling unfair lock.
>>> Hmm, I'm not sure we need pvlock_type _and_ xen_nopvspin. What do others
>>> think?
>> I don't think we need xen_nopvspin, but I don't want to remove that
>> without agreement from the community.
> I also don't think xen_nopvspin will be needed after pvlock_type is
> introduced.
>
> -boris
Another reason that I didn't try to remove xen_nopvspin is backward
compatibility concern. One way to handle it is to depreciate it and
treat it as an alias to pvlock_type=queued.
Cheers,
Longman
^ permalink raw reply
* [PATCH-tip v2 0/2] x86/paravirt: Enable users to choose PV lock type
From: Waiman Long @ 2017-11-01 20:58 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jonathan Corbet
Cc: Juergen Gross, Rusty Russell, kvm, Radim Krčmář,
Peter Zijlstra, x86, linux-kernel, virtualization, Waiman Long,
Paolo Bonzini, xen-devel, Alok Kataria, Boris Ostrovsky
v1->v2:
- Make pv_spinlock_type a bit mask for easier checking.
- Add patch 2 to deprecate xen_nopvspin
v1 - https://lkml.org/lkml/2017/11/1/381
Patch 1 adds a new pvlock_type parameter for the administrators to
specify the type of lock to be used in a para-virtualized kernel.
Patch 2 deprecates Xen's xen_nopvspin parameter as it is no longer
needed.
Waiman Long (2):
x86/paravirt: Add kernel parameter to choose paravirt lock type
x86/xen: Deprecate xen_nopvspin
Documentation/admin-guide/kernel-parameters.txt | 11 ++++---
arch/x86/include/asm/paravirt.h | 9 ++++++
arch/x86/kernel/kvm.c | 3 ++
arch/x86/kernel/paravirt.c | 40 ++++++++++++++++++++++++-
arch/x86/xen/spinlock.c | 17 +++++------
5 files changed, 65 insertions(+), 15 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH-tip v2 1/2] x86/paravirt: Add kernel parameter to choose paravirt lock type
From: Waiman Long @ 2017-11-01 20:58 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jonathan Corbet
Cc: Juergen Gross, Rusty Russell, kvm, Radim Krčmář,
Peter Zijlstra, x86, linux-kernel, virtualization, Waiman Long,
Paolo Bonzini, xen-devel, Alok Kataria, Boris Ostrovsky
In-Reply-To: <1509569934-4919-1-git-send-email-longman@redhat.com>
Currently, there are 3 different lock types that can be chosen for
the x86 architecture:
- qspinlock
- pvqspinlock
- unfair lock
One of the above lock types will be chosen at boot time depending on
a number of different factors.
Ideally, the hypervisors should be able to pick the best performing
lock type for the current VM configuration. That is not currently
the case as the performance of each lock type are affected by many
different factors like the number of vCPUs in the VM, the amount vCPU
overcommitment, the CPU type and so on.
Generally speaking, unfair lock performs well for VMs with a small
number of vCPUs. Native qspinlock may perform better than pvqspinlock
if there is vCPU pinning and there is no vCPU over-commitment.
This patch adds a new kernel parameter to allow administrator to
choose the paravirt spinlock type to be used. VM administrators can
experiment with the different lock types and choose one that can best
suit their need, if they want to. Hypervisor developers can also use
that to experiment with different lock types so that they can come
up with a better algorithm to pick the best lock type.
The hypervisor paravirt spinlock code will override this new parameter
in determining if pvqspinlock should be used. The parameter, however,
will override Xen's xen_nopvspin in term of disabling unfair lock.
Signed-off-by: Waiman Long <longman@redhat.com>
---
Documentation/admin-guide/kernel-parameters.txt | 7 +++++
arch/x86/include/asm/paravirt.h | 9 ++++++
arch/x86/kernel/kvm.c | 3 ++
arch/x86/kernel/paravirt.c | 40 ++++++++++++++++++++++++-
arch/x86/xen/spinlock.c | 12 ++++----
5 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index f7df49d..c98d9c7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3275,6 +3275,13 @@
[KNL] Number of legacy pty's. Overwrites compiled-in
default number.
+ pvlock_type= [X86,PV_OPS]
+ Specify the paravirt spinlock type to be used.
+ Options are:
+ queued - native queued spinlock
+ pv - paravirt queued spinlock
+ unfair - simple TATAS unfair lock
+
quiet [KNL] Disable most log messages
r128= [HW,DRM]
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 12deec7..c8f9ad9 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -690,6 +690,15 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
#endif /* SMP && PARAVIRT_SPINLOCKS */
+enum pv_spinlock_type {
+ locktype_auto = 0,
+ locktype_queued = 1,
+ locktype_paravirt = 2,
+ locktype_unfair = 4,
+};
+
+extern enum pv_spinlock_type pv_spinlock_type;
+
#ifdef CONFIG_X86_32
#define PV_SAVE_REGS "pushl %ecx; pushl %edx;"
#define PV_RESTORE_REGS "popl %edx; popl %ecx;"
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 8bb9594..3faee63 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -646,6 +646,9 @@ void __init kvm_spinlock_init(void)
if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
return;
+ if (pv_spinlock_type & (locktype_queued|locktype_unfair))
+ return;
+
__pv_init_lock_hash();
pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 041096b..aee6756 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -115,11 +115,48 @@ unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
return 5;
}
+/*
+ * The kernel argument "pvlock_type=" can be used to explicitly specify
+ * which type of spinlocks to be used. Currently, there are 3 options:
+ * 1) queued - the native queued spinlock
+ * 2) pv - the paravirt queued spinlock (if CONFIG_PARAVIRT_SPINLOCKS)
+ * 3) unfair - the simple TATAS unfair lock
+ *
+ * If this argument is not specified, the kernel will automatically choose
+ * an appropriate one depending on X86_FEATURE_HYPERVISOR and hypervisor
+ * specific settings.
+ */
+enum pv_spinlock_type __read_mostly pv_spinlock_type = locktype_auto;
+
+static int __init pvlock_setup(char *s)
+{
+ if (!s)
+ return -EINVAL;
+
+ if (!strcmp(s, "queued"))
+ pv_spinlock_type = locktype_queued;
+ else if (!strcmp(s, "pv"))
+ pv_spinlock_type = locktype_paravirt;
+ else if (!strcmp(s, "unfair"))
+ pv_spinlock_type = locktype_unfair;
+ else
+ return -EINVAL;
+
+ pr_info("PV lock type = %s (%d)\n", s, pv_spinlock_type);
+ return 0;
+}
+
+early_param("pvlock_type", pvlock_setup);
+
DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
void __init native_pv_lock_init(void)
{
- if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ if (pv_spinlock_type == locktype_unfair)
+ return;
+
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
+ (pv_spinlock_type == locktype_queued))
static_branch_disable(&virt_spin_lock_key);
}
@@ -473,3 +510,4 @@ struct pv_mmu_ops pv_mmu_ops __ro_after_init = {
EXPORT_SYMBOL (pv_mmu_ops);
EXPORT_SYMBOL_GPL(pv_info);
EXPORT_SYMBOL (pv_irq_ops);
+EXPORT_SYMBOL (pv_spinlock_type);
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 1e1462d..d5f79ac 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -81,8 +81,9 @@ void xen_init_lock_cpu(int cpu)
int irq;
char *name;
- if (!xen_pvspin) {
- if (cpu == 0)
+ if (!xen_pvspin ||
+ (pv_spinlock_type & (locktype_queued|locktype_unfair))) {
+ if ((cpu == 0) && !pv_spinlock_type)
static_branch_disable(&virt_spin_lock_key);
return;
}
@@ -109,7 +110,8 @@ void xen_init_lock_cpu(int cpu)
void xen_uninit_lock_cpu(int cpu)
{
- if (!xen_pvspin)
+ if (!xen_pvspin ||
+ (pv_spinlock_type & (locktype_queued|locktype_unfair)))
return;
unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
@@ -130,8 +132,8 @@ void xen_uninit_lock_cpu(int cpu)
*/
void __init xen_init_spinlocks(void)
{
-
- if (!xen_pvspin) {
+ if (!xen_pvspin ||
+ (pv_spinlock_type & (locktype_queued|locktype_unfair))) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
return;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip v2 2/2] x86/xen: Deprecate xen_nopvspin
From: Waiman Long @ 2017-11-01 20:58 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Jonathan Corbet
Cc: Juergen Gross, Rusty Russell, kvm, Radim Krčmář,
Peter Zijlstra, x86, linux-kernel, virtualization, Waiman Long,
Paolo Bonzini, xen-devel, Alok Kataria, Boris Ostrovsky
In-Reply-To: <1509569934-4919-1-git-send-email-longman@redhat.com>
With the new pvlock_type kernel parameter, xen_nopvspin is no longer
needed. This patch deprecates the xen_nopvspin parameter by removing
its documentation and treating it as an alias of "pvlock_type=queued".
Signed-off-by: Waiman Long <longman@redhat.com>
---
Documentation/admin-guide/kernel-parameters.txt | 4 ----
arch/x86/xen/spinlock.c | 19 +++++++------------
2 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index c98d9c7..683a817 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4596,10 +4596,6 @@
the unplug protocol
never -- do not unplug even if version check succeeds
- xen_nopvspin [X86,XEN]
- Disables the ticketlock slowpath using Xen PV
- optimizations.
-
xen_nopv [X86]
Disables the PV optimizations forcing the HVM guest to
run as generic HVM guest with no PV drivers.
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index d5f79ac..19e2e75 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -20,7 +20,6 @@
static DEFINE_PER_CPU(int, lock_kicker_irq) = -1;
static DEFINE_PER_CPU(char *, irq_name);
-static bool xen_pvspin = true;
#include <asm/qspinlock.h>
@@ -81,12 +80,8 @@ void xen_init_lock_cpu(int cpu)
int irq;
char *name;
- if (!xen_pvspin ||
- (pv_spinlock_type & (locktype_queued|locktype_unfair))) {
- if ((cpu == 0) && !pv_spinlock_type)
- static_branch_disable(&virt_spin_lock_key);
+ if (pv_spinlock_type & (locktype_queued|locktype_unfair))
return;
- }
WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
cpu, per_cpu(lock_kicker_irq, cpu));
@@ -110,8 +105,7 @@ void xen_init_lock_cpu(int cpu)
void xen_uninit_lock_cpu(int cpu)
{
- if (!xen_pvspin ||
- (pv_spinlock_type & (locktype_queued|locktype_unfair)))
+ if (pv_spinlock_type & (locktype_queued|locktype_unfair))
return;
unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
@@ -132,8 +126,7 @@ void xen_uninit_lock_cpu(int cpu)
*/
void __init xen_init_spinlocks(void)
{
- if (!xen_pvspin ||
- (pv_spinlock_type & (locktype_queued|locktype_unfair))) {
+ if (pv_spinlock_type & (locktype_queued|locktype_unfair)) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
return;
}
@@ -147,10 +140,12 @@ void __init xen_init_spinlocks(void)
pv_lock_ops.vcpu_is_preempted = PV_CALLEE_SAVE(xen_vcpu_stolen);
}
+/* TODO: To be removed in a future kernel version */
static __init int xen_parse_nopvspin(char *arg)
{
- xen_pvspin = false;
+ pr_warn("xen_nopvspin is deprecated, replace it with \"pvlock_type=queued\"!\n");
+ if (!pv_spinlock_type)
+ pv_spinlock_type = locktype_queued;
return 0;
}
early_param("xen_nopvspin", xen_parse_nopvspin);
-
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH-tip v2 2/2] x86/xen: Deprecate xen_nopvspin
From: Boris Ostrovsky @ 2017-11-01 22:01 UTC (permalink / raw)
To: Waiman Long, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Jonathan Corbet
Cc: Juergen Gross, Rusty Russell, kvm, Radim Krčmář,
Peter Zijlstra, x86, linux-kernel, virtualization, Paolo Bonzini,
xen-devel, Alok Kataria
In-Reply-To: <1509569934-4919-3-git-send-email-longman@redhat.com>
On 11/01/2017 04:58 PM, Waiman Long wrote:
> +/* TODO: To be removed in a future kernel version */
> static __init int xen_parse_nopvspin(char *arg)
> {
> - xen_pvspin = false;
> + pr_warn("xen_nopvspin is deprecated, replace it with \"pvlock_type=queued\"!\n");
> + if (!pv_spinlock_type)
> + pv_spinlock_type = locktype_queued;
Since we currently end up using unfair locks and because you are
deprecating xen_nopvspin I wonder whether it would be better to set this
to locktype_unfair so that current behavior doesn't change. (Sorry, I
haven't responded to your earlier message before you posted this). Juergen?
I am also not sure I agree with making pv_spinlock an enum *and* a
bitmask at the same time. I understand that it makes checks easier but I
think not assuming a value or a pattern would be better, especially
since none of the uses is on a critical path.
(For example, !pv_spinlock_type is the same as locktype_auto, which is
defined but never used)
-boris
> return 0;
> }
> early_param("xen_nopvspin", xen_parse_nopvspin);
> -
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: conditionally enable tx polling
From: Jason Wang @ 2017-11-02 3:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Matthew Rosato, kvm, netdev, linux-kernel, virtualization, Wei Xu
In-Reply-To: <20171101165953-mutt-send-email-mst@kernel.org>
On 2017年11月01日 23:03, Michael S. Tsirkin wrote:
> On Wed, Nov 01, 2017 at 08:51:36PM +0800, Jason Wang wrote:
>>
>> On 2017年11月01日 00:36, Michael S. Tsirkin wrote:
>>> On Tue, Oct 31, 2017 at 06:27:20PM +0800, Jason Wang wrote:
>>>> We always poll tx for socket, this is sub optimal since:
>>>>
>>>> - we only want to be notified when sndbuf is available
>>>> - this will slightly increase the waitqueue traversing time and more
>>>> important, vhost could not benefit from commit
>>>> commit 9e641bdcfa4e
>>>> ("net-tun: restructure tun_do_read for better sleep/wakeup efficiency")
>>>> even if we've stopped rx polling during handle_rx() since tx poll
>>>> were still left in the waitqueue.
>>>>
>>>> Pktgen from a remote host to VM over mlx4 shows 5.5% improvements on
>>>> rx PPS. (from 1.27Mpps to 1.34Mpps)
>>>>
>>>> Cc: Wei Xu <wexu@redhat.com>
>>>> Cc: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>> Now that vhost_poll_stop happens on data path
>>> a lot, I'd say
>>> if (poll->wqh)
>>> there should be unlikely().
>> It has been there since 8241a1e466cd ("vhost_net: stop polling socket during
>> rx processing"). So it will be used for rx path too which unlikely() does
>> not work as well as the case in tx.
> Worth testing, does not have to block this patch.
Ok.
>>>
>>>> drivers/vhost/net.c | 11 ++++++++---
>>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>>> index 68677d9..286c3e4 100644
>>>> --- a/drivers/vhost/net.c
>>>> +++ b/drivers/vhost/net.c
>>>> @@ -471,6 +471,7 @@ static void handle_tx(struct vhost_net *net)
>>>> goto out;
>>>> vhost_disable_notify(&net->dev, vq);
>>>> + vhost_net_disable_vq(net, vq);
>>>> hdr_size = nvq->vhost_hlen;
>>>> zcopy = nvq->ubufs;
>>>> @@ -556,6 +557,8 @@ static void handle_tx(struct vhost_net *net)
>>>> % UIO_MAXIOV;
>>>> }
>>>> vhost_discard_vq_desc(vq, 1);
>>>> + if (err == -EAGAIN)
>>>> + vhost_net_enable_vq(net, vq);
>>>> break;
>>>> }
>>>> if (err != len)
>>> I would probably just enable it unconditionally here. Why not?
>>>
>> I thought we only care about the case of tun_sock_write_space() and for the
>> errors other than -EAGAIN, they have nothing to do with polling.
> We could thinkably get ENOMEM I guess. If we miss a code things
> get stuck - It's just easier not to add extra code IMHO.
ENOBUFS actually. I agree to remove the specific checking of -EAGAIN here.
>
>>>> @@ -1145,9 +1148,11 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>>>> r = vhost_vq_init_access(vq);
>>>> if (r)
>>>> goto err_used;
>>>> - r = vhost_net_enable_vq(n, vq);
>>>> - if (r)
>>>> - goto err_used;
>>>> + if (index == VHOST_NET_VQ_RX) {
>>>> + r = vhost_net_enable_vq(n, vq);
>>>> + if (r)
>>>> + goto err_used;
>>>> + }
>>>> oldubufs = nvq->ubufs;
>>>> nvq->ubufs = ubufs;
>>> This last chunk seems questionable. If queue has stuff in it
>>> when we connect the backend, we'll miss a wakeup.
>>> I suspect this can happen during migration.
>> Unless qemu pass a tap which s already had pending tx packets.
>>
>> I can remove this chuck, but if guest does not transmit any packet, rx can't
>> benefit from this.
>>
>> Thanks
> Not sure I understand the last sentence. vhost will stay
> polling the socket - why is that a problem?
No function problem at all. If guest does not transmit any packet, the
patch does not have any effect.
Will remove this chunk in next version.
Thanks
>
>>>
>>>> --
>>>> 2.7.4
>> _______________________________________________
>> Virtualization mailing list
>> Virtualization@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: Virtio BoF minutes from KVM Forum 2017
From: Michael S. Tsirkin @ 2017-11-02 3:40 UTC (permalink / raw)
To: Ilya Lesokhin
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <AM4PR0501MB2723A2C058045E70A66959B3D45F0@AM4PR0501MB2723.eurprd05.prod.outlook.com>
On Wed, Nov 01, 2017 at 06:12:09PM +0000, Ilya Lesokhin wrote:
> On Wednesday, November 1, 2017 7:35 PM, Michael S. Tsirkin wrote:
> > > You have to either use and additional descriptor for metadata per chain.
> > > Or putting it in one of the buffers and forcing the lifetime of the metadata
> > and data to be the same.
> >
> > That's true. It would be easy to make descriptors e.g. 32 bytes each, so
> > you can add extra data in there. Or if we can live with wasting some
> > bytes per descriptor, we could add a descriptor flag that marks the
> > address field as meta-data. You could then chain it with a regular
> > descriptor for data. However all in all the simplest option is probably
> > in the virtio header which can be linear with the packet.
> [I.L] In the current proposal descriptor size == SGE (scatter gather entry) size.
> I'm not sure that's a good idea.
> For example we are considering having an RX ring were you just post a list
> of PFN's so a sge is only 8 bytes.
You mean without length, flags etc? So when you are concerned about
memory usage because you have many users for buffers (like e.g. with
Linux networking), then sizing buffers dynamically helps a lot. Single
user cases like DPDK or more recently XDP are different and they
can afford making all buffers same size.
For sure 8 byte entries would reduce cache pressure. Question is
how to we handle so much variety in the ring layouts. Thoughts?
> I might be wrong here, so please correct me if that's the not the case.
> but I've gotten the impression that due to DPDK limitations you've focused
> on the use case where you have 1 SGE.
> I'm not sure that a representative workload for network devices,
> As LSO is an important offload.
I don't think we focused on DPDK limitations. For sure, lots of people
use LSO or other buffers with many s/g entries. But it also works pretty
well more or less whatever you do as you are able to pass a single large
packet then - so the per-packet overhead is generally amortized.
So if you are doing LSO, I'd say just use indirect.
> And the storage guys also complained about this issue.
Interesting. What was the complaint exactly.
>
> > I suspect a good way to do this would be to just pass offsets within the
> > buffer back and forth. I agree sticking such small messages in a
> > separate buffer is not ideal. How about an option of replacing PA
> > with this data?
> [I.L] PA?
Sorry, I really meant the addr field in the descriptor.
> >
> >
> >
> > > 3. There is a usage model where you have multiple produce rings
> > > And a single completion ring.
> >
> > What good is it though? It seems to perform worse than combining
> > producer and consumer in my testing.
> >
> [I.L] It might be that for virtio-net a single ring is better
> But are you really going to argue that its better in all possible use cases?
>
> >
> > > You could implement the completion ring using an additional virtio ring,
> > but
> > > The current model will require an extra indirection as it force you to write
> > into
> > > The buffers the descriptor in the completion ring point to. Rather than
> > writing the
> > > Completion into the ring itself.
> > > Additionally the device is still required to write to the original producer ring
> > > in addition to the completion ring.
> > >
> > > I think the best and most flexible design is to have variable size descriptor
> > that
> > > start with a dword header.
> > > The dword header will include - an ownership bit, an opcode and
> > descriptor length.
> > > The opcode and the "length" dwords following the header will be device
> > specific.
> >
> > This means that device needs to do two reads just to decode the
> > descriptor fully. This conflicts with feedback Intel has been giving on
> > list which is to try and reduce number of reads. With header linear with
> > the packet, you need two reads to start transmitting the packet.
> >
> [I.L] The device can do a single large read and do the parsing afterword's.
For sure but that wastes some pcie bandwidth.
> You could also use the doorbell to tell the device how much to read.
We currently use that to pass address of last descriptor.
>
> > Seems to look like the avail bit in the kvm forum presentation.
> >
> [I.L] I don't want to argue over the name. The main difference in my
> proposal is that the device doesn't need to write to the descriptor.
> If it wants to you can define a separate bit for that.
A theoretical analysis shows less cache line bounces
if device writes and driver writes go to same location.
A micro-benchmark and dpdk tests seem to match that.
If you want to split them, how about a test showing
either a benefit for software or an explanation about why it's
significantly different for hardware than software?
> > > Each device (or device class) can choose whether completions are reported
> > directly inside
> > > the descriptors in that ring or in a separate completion ring.
> > >
> > > completions rings can be implemented in an efficient manner with this
> > design.
> > > The driver will initialize a dedicated completion ring with empty completion
> > sized descriptors.
> > > And the device will write the completions directly into the ring.
> >
> > I assume when you say completion you mean used entries, if I'm wrong
> > please correct me. In fact with the proposal in the kvm forum
> > presentation it is possible to write used entries in a separate address
> > as opposed to overlapping the available entries. If you are going to
> > support skipping writing back some used descriptors then accounting
> > would have to change slightly since driver won't be able to reset used
> > flags then. But in the past in all tests I've written this separate
> > ring underperforms a shared ring.
> >
> [I.L] A completion is a used entry + device specific metadata.
> I don't remember seeing an option to write used entries in a separate address,
> I'll appreciate it if you can point me to the right direction.
It wasn't described in the talk.
But it's simply this: driver detects used entry by detecting a used bit
flip. If device does not use the option to skip writing back some used
entries then there's no need for used entries written by device and by
driver to overlap. If device does skip then we need them to overlap as
driver also needs to reset the used flag so used != avail.
> Regarding the shared ring vs separate ring, I can't really argue with you
> as I haven't done the relevant measurement.
> I'm just saying it might not always be optimal in all use case,
> So you should consider leaving both options open.
>
> Its entirely possible that for virtio-net you want a single ring,
> Where for PV-RDMA you want separate rings.
Well RDMA consortium decided a low-level API for cards will help
application portability, and that spec has a concept of completion
queues which are shared between request queues. So the combined ring
optimization kind of goes out the window for that kind of device :) I'm
not sure just splitting out used rings will be enough though.
It's not a great fit for virtio right now, if someone's interested
in changing rings to match that use-case I'm all ears.
--
MST
^ permalink raw reply
* RE: Virtio BoF minutes from KVM Forum 2017
From: Ilya Lesokhin @ 2017-11-02 8:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <20171101214417-mutt-send-email-mst@kernel.org>
On Thursday, November 02, 2017 5:40 AM, Michael S. Tsirkin wrote:
> > [I.L] In the current proposal descriptor size == SGE (scatter gather entry) size.
> > I'm not sure that's a good idea.
> > For example we are considering having an RX ring were you just post a
> > list of PFN's so a sge is only 8 bytes.
>
> You mean without length, flags etc? So when you are concerned about memory
> usage because you have many users for buffers (like e.g. with Linux
> networking), then sizing buffers dynamically helps a lot. Single user cases like
> DPDK or more recently XDP are different and they can afford making all buffers
> same size.
>
[I.L] Yes, no length or flags, we just fill it with packets back to back, so
Memory usage wise it quite efficient.
> For sure 8 byte entries would reduce cache pressure. Question is how to we
> handle so much variety in the ring layouts. Thoughts?
>
[I.L] I agree that there are downsides to too much variety in the ring layouts.
It does seem that to get to most out of the ring layout I suggested
will require changing to the vring API to work with descriptors (== work requests)
rather than sg lists.
I guess I need to think about it a bit more.
> I don't think we focused on DPDK limitations. For sure, lots of people use LSO or
> other buffers with many s/g entries. But it also works pretty well more or less
> whatever you do as you are able to pass a single large packet then - so the per-
> packet overhead is generally amortized.
>
[I.L] I see you point, I'll have to think about it some more.
> > And the storage guys also complained about this issue.
>
> Interesting. What was the complaint exactly.
>
[I.L] That scatter gather is Important and that we shouldn't
Optimize for the single SGE case.
But the point you made earlier about amortization also applies here.
> > [I.L] The device can do a single large read and do the parsing afterword's.
>
> For sure but that wastes some pcie bandwidth.
>
> > You could also use the doorbell to tell the device how much to read.
>
> We currently use that to pass address of last descriptor.
[I.L] If you read up to the last descriptor you don't waste any pcie bandwidth.
> >
> > > Seems to look like the avail bit in the kvm forum presentation.
> > >
> > [I.L] I don't want to argue over the name. The main difference in my
> > proposal is that the device doesn't need to write to the descriptor.
> > If it wants to you can define a separate bit for that.
>
> A theoretical analysis shows less cache line bounces if device writes and driver
> writes go to same location.
> A micro-benchmark and dpdk tests seem to match that.
>
> If you want to split them, how about a test showing either a benefit for software
> or an explanation about why it's significantly different for hardware than
> software?
>
[I.L] The separate bit can be in the same cacheline.
> > I don't remember seeing an option to write used entries in a separate
> > address, I'll appreciate it if you can point me to the right direction.
>
> It wasn't described in the talk.
>
> But it's simply this: driver detects used entry by detecting a used bit flip. If
> device does not use the option to skip writing back some used entries then
> there's no need for used entries written by device and by driver to overlap. If
> device does skip then we need them to overlap as driver also needs to reset the
> used flag so used != avail.
>
[I.L] I don't follow, how does the device inform the driver the a descriptor has been
Processed?
> > Regarding the shared ring vs separate ring, I can't really argue with
> > you as I haven't done the relevant measurement.
> > I'm just saying it might not always be optimal in all use case, So you
> > should consider leaving both options open.
> >
> > It's entirely possible that for virtio-net you want a single ring,
> > Where for PV-RDMA you want separate rings.
>
> Well RDMA consortium decided a low-level API for cards will help application
> portability, and that spec has a concept of completion queues which are shared
> between request queues. So the combined ring optimization kind of goes out
> the window for that kind of device :) I'm not sure just splitting out used rings will
> be enough though.
>
I didn't say the splitting the used ring is going to be enough. I suggested two things:
1. don't force the device to write to the request queue.
2. Allow efficient implementation of completion queues
Through support for inline descriptors.
In any case, You've given me some valuable feedback
and I have better understanding of why you went with the
current ring layout.
Thanks,
Ilya
^ permalink raw reply
* Re: Possible unsafe usage of skb->cb in virtio-net
From: Michael S. Tsirkin @ 2017-11-02 13:01 UTC (permalink / raw)
To: Ilya Lesokhin; +Cc: netdev, Willem de Bruijn, David Miller, virtualization
In-Reply-To: <VI1PR0501MB2734AAE4311FE84881F622A2D45C0@VI1PR0501MB2734.eurprd05.prod.outlook.com>
On Thu, Nov 02, 2017 at 11:40:36AM +0000, Ilya Lesokhin wrote:
> Hi,
> I've noticed that the virtio-net uses skb->cb.
>
> I don't know all the detail by my understanding is it caused problem with the mlx5 driver
> and was fixed here:
> https://github.com/torvalds/linux/commit/34802a42b3528b0e18ea4517c8b23e1214a09332
>
> Thanks,
> Ilya
Thanks a lot for the pointer.
I think this was in response to this:
https://patchwork.ozlabs.org/patch/558324/
> >
> > + skb_push(skb, skb->data - skb_data_orig);
> > sq->skb[pi] = skb;
> >
> > MLX5E_TX_SKB_CB(skb)->num_wqebbs = DIV_ROUND_UP(ds_cnt,
>
> And in the middle of this we have:
>
> skb_pull_inline(skb, ihs);
>
> This is looks illegal.
>
> You must not modify the data pointers of any SKB that you receive for
> sending via ->ndo_start_xmit() unless you know that absolutely you are
> the one and only reference that exists to that SKB.
>
> And exactly for the case you are trying to "fix" here, you do not. If
> the SKB is cloned, or has an elevated users count, someone else can be
> looking at it exactly at the same time you are messing with the data
> pointers.
>
> I bet mlx4 has this bug too.
>
> You must fix this properly, by keeping track of an offset or similar
> internally to your driver, rather than changing the SKB data pointers.
What virtio does is this:
can_push = vi->any_header_sg &&
!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
/* Even if we can, don't push here yet as this would skew
* csum_start offset below. */
if (can_push)
hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len);
else
hdr = skb_vnet_hdr(skb);
This doesn't change the data pointers in a cloned skb but it does change the cb.
Is it true that it's illegal to touch the cb in a cloned skb then?
--
MST
^ permalink raw reply
* Re: [PATCH-tip v2 2/2] x86/xen: Deprecate xen_nopvspin
From: Waiman Long @ 2017-11-02 13:25 UTC (permalink / raw)
To: Boris Ostrovsky, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Jonathan Corbet
Cc: Juergen Gross, Rusty Russell, kvm, Radim Krčmář,
Peter Zijlstra, x86, linux-kernel, virtualization, Paolo Bonzini,
xen-devel, Alok Kataria
In-Reply-To: <5c36ed18-e59e-1ea3-28e0-414f88965761@oracle.com>
On 11/01/2017 06:01 PM, Boris Ostrovsky wrote:
> On 11/01/2017 04:58 PM, Waiman Long wrote:
>> +/* TODO: To be removed in a future kernel version */
>> static __init int xen_parse_nopvspin(char *arg)
>> {
>> - xen_pvspin = false;
>> + pr_warn("xen_nopvspin is deprecated, replace it with \"pvlock_type=queued\"!\n");
>> + if (!pv_spinlock_type)
>> + pv_spinlock_type = locktype_queued;
> Since we currently end up using unfair locks and because you are
> deprecating xen_nopvspin I wonder whether it would be better to set this
> to locktype_unfair so that current behavior doesn't change. (Sorry, I
> haven't responded to your earlier message before you posted this). Juergen?
I think the latest patch from Juergen in tip is to use native qspinlock
when xen_nopvspin is specified. Right? That is why I made the current
choice. I can certainly change to unfair if it is what you guys want.
> I am also not sure I agree with making pv_spinlock an enum *and* a
> bitmask at the same time. I understand that it makes checks easier but I
> think not assuming a value or a pattern would be better, especially
> since none of the uses is on a critical path.
>
> (For example, !pv_spinlock_type is the same as locktype_auto, which is
> defined but never used)
OK, I will take out the enum and make explicit use of locktype_auto.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH-tip v2 2/2] x86/xen: Deprecate xen_nopvspin
From: Juergen Gross @ 2017-11-02 13:28 UTC (permalink / raw)
To: Waiman Long, Boris Ostrovsky, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Jonathan Corbet
Cc: Rusty Russell, kvm, Radim Krčmář, Peter Zijlstra,
x86, linux-kernel, virtualization, Paolo Bonzini, xen-devel,
Alok Kataria
In-Reply-To: <2fb79da7-4c95-160a-a626-670c536f490a@redhat.com>
On 02/11/17 14:25, Waiman Long wrote:
> On 11/01/2017 06:01 PM, Boris Ostrovsky wrote:
>> On 11/01/2017 04:58 PM, Waiman Long wrote:
>>> +/* TODO: To be removed in a future kernel version */
>>> static __init int xen_parse_nopvspin(char *arg)
>>> {
>>> - xen_pvspin = false;
>>> + pr_warn("xen_nopvspin is deprecated, replace it with \"pvlock_type=queued\"!\n");
>>> + if (!pv_spinlock_type)
>>> + pv_spinlock_type = locktype_queued;
>> Since we currently end up using unfair locks and because you are
>> deprecating xen_nopvspin I wonder whether it would be better to set this
>> to locktype_unfair so that current behavior doesn't change. (Sorry, I
>> haven't responded to your earlier message before you posted this). Juergen?
>
> I think the latest patch from Juergen in tip is to use native qspinlock
> when xen_nopvspin is specified. Right? That is why I made the current
> choice. I can certainly change to unfair if it is what you guys want.
No, when we are keeping xen_nopvspin (even as deprecated) it should
behave as designed, so locktype_queued is correct.
Juergen
^ permalink raw reply
* Re: Possible unsafe usage of skb->cb in virtio-net
From: Willem de Bruijn @ 2017-11-03 4:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Network Development, Willem de Bruijn, David Miller,
virtualization
In-Reply-To: <20171102142758-mutt-send-email-mst@kernel.org>
On Thu, Nov 2, 2017 at 10:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Nov 02, 2017 at 11:40:36AM +0000, Ilya Lesokhin wrote:
>> Hi,
>> I've noticed that the virtio-net uses skb->cb.
>>
>> I don't know all the detail by my understanding is it caused problem with the mlx5 driver
>> and was fixed here:
>> https://github.com/torvalds/linux/commit/34802a42b3528b0e18ea4517c8b23e1214a09332
>>
>> Thanks,
>> Ilya
>
> Thanks a lot for the pointer.
>
> I think this was in response to this:
> https://patchwork.ozlabs.org/patch/558324/
>
>> >
>> > + skb_push(skb, skb->data - skb_data_orig);
>> > sq->skb[pi] = skb;
>> >
>> > MLX5E_TX_SKB_CB(skb)->num_wqebbs = DIV_ROUND_UP(ds_cnt,
>>
>> And in the middle of this we have:
>>
>> skb_pull_inline(skb, ihs);
>>
>> This is looks illegal.
>>
>> You must not modify the data pointers of any SKB that you receive for
>> sending via ->ndo_start_xmit() unless you know that absolutely you are
>> the one and only reference that exists to that SKB.
>>
>> And exactly for the case you are trying to "fix" here, you do not. If
>> the SKB is cloned, or has an elevated users count, someone else can be
>> looking at it exactly at the same time you are messing with the data
>> pointers.
>>
>> I bet mlx4 has this bug too.
>>
>> You must fix this properly, by keeping track of an offset or similar
>> internally to your driver, rather than changing the SKB data pointers.
>
> What virtio does is this:
>
> can_push = vi->any_header_sg &&
> !((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> !skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> /* Even if we can, don't push here yet as this would skew
> * csum_start offset below. */
> if (can_push)
> hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len);
> else
> hdr = skb_vnet_hdr(skb);
>
>
> This doesn't change the data pointers in a cloned skb but it does change the cb.
> Is it true that it's illegal to touch the cb in a cloned skb then?
I don't have all the context for this bug. But in general, clones do not share
the struct sk_buff, which holds the CB. So skb_push and skb_pull_inline
cannot affect the view of other clones. If an skb is shared, that's a different
story.
^ 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