On Tue, Jun 26, 2018 at 11:46:35AM +0800, Wei Wang wrote:
> On 06/26/2018 09:37 AM, Michael S. Tsirkin wrote:
> > On Mon, Jun 25, 2018 at 08:05:10PM +0800, Wei Wang wrote:
> >
> > > @@ -326,17 +353,6 @@ static void stats_handle_request(struct virtio_balloon *vb)
> > > virtqueue_kick(vq);
> > > }
> > > -static void virtballoon_changed(struct virtio_device *vdev)
> > > -{
> > > - struct virtio_balloon *vb = vdev->priv;
> > > - unsigned long flags;
> > > -
> > > - spin_lock_irqsave(&vb->stop_update_lock, flags);
> > > - if (!vb->stop_update)
> > > - queue_work(system_freezable_wq, &vb->update_balloon_size_work);
> > > - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> > > -}
> > > -
> > > static inline s64 towards_target(struct virtio_balloon *vb)
> > > {
> > > s64 target;
> > > @@ -353,6 +369,35 @@ static inline s64 towards_target(struct virtio_balloon *vb)
> > > return target - vb->num_pages;
> > > }
> > > +static void virtballoon_changed(struct virtio_device *vdev)
> > > +{
> > > + struct virtio_balloon *vb = vdev->priv;
> > > + unsigned long flags;
> > > + s64 diff = towards_target(vb);
> > > +
> > > + if (diff) {
> > > + spin_lock_irqsave(&vb->stop_update_lock, flags);
> > > + if (!vb->stop_update)
> > > + queue_work(system_freezable_wq,
> > > + &vb->update_balloon_size_work);
> > > + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> > > + }
> > > +
> > > + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> > > + virtio_cread(vdev, struct virtio_balloon_config,
> > > + free_page_report_cmd_id, &vb->cmd_id_received);
> > > + if (vb->cmd_id_received !=
> > > + VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID &&
> > > + vb->cmd_id_received != vb->cmd_id_active) {
> > > + spin_lock_irqsave(&vb->stop_update_lock, flags);
> > > + if (!vb->stop_update)
> > > + queue_work(vb->balloon_wq,
> > > + &vb->report_free_page_work);
> > > + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> > > + }
> > > + }
> > > +}
> > > +
> > > static void update_balloon_size(struct virtio_balloon *vb)
> > > {
> > > u32 actual = vb->num_pages;
> > > @@ -425,44 +470,253 @@ static void update_balloon_size_func(struct work_struct *work)
> > > queue_work(system_freezable_wq, work);
> > > }
> > > +static void free_page_vq_cb(struct virtqueue *vq)
> > > +{
> > > + unsigned int len;
> > > + void *buf;
> > > + struct virtio_balloon *vb = vq->vdev->priv;
> > > +
> > > + while (1) {
> > > + buf = virtqueue_get_buf(vq, &len);
> > > +
> > > + if (!buf || buf == &vb->cmd_start || buf == &vb->cmd_stop)
> > > + break;
> > If there's any buffer after this one we might never get another
> > callback.
>
> I think every used buffer can get the callback, because host takes from the
> arrays one by one, and puts back each with a vq notify.
It's probabky racy even in this case. Besides, host is free to do it in
any way that's legal in spec.
>
>
> > > + free_pages((unsigned long)buf, ARRAY_ALLOC_ORDER);
> > > + }
> > > +}
> > > +
> > > static int init_vqs(struct virtio_balloon *vb)
> > > {
> > > - struct virtqueue *vqs[3];
> > > - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
> > > - static const char * const names[] = { "inflate", "deflate", "stats" };
> > > - int err, nvqs;
> > > + struct virtqueue *vqs[VIRTIO_BALLOON_VQ_MAX];
> > > + vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX];
> > > + const char *names[VIRTIO_BALLOON_VQ_MAX];
> > > + struct scatterlist sg;
> > > + int ret;
> > > /*
> > > - * We expect two virtqueues: inflate and deflate, and
> > > - * optionally stat.
> > > + * Inflateq and deflateq are used unconditionally. The names[]
> > > + * will be NULL if the related feature is not enabled, which will
> > > + * cause no allocation for the corresponding virtqueue in find_vqs.
> > > */
> > > - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
> > > - err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
> > > - if (err)
> > > - return err;
> > > + callbacks[VIRTIO_BALLOON_VQ_INFLATE] = balloon_ack;
> > > + names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
> > > + callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
> > > + names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
> > > + names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> > > + names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> > > - vb->inflate_vq = vqs[0];
> > > - vb->deflate_vq = vqs[1];
> > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> > > - struct scatterlist sg;
> > > - unsigned int num_stats;
> > > - vb->stats_vq = vqs[2];
> > > + names[VIRTIO_BALLOON_VQ_STATS] = "stats";
> > > + callbacks[VIRTIO_BALLOON_VQ_STATS] = stats_request;
> > > + }
> > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> > > + names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq";
> > > + callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = free_page_vq_cb;
> > > + }
> > > +
> > > + ret = vb->vdev->config->find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX,
> > > + vqs, callbacks, names, NULL, NULL);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
> > > + vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
> > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> > > + vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS];
> > > /*
> > > * Prime this virtqueue with one buffer so the hypervisor can
> > > * use it to signal us later (it can't be broken yet!).
> > > */
> > > - num_stats = update_balloon_stats(vb);
> > > -
> > > - sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
> > > - if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
> > > - < 0)
> > > - BUG();
> > > + sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> > > + ret = virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb,
> > > + GFP_KERNEL);
> > > + if (ret) {
> > > + dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
> > > + __func__);
> > > + return ret;
> > > + }
> > Why the change? Is it more likely to happen now?
>
> Actually this part remains the same as the previous versions (e.g. v32). It
> is changed because we agreed that using BUG() isn't necessary here, and
> better to bail out nicely.
Why is this part of the hinting patch though? I'd rather have
a separate one.
>
>
> >
> > +/*
> > + * virtio_balloon_send_hints - send arrays of hints to host
> > + * @vb: the virtio_balloon struct
> > + * @arrays: the arrays of hints
> > + * @array_num: the number of arrays give by the caller
> > + * @last_array_hints: the number of hints in the last array
> > + *
> > + * Send hints to host array by array. This begins by sending a start cmd,
> > + * which contains a cmd id received from host and the free page block size in
> > + * bytes of each hint. At the end, a stop cmd is sent to host to indicate the
> > + * end of this reporting. If host actively requests to stop the reporting, free
> > + * the arrays that have not been sent.
> > + */
> > +static void virtio_balloon_send_hints(struct virtio_balloon *vb,
> > + __le64 **arrays,
> > + uint32_t array_num,
> > + uint32_t last_array_hints)
> > +{
> > + int err, i = 0;
> > + struct scatterlist sg;
> > + struct virtqueue *vq = vb->free_page_vq;
> > +
> > + /* Start by sending the received cmd id to host with an outbuf. */
> > + err = send_start_cmd_id(vb);
> > + if (unlikely(err))
> > + goto out_err;
> > + /* Kick host to start taking entries from the vq. */
> > + virtqueue_kick(vq);
> > +
> > + for (i = 0; i < array_num; i++) {
> > + /*
> > + * If a stop id or a new cmd id was just received from host,
> > + * stop the reporting, and free the remaining arrays that
> > + * haven't been sent to host.
> > + */
> > + if (vb->cmd_id_received != vb->cmd_id_active)
> > + goto out_free;
> > +
> > + if (i + 1 == array_num)
> > + sg_init_one(&sg, (void *)arrays[i],
> > + last_array_hints * sizeof(__le64));
> > + else
> > + sg_init_one(&sg, (void *)arrays[i], ARRAY_ALLOC_SIZE);
> > + err = virtqueue_add_inbuf(vq, &sg, 1, (void *)arrays[i],
> > + GFP_KERNEL);
> > + if (unlikely(err))
> > + goto out_err;
> > + }
> > +
> > + /* End by sending a stop id to host with an outbuf. */
> > + err = send_stop_cmd_id(vb);
> > + if (unlikely(err))
> > + goto out_err;
> > Don't we need to kick here?
>
> I think not needed, because we have kicked host about starting the report,
> and the host side optimization won't exit unless receiving this stop sign or
> the migration thread asks to exit.
You can't assume that. Host might want to sleep.
If it doesn't then it will disable notifications and kick will be free.
> >
> > > + int i;
> > > +
> > > + max_entries = max_free_page_blocks(ARRAY_ALLOC_ORDER);
> > > + entries_per_page = PAGE_SIZE / sizeof(__le64);
> > > + entries_per_array = entries_per_page * (1 << ARRAY_ALLOC_ORDER);
> > > + max_array_num = max_entries / entries_per_array +
> > > + !!(max_entries % entries_per_array);
> > > + arrays = kmalloc_array(max_array_num, sizeof(__le64 *), GFP_KERNEL);
> > Instead of all this mess, how about get_free_pages here as well?
>
> Sounds good, will replace kmalloc_array with __get_free_pages(),
Or alloc_pages, __ APIs are better avoided if possible.
> but still
> need the above calculation to get max_array_num.
Maybe alloc_pages?
> >
> > Also why do we need GFP_KERNEL for this?
>
> I guess it is better to use "__GFP_ATOMIC | __GFP_NOMEMALLOC", thanks.
>
> >
> >
> > > + if (!arrays)
> > > + return NULL;
> > > +
> > > + for (i = 0; i < max_array_num; i++) {
> > So we are getting a ton of memory here just to free it up a bit later.
> > Why doesn't get_from_free_page_list get the pages from free list for us?
> > We could also avoid the 1st allocation then - just build a list
> > of these.
>
> That wouldn't be a good choice for us. If we check how the regular
> allocation works, there are many many things we need to consider when pages
> are allocated to users.
> For example, we need to take care of the nr_free
> counter, we need to check the watermark and perform the related actions.
> Also the folks working on arch_alloc_page to monitor page allocation
> activities would get a surprise..if page allocation is allowed to work in
> this way.
>
mm/ code is well positioned to handle all this correctly.
>
>
>
> >
> > > + arrays[i] =
> > > + (__le64 *)__get_free_pages(__GFP_ATOMIC | __GFP_NOMEMALLOC,
> > > + ARRAY_ALLOC_ORDER);
> > Coding style says:
> >
> > Descendants are always substantially shorter than the parent and
> > are placed substantially to the right.
>
> Thanks, will rearrange it:
>
> arrays[i] = (__le64 *)__get_free_pages(__GFP_ATOMIC |
> __GFP_NOMEMALLOC, ARRAY_ALLOC_ORDER);
>
>
>
> >
> > > + if (!arrays[i]) {
> > Also if it does fail (small guest), shall we try with less arrays?
>
> I think it's not needed. If the free list is empty, no matter it is a huge
> guest or a small guest, get_from_free_page_list() will load nothing even we
> pass a small array to it.
>
>
> Best,
> Wei
Yes but the reason it's empty is maybe because we used a ton of
memory for all of the arrays. Why allocate a top level array at all?
Can't we pass in a list?
--
MST
