From: "Michael S. Tsirkin" <mst@redhat.com>
To: "Li, Liang Z" <liang.z.li@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"virtio-dev@lists.oasis-open.org"
<virtio-dev@lists.oasis-open.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"amit.shah@redhat.com" <amit.shah@redhat.com>,
"Hansen, Dave" <dave.hansen@intel.com>,
"cornelia.huck@de.ibm.com" <cornelia.huck@de.ibm.com>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
"david@redhat.com" <david@redhat.com>,
"aarcange@redhat.com" <aarcange@redhat.com>,
"dgilbert@redhat.com" <dgilbert@redhat.com>,
"quintela@redhat.com" <quintela@redhat.com>
Subject: Re: [PATCH v6 kernel 3/5] virtio-balloon: speed up inflate/deflate process
Date: Wed, 18 Jan 2017 17:30:12 +0200 [thread overview]
Message-ID: <20170118172401-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <F2CBF3009FA73547804AE4C663CAB28E3C355672@shsmsx102.ccr.corp.intel.com>
On Wed, Jan 18, 2017 at 04:56:58AM +0000, Li, Liang Z wrote:
> > > - virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > > - virtqueue_kick(vq);
> > > +static void do_set_resp_bitmap(struct virtio_balloon *vb,
> > > + unsigned long base_pfn, int pages)
> > >
> > > - /* When host has read buffer, this completes via balloon_ack */
> > > - wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > +{
> > > + __le64 *range = vb->resp_data + vb->resp_pos;
> > >
> > > + if (pages > (1 << VIRTIO_BALLOON_NR_PFN_BITS)) {
> > > + /* when the length field can't contain pages, set it to 0 to
> >
> > /*
> > * Multi-line
> > * comments
> > * should look like this.
> > */
> >
> > Also, pls start sentences with an upper-case letter.
> >
>
> Sorry for that.
>
> > > + * indicate the actual length is in the next __le64;
> > > + */
> >
> > This is part of the interface so should be documented as such.
> >
> > > + *range = cpu_to_le64((base_pfn <<
> > > + VIRTIO_BALLOON_NR_PFN_BITS) | 0);
> > > + *(range + 1) = cpu_to_le64(pages);
> > > + vb->resp_pos += 2;
> >
> > Pls use structs for this kind of stuff.
>
> I am not sure if you mean to use
>
> struct range {
> __le64 pfn: 52;
> __le64 nr_page: 12
> }
> Instead of the shift operation?
Not just that. You want to add a pages field as well.
Generally describe the format in the header in some way
so host and guest can easily stay in sync.
All the pointer math and void * means we get zero type
safety and I'm not happy about it.
> I didn't use this way because I don't want to include 'virtio-balloon.h' in page_alloc.c,
> or copy the define of this struct in page_alloc.c
>
> Thanks!
> Liang
It's not good that virtio format seeps out to page_alloc anyway.
If unavoidable it is not a good idea to try to hide this fact,
people will assume they can change the format at will.
--
MST
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: "Li, Liang Z" <liang.z.li@intel.com>
Cc: "aarcange@redhat.com" <aarcange@redhat.com>,
"virtio-dev@lists.oasis-open.org"
<virtio-dev@lists.oasis-open.org>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"Hansen, Dave" <dave.hansen@intel.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"amit.shah@redhat.com" <amit.shah@redhat.com>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
"dgilbert@redhat.com" <dgilbert@redhat.com>
Subject: Re: [PATCH v6 kernel 3/5] virtio-balloon: speed up inflate/deflate process
Date: Wed, 18 Jan 2017 17:30:12 +0200 [thread overview]
Message-ID: <20170118172401-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <F2CBF3009FA73547804AE4C663CAB28E3C355672@shsmsx102.ccr.corp.intel.com>
On Wed, Jan 18, 2017 at 04:56:58AM +0000, Li, Liang Z wrote:
> > > - virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > > - virtqueue_kick(vq);
> > > +static void do_set_resp_bitmap(struct virtio_balloon *vb,
> > > + unsigned long base_pfn, int pages)
> > >
> > > - /* When host has read buffer, this completes via balloon_ack */
> > > - wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > +{
> > > + __le64 *range = vb->resp_data + vb->resp_pos;
> > >
> > > + if (pages > (1 << VIRTIO_BALLOON_NR_PFN_BITS)) {
> > > + /* when the length field can't contain pages, set it to 0 to
> >
> > /*
> > * Multi-line
> > * comments
> > * should look like this.
> > */
> >
> > Also, pls start sentences with an upper-case letter.
> >
>
> Sorry for that.
>
> > > + * indicate the actual length is in the next __le64;
> > > + */
> >
> > This is part of the interface so should be documented as such.
> >
> > > + *range = cpu_to_le64((base_pfn <<
> > > + VIRTIO_BALLOON_NR_PFN_BITS) | 0);
> > > + *(range + 1) = cpu_to_le64(pages);
> > > + vb->resp_pos += 2;
> >
> > Pls use structs for this kind of stuff.
>
> I am not sure if you mean to use
>
> struct range {
> __le64 pfn: 52;
> __le64 nr_page: 12
> }
> Instead of the shift operation?
Not just that. You want to add a pages field as well.
Generally describe the format in the header in some way
so host and guest can easily stay in sync.
All the pointer math and void * means we get zero type
safety and I'm not happy about it.
> I didn't use this way because I don't want to include 'virtio-balloon.h' in page_alloc.c,
> or copy the define of this struct in page_alloc.c
>
> Thanks!
> Liang
It's not good that virtio format seeps out to page_alloc anyway.
If unavoidable it is not a good idea to try to hide this fact,
people will assume they can change the format at will.
--
MST
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: "Li, Liang Z" <liang.z.li@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"virtio-dev@lists.oasis-open.org"
<virtio-dev@lists.oasis-open.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"amit.shah@redhat.com" <amit.shah@redhat.com>,
"Hansen, Dave" <dave.hansen@intel.com>,
"cornelia.huck@de.ibm.com" <cornelia.huck@de.ibm.com>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
"david@redhat.com" <david@redhat.com>,
"aarcange@redhat.com" <aarcange@redhat.com>,
"dgilbert@redhat.com" <dgilbert@redhat.com>,
"quintela@redhat.com" <quintela@redhat.com>
Subject: Re: [PATCH v6 kernel 3/5] virtio-balloon: speed up inflate/deflate process
Date: Wed, 18 Jan 2017 17:30:12 +0200 [thread overview]
Message-ID: <20170118172401-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <F2CBF3009FA73547804AE4C663CAB28E3C355672@shsmsx102.ccr.corp.intel.com>
On Wed, Jan 18, 2017 at 04:56:58AM +0000, Li, Liang Z wrote:
> > > - virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > > - virtqueue_kick(vq);
> > > +static void do_set_resp_bitmap(struct virtio_balloon *vb,
> > > + unsigned long base_pfn, int pages)
> > >
> > > - /* When host has read buffer, this completes via balloon_ack */
> > > - wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > +{
> > > + __le64 *range = vb->resp_data + vb->resp_pos;
> > >
> > > + if (pages > (1 << VIRTIO_BALLOON_NR_PFN_BITS)) {
> > > + /* when the length field can't contain pages, set it to 0 to
> >
> > /*
> > * Multi-line
> > * comments
> > * should look like this.
> > */
> >
> > Also, pls start sentences with an upper-case letter.
> >
>
> Sorry for that.
>
> > > + * indicate the actual length is in the next __le64;
> > > + */
> >
> > This is part of the interface so should be documented as such.
> >
> > > + *range = cpu_to_le64((base_pfn <<
> > > + VIRTIO_BALLOON_NR_PFN_BITS) | 0);
> > > + *(range + 1) = cpu_to_le64(pages);
> > > + vb->resp_pos += 2;
> >
> > Pls use structs for this kind of stuff.
>
> I am not sure if you mean to use
>
> struct range {
> __le64 pfn: 52;
> __le64 nr_page: 12
> }
> Instead of the shift operation?
Not just that. You want to add a pages field as well.
Generally describe the format in the header in some way
so host and guest can easily stay in sync.
All the pointer math and void * means we get zero type
safety and I'm not happy about it.
> I didn't use this way because I don't want to include 'virtio-balloon.h' in page_alloc.c,
> or copy the define of this struct in page_alloc.c
>
> Thanks!
> Liang
It's not good that virtio format seeps out to page_alloc anyway.
If unavoidable it is not a good idea to try to hide this fact,
people will assume they can change the format at will.
--
MST
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: "Li, Liang Z" <liang.z.li@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"virtio-dev@lists.oasis-open.org"
<virtio-dev@lists.oasis-open.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"amit.shah@redhat.com" <amit.shah@redhat.com>,
"Hansen, Dave" <dave.hansen@intel.com>,
"cornelia.huck@de.ibm.com" <cornelia.huck@de.ibm.com>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
"david@redhat.com" <david@redhat.com>,
"aarcange@redhat.com" <aarcange@redhat.com>,
"dgilbert@redhat.com" <dgilbert@redhat.com>,
"quintela@redhat.com" <quintela@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v6 kernel 3/5] virtio-balloon: speed up inflate/deflate process
Date: Wed, 18 Jan 2017 17:30:12 +0200 [thread overview]
Message-ID: <20170118172401-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <F2CBF3009FA73547804AE4C663CAB28E3C355672@shsmsx102.ccr.corp.intel.com>
On Wed, Jan 18, 2017 at 04:56:58AM +0000, Li, Liang Z wrote:
> > > - virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > > - virtqueue_kick(vq);
> > > +static void do_set_resp_bitmap(struct virtio_balloon *vb,
> > > + unsigned long base_pfn, int pages)
> > >
> > > - /* When host has read buffer, this completes via balloon_ack */
> > > - wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > +{
> > > + __le64 *range = vb->resp_data + vb->resp_pos;
> > >
> > > + if (pages > (1 << VIRTIO_BALLOON_NR_PFN_BITS)) {
> > > + /* when the length field can't contain pages, set it to 0 to
> >
> > /*
> > * Multi-line
> > * comments
> > * should look like this.
> > */
> >
> > Also, pls start sentences with an upper-case letter.
> >
>
> Sorry for that.
>
> > > + * indicate the actual length is in the next __le64;
> > > + */
> >
> > This is part of the interface so should be documented as such.
> >
> > > + *range = cpu_to_le64((base_pfn <<
> > > + VIRTIO_BALLOON_NR_PFN_BITS) | 0);
> > > + *(range + 1) = cpu_to_le64(pages);
> > > + vb->resp_pos += 2;
> >
> > Pls use structs for this kind of stuff.
>
> I am not sure if you mean to use
>
> struct range {
> __le64 pfn: 52;
> __le64 nr_page: 12
> }
> Instead of the shift operation?
Not just that. You want to add a pages field as well.
Generally describe the format in the header in some way
so host and guest can easily stay in sync.
All the pointer math and void * means we get zero type
safety and I'm not happy about it.
> I didn't use this way because I don't want to include 'virtio-balloon.h' in page_alloc.c,
> or copy the define of this struct in page_alloc.c
>
> Thanks!
> Liang
It's not good that virtio format seeps out to page_alloc anyway.
If unavoidable it is not a good idea to try to hide this fact,
people will assume they can change the format at will.
--
MST
next prev parent reply other threads:[~2017-01-18 15:30 UTC|newest]
Thread overview: 100+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-21 6:52 [PATCH v6 kernel 0/5] Extend virtio-balloon for fast (de)inflating & fast live migration Liang Li
2016-12-21 6:52 ` [Qemu-devel] " Liang Li
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` [PATCH v6 kernel 1/5] virtio-balloon: rework deflate to add page to a list Liang Li
2016-12-21 6:52 ` [Qemu-devel] " Liang Li
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` [PATCH v6 kernel 2/5] virtio-balloon: define new feature bit and head struct Liang Li
2016-12-21 6:52 ` [Qemu-devel] " Liang Li
2016-12-21 6:52 ` Liang Li
2017-01-12 19:43 ` Michael S. Tsirkin
2017-01-12 19:43 ` Michael S. Tsirkin
2017-01-12 19:43 ` [Qemu-devel] " Michael S. Tsirkin
2017-01-12 19:43 ` Michael S. Tsirkin
2017-01-13 9:24 ` [virtio-dev] " Li, Liang Z
2017-01-13 9:24 ` [Qemu-devel] " Li, Liang Z
2017-01-13 9:24 ` Li, Liang Z
2017-01-17 19:11 ` Michael S. Tsirkin
2017-01-17 19:11 ` [Qemu-devel] " Michael S. Tsirkin
2017-01-17 19:11 ` Michael S. Tsirkin
2017-01-18 1:55 ` Li, Liang Z
2017-01-18 1:55 ` Li, Liang Z
2017-01-18 1:55 ` [Qemu-devel] " Li, Liang Z
2017-01-18 1:55 ` Li, Liang Z
2017-01-18 15:30 ` Michael S. Tsirkin
2017-01-18 15:30 ` Michael S. Tsirkin
2017-01-18 15:30 ` [Qemu-devel] " Michael S. Tsirkin
2017-01-18 15:30 ` Michael S. Tsirkin
2017-01-18 15:30 ` Michael S. Tsirkin
2017-01-19 1:30 ` [virtio-dev] " Li, Liang Z
2017-01-19 1:30 ` [Qemu-devel] " Li, Liang Z
2017-01-19 1:30 ` Li, Liang Z
2017-01-19 1:30 ` Li, Liang Z
2017-01-19 1:30 ` [virtio-dev] " Li, Liang Z
2017-01-17 19:11 ` Michael S. Tsirkin
2017-01-13 9:24 ` Li, Liang Z
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` [PATCH v6 kernel 3/5] virtio-balloon: speed up inflate/deflate process Liang Li
2016-12-21 6:52 ` [Qemu-devel] " Liang Li
2016-12-21 6:52 ` Liang Li
2017-01-17 19:15 ` Michael S. Tsirkin
2017-01-17 19:15 ` [Qemu-devel] " Michael S. Tsirkin
2017-01-17 19:15 ` Michael S. Tsirkin
2017-01-18 4:56 ` Li, Liang Z
2017-01-18 4:56 ` Li, Liang Z
2017-01-18 4:56 ` [Qemu-devel] " Li, Liang Z
2017-01-18 4:56 ` Li, Liang Z
2017-01-18 4:56 ` Li, Liang Z
2017-01-18 15:30 ` Michael S. Tsirkin [this message]
2017-01-18 15:30 ` [Qemu-devel] " Michael S. Tsirkin
2017-01-18 15:30 ` Michael S. Tsirkin
2017-01-18 15:30 ` Michael S. Tsirkin
2017-01-19 1:44 ` Li, Liang Z
2017-01-19 1:44 ` [Qemu-devel] " Li, Liang Z
2017-01-19 1:44 ` Li, Liang Z
2017-01-19 1:44 ` Li, Liang Z
2017-01-20 16:34 ` Michael S. Tsirkin
2017-01-20 16:34 ` [Qemu-devel] " Michael S. Tsirkin
2017-01-20 16:34 ` Michael S. Tsirkin
2017-01-20 16:34 ` Michael S. Tsirkin
2017-01-17 19:15 ` Michael S. Tsirkin
2017-01-20 11:48 ` Dr. David Alan Gilbert
2017-01-20 11:48 ` [Qemu-devel] " Dr. David Alan Gilbert
2017-01-20 11:48 ` Dr. David Alan Gilbert
2017-02-04 4:35 ` Li, Liang Z
2017-02-04 4:35 ` [Qemu-devel] " Li, Liang Z
2017-02-04 4:35 ` Li, Liang Z
2017-02-04 4:35 ` Li, Liang Z
2017-01-20 11:48 ` Dr. David Alan Gilbert
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` [PATCH v6 kernel 4/5] virtio-balloon: define flags and head for host request vq Liang Li
2016-12-21 6:52 ` [Qemu-devel] " Liang Li
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` [PATCH v6 kernel 5/5] virtio-balloon: tell host vm's unused page info Liang Li
2016-12-21 6:52 ` [Qemu-devel] " Liang Li
2016-12-21 6:52 ` Liang Li
2016-12-21 6:52 ` Liang Li
2017-01-10 6:43 ` [PATCH v6 kernel 0/5] Extend virtio-balloon for fast (de)inflating & fast live migration Li, Liang Z
2017-01-10 6:43 ` [Qemu-devel] " Li, Liang Z
2017-01-10 6:43 ` Li, Liang Z
2017-01-10 6:43 ` Li, Liang Z
2017-01-18 10:09 ` David Hildenbrand
2017-01-18 10:09 ` [Qemu-devel] " David Hildenbrand
2017-01-18 10:09 ` David Hildenbrand
2017-01-18 10:09 ` David Hildenbrand
2017-01-18 13:29 ` Li, Liang Z
2017-01-18 13:29 ` [Qemu-devel] " Li, Liang Z
2017-01-18 13:29 ` Li, Liang Z
2017-01-18 13:29 ` Li, Liang Z
2017-01-18 13:29 ` Li, Liang Z
2017-01-18 15:38 ` Michael S. Tsirkin
2017-01-18 15:38 ` Michael S. Tsirkin
2017-01-18 15:38 ` [Qemu-devel] " Michael S. Tsirkin
2017-01-18 15:38 ` Michael S. Tsirkin
2017-01-19 17:24 ` David Hildenbrand
2017-01-19 17:24 ` David Hildenbrand
2017-01-19 17:24 ` [Qemu-devel] " David Hildenbrand
2017-01-19 17:24 ` David Hildenbrand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170118172401-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=aarcange@redhat.com \
--cc=amit.shah@redhat.com \
--cc=cornelia.huck@de.ibm.com \
--cc=dave.hansen@intel.com \
--cc=david@redhat.com \
--cc=dgilbert@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=liang.z.li@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
--cc=virtio-dev@lists.oasis-open.org \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.