lkml.org 
[lkml]   [2019]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC][Patch v12 2/2] virtio-balloon: interface to support free page reporting
    From
    Date

    On 8/14/19 9:42 AM, Cornelia Huck wrote:
    > On Wed, 14 Aug 2019 07:47:40 -0400
    > Nitesh Narayan Lal <nitesh@redhat.com> wrote:
    >
    >> On 8/14/19 6:29 AM, Cornelia Huck wrote:
    >>> On Mon, 12 Aug 2019 09:12:35 -0400
    >>> Nitesh Narayan Lal <nitesh@redhat.com> wrote:
    >>>
    >>>> Enables the kernel to negotiate VIRTIO_BALLOON_F_REPORTING feature with
    >>>> the host. If it is available and page_reporting_flag is set to true,
    >>>> page_reporting is enabled and its callback is configured along with
    >>>> the max_pages count which indicates the maximum number of pages that
    >>>> can be isolated and reported at a time. Currently, only free pages of
    >>>> order >= (MAX_ORDER - 2) are reported. To prevent any false OOM
    >>>> max_pages count is set to 16.
    >>>>
    >>>> By default page_reporting feature is enabled and gets loaded as soon
    >>>> as the virtio-balloon driver is loaded. However, it could be disabled
    >>>> by writing the page_reporting_flag which is a virtio-balloon parameter.
    >>>>
    >>>> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com>
    >>>> ---
    >>>> drivers/virtio/Kconfig | 1 +
    >>>> drivers/virtio/virtio_balloon.c | 64 ++++++++++++++++++++++++++++-
    >>>> include/uapi/linux/virtio_balloon.h | 1 +
    >>>> 3 files changed, 65 insertions(+), 1 deletion(-)
    >>>>
    >>>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
    >>>> index 226fbb995fb0..defec00d4ee2 100644
    >>>> --- a/drivers/virtio/virtio_balloon.c
    >>>> +++ b/drivers/virtio/virtio_balloon.c
    >>> (...)
    >>>
    >>>> +static void virtballoon_page_reporting_setup(struct virtio_balloon *vb)
    >>>> +{
    >>>> + struct device *dev = &vb->vdev->dev;
    >>>> + int err;
    >>>> +
    >>>> + vb->page_reporting_conf.report = virtballoon_report_pages;
    >>>> + vb->page_reporting_conf.max_pages = PAGE_REPORTING_MAX_PAGES;
    >>>> + err = page_reporting_enable(&vb->page_reporting_conf);
    >>>> + if (err < 0) {
    >>>> + dev_err(dev, "Failed to enable reporting, err = %d\n", err);
    >>>> + page_reporting_flag = false;
    >>> Should we clear the feature bit in this case as well?
    >> I think yes.
    > Eww, I didn't recall that we don't call the ->probe callback until
    > after feature negotiation has finished, so scratch that particular idea.
    >
    > For what reasons may page_reporting_enable() fail?

    If the guest is low in memory and some allocation required for page reporting
    setup fails.

    > Does it make sense
    > to fail probing the device in that case? And does it make sense to
    > re-try later (i.e. leave page_reporting_flag set)?


    Re-trying to setup page reporting will mean that virtballoon_probe has to be
    called again.
    For which the driver has to be re-loaded, isn't?

    >
    >> If I am not wrong then in a case where page reporting setup fails for some
    >> reason and at a later point the user wants to re-enable it then for that balloon
    >> driver has to be reloaded.
    >> Which would mean re-negotiation of the feature bit.
    > Re-negotiation actually already happens if a driver is unbound and
    > rebound.
    >
    >>>
    >>>> + vb->page_reporting_conf.report = NULL;
    >>>> + vb->page_reporting_conf.max_pages = 0;
    >>>> + return;
    >>>> + }
    >>>> +}
    >>>> +
    >>>> static void set_page_pfns(struct virtio_balloon *vb,
    >>>> __virtio32 pfns[], struct page *page)
    >>>> {
    >>>> @@ -476,6 +524,7 @@ static int init_vqs(struct virtio_balloon *vb)
    >>>> names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
    >>>> names[VIRTIO_BALLOON_VQ_STATS] = NULL;
    >>>> names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
    >>>> + names[VIRTIO_BALLOON_VQ_REPORTING] = NULL;
    >>>>
    >>>> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
    >>>> names[VIRTIO_BALLOON_VQ_STATS] = "stats";
    >>>> @@ -487,11 +536,18 @@ static int init_vqs(struct virtio_balloon *vb)
    >>>> callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
    >>>> }
    >>>>
    >>>> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) {
    >>>> + names[VIRTIO_BALLOON_VQ_REPORTING] = "reporting_vq";
    >>>> + callbacks[VIRTIO_BALLOON_VQ_REPORTING] = balloon_ack;
    >>> Do we even want to try to set up the reporting queue if reporting has
    >>> been disabled via module parameter? Might make more sense to not even
    >>> negotiate the feature bit in that case.
    >> True.
    >> I think this should be replaced with something like (page_reporting_flag &&
    >> virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)).
    > Yes.
    >
    > Is page_reporting_flag supposed to be changeable on the fly?


    Yes.

    > The only
    > way to really turn off the feature bit from the driver is to not pass
    > in the feature in the features table; we could provide two different
    > tables depending on the flag if it were static.

    I did have a plan of moving to static keys eventually instead of using module
    parameters for this purpose. :)
    That way I will be able to just control the kernel side of things on the fly
    without changing the balloon-page-reporting framework.
    The objective is to allow the user to enable/disable page tracking on the fly.

    >
    >>>
    >>>> + }
    >>>> err = vb->vdev->config->find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX,
    >>>> vqs, callbacks, names, NULL, NULL);
    >>>> if (err)
    >>>> return err;
    >>>>
    >>>> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING))
    >>>> + vb->reporting_vq = vqs[VIRTIO_BALLOON_VQ_REPORTING];
    >>>> +
    >>>> 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)) {
    >>>> @@ -924,6 +980,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
    >>>> if (err)
    >>>> goto out_del_balloon_wq;
    >>>> }
    >>>> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING) &&
    >>>> + page_reporting_flag)
    >>>> + virtballoon_page_reporting_setup(vb);
    >>> In that case, you'd only need to check for the feature bit here.
    >> Why is that?
    >> I think both the checks should be present here as we need both the conditions to
    >> be true to enable page reporting.
    > Yeah, because we can't clear the feature bit if the flag is not set.


    +1

    >
    >> However, the order should be reversed because of the reason you mentioned earlier.
    >>
    >>>
    >>>> virtio_device_ready(vdev);
    >>>>
    >>>> if (towards_target(vb))
    --
    Thanks
    Nitesh

    \
     
     \ /
      Last update: 2019-08-14 16:02    [W:2.138 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site