* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-29 22:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <20170829233920-mutt-send-email-mst@kernel.org>
On Tue, Aug 29, 2017 at 4:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Aug 29, 2017 at 03:53:08PM -0400, Willem de Bruijn wrote:
>> On Tue, Aug 29, 2017 at 3:42 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Tue, Aug 29, 2017 at 03:35:38PM -0400, Willem de Bruijn wrote:
>> >> By the way, I have had an unrelated patch outstanding for a while
>> >> to have virtio-net support the VIRTIO_CONFIG_S_NEEDS_RESET
>> >> command. Will send that as RFC.
>> >
>> > Oh nice.
>>
>> Great :)
>>
>> > One needs to be careful about locking there which is why
>> > no devices support that yet.
>>
>> I originally wrote it based on the virtnet_reset function introduced
>> for xdp. Calling this from virtnet_config_changed_work is non trivial,
>> as virtnet_freeze_down waits until no config worker is running.
>>
>> Otherwise, I could not find any constraints on when freeze may be
>> called, and it largely follows the same path. I hope I didn't miss anything.
>
> The issue is that on freeze processes are not running so we
> generally know no new packets will arrive (might be wrong
> for bridging, then it's a bug). On device error you must
> prevent new skbs from coming in, etc.
Thanks a lot for the quick review. I had indeed not yet figured out
which invariants freeze can depend on that are not universal. Same
for the virtnet_reset call from the .ndo_xdp. Will need to take a much
better look at that.
^ permalink raw reply
* Re: [PATCH RFC 2/2] virtio_net: enable probing for NEEDS_RESET support
From: Willem de Bruijn @ 2017-08-29 22:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Network Development, virtio-dev, Willem de Bruijn, virtualization
In-Reply-To: <20170830000452-mutt-send-email-mst@kernel.org>
On Tue, Aug 29, 2017 at 5:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Aug 29, 2017 at 05:02:29PM -0400, Willem de Bruijn wrote:
>> + virtio-dev
>>
>> On Tue, Aug 29, 2017 at 4:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Tue, Aug 29, 2017 at 04:27:41PM -0400, Willem de Bruijn wrote:
>> >> On Tue, Aug 29, 2017 at 4:16 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> > On Tue, Aug 29, 2017 at 04:07:59PM -0400, Willem de Bruijn wrote:
>> >> >> From: Willem de Bruijn <willemb@google.com>
>> >> >>
>> >> >> Implement a mechanism to signal that a virtio device implements the
>> >> >> VIRTIO_CONFIG_S_NEEDS_RESET command.
>> >> >>
>> >> >> Testing for VIRTIO_CONFIG_S_NEEDS_RESET support by issuing the request
>> >> >> and verifying the reset state would require an expensive state change.
>> >> >>
>> >> >> To avoid that, add a status bit to the feature register used during
>> >> >> feature negotiation between host and guest.
>> >> >>
>> >> >> Set the bit for virtio-net, which supports the feature.
>> >> >>
>> >> >> Signed-off-by: Willem de Bruijn <willemb@google.com>
>> >> >
>> >> > All virtio 1 devices have the reset feature
>> >>
>> >> I don't quite follow. No device drivers implement this request currently?
>> >
>> > Depends. Spec 1.0 describes the bit and that driver can respond
>> > by reseting the device. You seem to do something else
>> > in this patchset, but as designed in 1.0 it does not seem to need
>> > a feature bit.
>>
>> I see. So support is designed to be best effort?
>>
>> The feature bit is only needed if driver support is optional.
>>
>> The ack response is necessary if the device acts differently
>> depending on whether the reset happened. The device has
>> to reset its local state, too, so I think that this is needed.
>
> That reset should only happen when guest driver resets the device.
> And spec already has a mechanism for that anyway.
And the device can read the ring state to see whether it has reset,
perhaps.
>>
>> >> > so maybe guest does
>> >> > not need this flag. Does device need it? Does device really
>> >> > care that guest can't recover?
>> >>
>> >> If all device drivers support it, then the flag is not needed.
>> >>
>> >> But as long as legacy device drivers can exist that do not support
>> >> this feature, it has to have a way of differentiating between the two.
>> >
>> > Why? Device won't set this unless it's in a bad state. In that case,
>> > setting the bit is harmless even if drivers ignore it.
>>
>> The goal is for the device to be able to rely on the driver reset to get
>> to a good state even if it gets it into a bad state.
>>
>> That allows it to implement optimizations that could get it into that bad
>> state.
>
> I see. I don't think this is what the need reset was designed for.
>
> We can extend it to cover this case but let's add a bit more
> documentation then.
Okay.
> And in particular won't it be better if we could just reset one ring,
> and not the whole device state? This might be nicer so flows on other
> rings aren't disrupted.
Indeed. But that would require a different request, then? It also
depends on the use case. A full device reset is a big hammer,
but if used only to get out of rare edge cases, it is good enough.
>>
>> In particular, in the edge case where the device performs backpressure,
>> takes the descriptor out of the avail ring and does not immediately post
>> it to the used ring.
>>
>> A reset will make the guest free all delayed packets and treat any
>> unsent and unacknowledged as network drops. This allows the
>> device to indeed drop long delayed packets when they eventually
>> surface (e.g., leave a qdisc queue).
>
> In this particular case, won't it be easier for device to just
> report all packets as used, without involving the guest?
When the device can just iterate over the outstanding packet, yeah,
that's actually simpler.
>> This is of course not safe with
>> zerocopy packets.
>
>
> I wonder if we can teach kernel to drop zero copy packets too.
Your point about changing frags[] underneath a cloned skb really does
make that hard. We might be able to mitigate individual specific cases
of high latency, such as TC queue occupancy.
>
> --
> MST
^ permalink raw reply
* Re: [PATCH RFC 2/2] virtio_net: enable probing for NEEDS_RESET support
From: Michael S. Tsirkin @ 2017-08-29 21:13 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Network Development, virtio-dev, Willem de Bruijn, virtualization
In-Reply-To: <CAF=yD-+gPOYSgq8ckJ4wt3TPQYEnnB36g=Q5G4=yfoesGc=4hA@mail.gmail.com>
On Tue, Aug 29, 2017 at 05:02:29PM -0400, Willem de Bruijn wrote:
> + virtio-dev
>
> On Tue, Aug 29, 2017 at 4:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Aug 29, 2017 at 04:27:41PM -0400, Willem de Bruijn wrote:
> >> On Tue, Aug 29, 2017 at 4:16 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Tue, Aug 29, 2017 at 04:07:59PM -0400, Willem de Bruijn wrote:
> >> >> From: Willem de Bruijn <willemb@google.com>
> >> >>
> >> >> Implement a mechanism to signal that a virtio device implements the
> >> >> VIRTIO_CONFIG_S_NEEDS_RESET command.
> >> >>
> >> >> Testing for VIRTIO_CONFIG_S_NEEDS_RESET support by issuing the request
> >> >> and verifying the reset state would require an expensive state change.
> >> >>
> >> >> To avoid that, add a status bit to the feature register used during
> >> >> feature negotiation between host and guest.
> >> >>
> >> >> Set the bit for virtio-net, which supports the feature.
> >> >>
> >> >> Signed-off-by: Willem de Bruijn <willemb@google.com>
> >> >
> >> > All virtio 1 devices have the reset feature
> >>
> >> I don't quite follow. No device drivers implement this request currently?
> >
> > Depends. Spec 1.0 describes the bit and that driver can respond
> > by reseting the device. You seem to do something else
> > in this patchset, but as designed in 1.0 it does not seem to need
> > a feature bit.
>
> I see. So support is designed to be best effort?
>
> The feature bit is only needed if driver support is optional.
>
> The ack response is necessary if the device acts differently
> depending on whether the reset happened. The device has
> to reset its local state, too, so I think that this is needed.
That reset should only happen when guest driver resets the device.
And spec already has a mechanism for that anyway.
>
> >> > so maybe guest does
> >> > not need this flag. Does device need it? Does device really
> >> > care that guest can't recover?
> >>
> >> If all device drivers support it, then the flag is not needed.
> >>
> >> But as long as legacy device drivers can exist that do not support
> >> this feature, it has to have a way of differentiating between the two.
> >
> > Why? Device won't set this unless it's in a bad state. In that case,
> > setting the bit is harmless even if drivers ignore it.
>
> The goal is for the device to be able to rely on the driver reset to get
> to a good state even if it gets it into a bad state.
>
> That allows it to implement optimizations that could get it into that bad
> state.
I see. I don't think this is what the need reset was designed for.
We can extend it to cover this case but let's add a bit more
documentation then.
And in particular won't it be better if we could just reset one ring,
and not the whole device state? This might be nicer so flows on other
rings aren't disrupted.
>
> In particular, in the edge case where the device performs backpressure,
> takes the descriptor out of the avail ring and does not immediately post
> it to the used ring.
>
> A reset will make the guest free all delayed packets and treat any
> unsent and unacknowledged as network drops. This allows the
> device to indeed drop long delayed packets when they eventually
> surface (e.g., leave a qdisc queue).
In this particular case, won't it be easier for device to just
report all packets as used, without involving the guest?
> This is of course not safe with
> zerocopy packets.
I wonder if we can teach kernel to drop zero copy packets too.
--
MST
^ permalink raw reply
* Re: [PATCH RFC 2/2] virtio_net: enable probing for NEEDS_RESET support
From: Willem de Bruijn @ 2017-08-29 21:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Network Development, virtio-dev, Willem de Bruijn, virtualization
In-Reply-To: <20170829233525-mutt-send-email-mst@kernel.org>
+ virtio-dev
On Tue, Aug 29, 2017 at 4:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Aug 29, 2017 at 04:27:41PM -0400, Willem de Bruijn wrote:
>> On Tue, Aug 29, 2017 at 4:16 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Tue, Aug 29, 2017 at 04:07:59PM -0400, Willem de Bruijn wrote:
>> >> From: Willem de Bruijn <willemb@google.com>
>> >>
>> >> Implement a mechanism to signal that a virtio device implements the
>> >> VIRTIO_CONFIG_S_NEEDS_RESET command.
>> >>
>> >> Testing for VIRTIO_CONFIG_S_NEEDS_RESET support by issuing the request
>> >> and verifying the reset state would require an expensive state change.
>> >>
>> >> To avoid that, add a status bit to the feature register used during
>> >> feature negotiation between host and guest.
>> >>
>> >> Set the bit for virtio-net, which supports the feature.
>> >>
>> >> Signed-off-by: Willem de Bruijn <willemb@google.com>
>> >
>> > All virtio 1 devices have the reset feature
>>
>> I don't quite follow. No device drivers implement this request currently?
>
> Depends. Spec 1.0 describes the bit and that driver can respond
> by reseting the device. You seem to do something else
> in this patchset, but as designed in 1.0 it does not seem to need
> a feature bit.
I see. So support is designed to be best effort?
The feature bit is only needed if driver support is optional.
The ack response is necessary if the device acts differently
depending on whether the reset happened. The device has
to reset its local state, too, so I think that this is needed.
>> > so maybe guest does
>> > not need this flag. Does device need it? Does device really
>> > care that guest can't recover?
>>
>> If all device drivers support it, then the flag is not needed.
>>
>> But as long as legacy device drivers can exist that do not support
>> this feature, it has to have a way of differentiating between the two.
>
> Why? Device won't set this unless it's in a bad state. In that case,
> setting the bit is harmless even if drivers ignore it.
The goal is for the device to be able to rely on the driver reset to get
to a good state even if it gets it into a bad state.
That allows it to implement optimizations that could get it into that bad
state.
In particular, in the edge case where the device performs backpressure,
takes the descriptor out of the avail ring and does not immediately post
it to the used ring.
A reset will make the guest free all delayed packets and treat any
unsent and unacknowledged as network drops. This allows the
device to indeed drop long delayed packets when they eventually
surface (e.g., leave a qdisc queue). This is of course not safe with
zerocopy packets.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Michael S. Tsirkin @ 2017-08-29 20:40 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-L5MG5RCZzT8EZ0coFiuQLp-2UQN0zzQefVnEd7rMQbyw@mail.gmail.com>
On Tue, Aug 29, 2017 at 03:53:08PM -0400, Willem de Bruijn wrote:
> On Tue, Aug 29, 2017 at 3:42 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Aug 29, 2017 at 03:35:38PM -0400, Willem de Bruijn wrote:
> >> By the way, I have had an unrelated patch outstanding for a while
> >> to have virtio-net support the VIRTIO_CONFIG_S_NEEDS_RESET
> >> command. Will send that as RFC.
> >
> > Oh nice.
>
> Great :)
>
> > One needs to be careful about locking there which is why
> > no devices support that yet.
>
> I originally wrote it based on the virtnet_reset function introduced
> for xdp. Calling this from virtnet_config_changed_work is non trivial,
> as virtnet_freeze_down waits until no config worker is running.
>
> Otherwise, I could not find any constraints on when freeze may be
> called, and it largely follows the same path. I hope I didn't miss anything.
The issue is that on freeze processes are not running so we
generally know no new packets will arrive (might be wrong
for bridging, then it's a bug). On device error you must
prevent new skbs from coming in, etc.
--
MST
^ permalink raw reply
* Re: [PATCH RFC 2/2] virtio_net: enable probing for NEEDS_RESET support
From: Michael S. Tsirkin @ 2017-08-29 20:38 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Willem de Bruijn, virtualization
In-Reply-To: <CAF=yD-L95x1EPKFeiPiZ2hbTzwYmDaG9O54+0tUW_4opuRKDMw@mail.gmail.com>
On Tue, Aug 29, 2017 at 04:27:41PM -0400, Willem de Bruijn wrote:
> On Tue, Aug 29, 2017 at 4:16 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Aug 29, 2017 at 04:07:59PM -0400, Willem de Bruijn wrote:
> >> From: Willem de Bruijn <willemb@google.com>
> >>
> >> Implement a mechanism to signal that a virtio device implements the
> >> VIRTIO_CONFIG_S_NEEDS_RESET command.
> >>
> >> Testing for VIRTIO_CONFIG_S_NEEDS_RESET support by issuing the request
> >> and verifying the reset state would require an expensive state change.
> >>
> >> To avoid that, add a status bit to the feature register used during
> >> feature negotiation between host and guest.
> >>
> >> Set the bit for virtio-net, which supports the feature.
> >>
> >> Signed-off-by: Willem de Bruijn <willemb@google.com>
> >
> > All virtio 1 devices have the reset feature
>
> I don't quite follow. No device drivers implement this request currently?
Depends. Spec 1.0 describes the bit and that driver can respond
by reseting the device. You seem to do something else
in this patchset, but as designed in 1.0 it does not seem to need
a feature bit.
> > so maybe guest does
> > not need this flag. Does device need it? Does device really
> > care that guest can't recover?
>
> If all device drivers support it, then the flag is not needed.
>
> But as long as legacy device drivers can exist that do not support
> this feature, it has to have a way of differentiating between the two.
Why? Device won't set this unless it's in a bad state. In that case,
setting the bit is harmless even if drivers ignore it.
--
MST
^ permalink raw reply
* Re: [PATCH RFC 2/2] virtio_net: enable probing for NEEDS_RESET support
From: Willem de Bruijn @ 2017-08-29 20:27 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Network Development, Willem de Bruijn, virtualization
In-Reply-To: <20170829231606-mutt-send-email-mst@kernel.org>
On Tue, Aug 29, 2017 at 4:16 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Aug 29, 2017 at 04:07:59PM -0400, Willem de Bruijn wrote:
>> From: Willem de Bruijn <willemb@google.com>
>>
>> Implement a mechanism to signal that a virtio device implements the
>> VIRTIO_CONFIG_S_NEEDS_RESET command.
>>
>> Testing for VIRTIO_CONFIG_S_NEEDS_RESET support by issuing the request
>> and verifying the reset state would require an expensive state change.
>>
>> To avoid that, add a status bit to the feature register used during
>> feature negotiation between host and guest.
>>
>> Set the bit for virtio-net, which supports the feature.
>>
>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> All virtio 1 devices have the reset feature
I don't quite follow. No device drivers implement this request currently?
> so maybe guest does
> not need this flag. Does device need it? Does device really
> care that guest can't recover?
If all device drivers support it, then the flag is not needed.
But as long as legacy device drivers can exist that do not support
this feature, it has to have a way of differentiating between the two.
^ permalink raw reply
* Re: [PATCH RFC 1/2] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Michael S. Tsirkin @ 2017-08-29 20:20 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev, Willem de Bruijn, virtualization
In-Reply-To: <20170829200759.13975-2-willemdebruijn.kernel@gmail.com>
On Tue, Aug 29, 2017 at 04:07:58PM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Implement the reset communication request defined in the VIRTIO 1.0
> specification and introduces in Linux in commit c00bbcf862896 ("virtio:
> add VIRTIO_CONFIG_S_NEEDS_RESET device status bit").
>
> Since that patch, the virtio-net driver has added a virtnet_reset
> function that implements the requested behavior through calls to the
> power management freeze and restore functions.
>
> That function has recently been reverted when its sole caller was
> updated. Bring it back and listen for the request from the host on
> the config channel.
>
> Implement the feature analogous to other config requests. In
> particular, acknowledge the request in the same manner as the
> NET_S_ANNOUNCE link announce request, by responding with a new
> VIRTIO_NET_CTRL_${TYPE} command. On reception, the host must check
> the config status register for success or failure.
Pls make it clearer why do you need these interface extensions.
> The existing freeze handler verifies that no config changes are
> running concurrently. Elide this check for reset. The request is
> always handled from the config workqueue. No other config requests
> can be active or scheduled concurrently on vi->config.
You need to prevent packet TX from being in progress.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> drivers/net/virtio_net.c | 69 +++++++++++++++++++++++++++++++++++------
> include/uapi/linux/virtio_net.h | 4 +++
virtio dev or another virtio TC list must be Cc'd on any proposed API changes.
> 2 files changed, 64 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 52ae78ca3d38..5e349226f7c1 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1458,12 +1458,11 @@ static void virtnet_netpoll(struct net_device *dev)
> }
> #endif
>
> -static void virtnet_ack_link_announce(struct virtnet_info *vi)
> +static void virtnet_ack(struct virtnet_info *vi, u8 class, u8 cmd)
> {
> rtnl_lock();
> - if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> - VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL))
> - dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
> + if (!virtnet_send_command(vi, class, cmd, NULL))
> + dev_warn(&vi->dev->dev, "Failed to ack %u.%u\n", class, cmd);
> rtnl_unlock();
> }
>
> @@ -1857,13 +1856,16 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
> .set_link_ksettings = virtnet_set_link_ksettings,
> };
>
> -static void virtnet_freeze_down(struct virtio_device *vdev)
> +static void virtnet_freeze_down(struct virtio_device *vdev, bool in_reset)
> {
> struct virtnet_info *vi = vdev->priv;
> int i;
>
> - /* Make sure no work handler is accessing the device */
> - flush_work(&vi->config_work);
> + /* Make sure no work handler is accessing the device,
> + * unless this call is made from the reset work handler itself.
> + */
> + if (!in_reset)
> + flush_work(&vi->config_work);
>
> netif_device_detach(vi->dev);
> netif_tx_disable(vi->dev);
> @@ -1878,6 +1880,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
> }
>
> static int init_vqs(struct virtnet_info *vi);
> +static void remove_vq_common(struct virtnet_info *vi);
>
> static int virtnet_restore_up(struct virtio_device *vdev)
> {
> @@ -1906,6 +1909,45 @@ static int virtnet_restore_up(struct virtio_device *vdev)
> return err;
> }
>
> +static int virtnet_reset(struct virtnet_info *vi)
> +{
> + struct virtio_device *dev = vi->vdev;
> + bool failed;
> + int ret;
> +
> + virtio_config_disable(dev);
> + failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> + virtnet_freeze_down(dev, true);
> + remove_vq_common(vi);
> +
> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> +
> + /* Restore the failed status (see virtio_device_restore). */
> + if (failed)
> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> +
> + ret = virtio_finalize_features(dev);
> + if (ret)
> + goto err;
> +
> + ret = virtnet_restore_up(dev);
> + if (ret)
> + goto err;
> +
> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
> + if (ret)
> + goto err;
> +
> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> + virtio_config_enable(dev);
> + return 0;
> +
> +err:
> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> + return ret;
> +}
> +
> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> {
> struct scatterlist sg;
> @@ -2085,7 +2127,16 @@ static void virtnet_config_changed_work(struct work_struct *work)
>
> if (v & VIRTIO_NET_S_ANNOUNCE) {
> netdev_notify_peers(vi->dev);
> - virtnet_ack_link_announce(vi);
> + virtnet_ack(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> + VIRTIO_NET_CTRL_ANNOUNCE_ACK);
> + }
> +
> + if (vi->vdev->config->get_status(vi->vdev) &
> + VIRTIO_CONFIG_S_NEEDS_RESET) {
> + virtnet_reset(vi);
> + virtnet_ack(vi, VIRTIO_NET_CTRL_RESET,
> + VIRTIO_NET_CTRL_RESET_ACK);
> +
> }
>
> /* Ignore unknown (future) status bits */
> @@ -2708,7 +2759,7 @@ static __maybe_unused int virtnet_freeze(struct virtio_device *vdev)
> struct virtnet_info *vi = vdev->priv;
>
> virtnet_cpu_notif_remove(vi);
> - virtnet_freeze_down(vdev);
> + virtnet_freeze_down(vdev, false);
> remove_vq_common(vi);
>
> return 0;
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index fc353b518288..188fdc528f13 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -245,4 +245,8 @@ struct virtio_net_ctrl_mq {
> #define VIRTIO_NET_CTRL_GUEST_OFFLOADS 5
> #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET 0
>
> +/* Signal that the driver received and executed the reset command. */
> +#define VIRTIO_NET_CTRL_RESET 6
> +#define VIRTIO_NET_CTRL_RESET_ACK 0
> +
> #endif /* _UAPI_LINUX_VIRTIO_NET_H */
> --
> 2.14.1.342.g6490525c54-goog
^ permalink raw reply
* Re: [PATCH RFC 2/2] virtio_net: enable probing for NEEDS_RESET support
From: Michael S. Tsirkin @ 2017-08-29 20:16 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev, Willem de Bruijn, virtualization
In-Reply-To: <20170829200759.13975-3-willemdebruijn.kernel@gmail.com>
On Tue, Aug 29, 2017 at 04:07:59PM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Implement a mechanism to signal that a virtio device implements the
> VIRTIO_CONFIG_S_NEEDS_RESET command.
>
> Testing for VIRTIO_CONFIG_S_NEEDS_RESET support by issuing the request
> and verifying the reset state would require an expensive state change.
>
> To avoid that, add a status bit to the feature register used during
> feature negotiation between host and guest.
>
> Set the bit for virtio-net, which supports the feature.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
All virtio 1 devices have the reset feature so maybe guest does
not need this flag. Does device need it? Does device really
care that guest can't recover?
> ---
> drivers/net/virtio_net.c | 2 ++
> include/uapi/linux/virtio_config.h | 3 +++
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 5e349226f7c1..15483a982106 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2801,12 +2801,14 @@ static struct virtio_device_id id_table[] = {
>
> static unsigned int features[] = {
> VIRTNET_FEATURES,
> + VIRTIO_F_CAN_RESET,
> };
>
> static unsigned int features_legacy[] = {
> VIRTNET_FEATURES,
> VIRTIO_NET_F_GSO,
> VIRTIO_F_ANY_LAYOUT,
> + VIRTIO_F_CAN_RESET,
> };
>
> static struct virtio_driver virtio_net_driver = {
> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index 308e2096291f..726be44a04d3 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -56,6 +56,9 @@
> * suppressed them? */
> #define VIRTIO_F_NOTIFY_ON_EMPTY 24
>
> +/* Driver supports the VIRTIO_CONFIG_S_NEEDS_RESET command */
> +#define VIRTIO_F_CAN_RESET 25
> +
> /* Can the device handle any descriptor layout? */
> #define VIRTIO_F_ANY_LAYOUT 27
> #endif /* VIRTIO_CONFIG_NO_LEGACY */
> --
> 2.14.1.342.g6490525c54-goog
^ permalink raw reply
* [PATCH RFC 2/2] virtio_net: enable probing for NEEDS_RESET support
From: Willem de Bruijn @ 2017-08-29 20:07 UTC (permalink / raw)
To: netdev; +Cc: virtualization, Willem de Bruijn, mst
In-Reply-To: <20170829200759.13975-1-willemdebruijn.kernel@gmail.com>
From: Willem de Bruijn <willemb@google.com>
Implement a mechanism to signal that a virtio device implements the
VIRTIO_CONFIG_S_NEEDS_RESET command.
Testing for VIRTIO_CONFIG_S_NEEDS_RESET support by issuing the request
and verifying the reset state would require an expensive state change.
To avoid that, add a status bit to the feature register used during
feature negotiation between host and guest.
Set the bit for virtio-net, which supports the feature.
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
drivers/net/virtio_net.c | 2 ++
include/uapi/linux/virtio_config.h | 3 +++
2 files changed, 5 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5e349226f7c1..15483a982106 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2801,12 +2801,14 @@ static struct virtio_device_id id_table[] = {
static unsigned int features[] = {
VIRTNET_FEATURES,
+ VIRTIO_F_CAN_RESET,
};
static unsigned int features_legacy[] = {
VIRTNET_FEATURES,
VIRTIO_NET_F_GSO,
VIRTIO_F_ANY_LAYOUT,
+ VIRTIO_F_CAN_RESET,
};
static struct virtio_driver virtio_net_driver = {
diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index 308e2096291f..726be44a04d3 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -56,6 +56,9 @@
* suppressed them? */
#define VIRTIO_F_NOTIFY_ON_EMPTY 24
+/* Driver supports the VIRTIO_CONFIG_S_NEEDS_RESET command */
+#define VIRTIO_F_CAN_RESET 25
+
/* Can the device handle any descriptor layout? */
#define VIRTIO_F_ANY_LAYOUT 27
#endif /* VIRTIO_CONFIG_NO_LEGACY */
--
2.14.1.342.g6490525c54-goog
^ permalink raw reply related
* [PATCH RFC 1/2] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Willem de Bruijn @ 2017-08-29 20:07 UTC (permalink / raw)
To: netdev; +Cc: virtualization, Willem de Bruijn, mst
In-Reply-To: <20170829200759.13975-1-willemdebruijn.kernel@gmail.com>
From: Willem de Bruijn <willemb@google.com>
Implement the reset communication request defined in the VIRTIO 1.0
specification and introduces in Linux in commit c00bbcf862896 ("virtio:
add VIRTIO_CONFIG_S_NEEDS_RESET device status bit").
Since that patch, the virtio-net driver has added a virtnet_reset
function that implements the requested behavior through calls to the
power management freeze and restore functions.
That function has recently been reverted when its sole caller was
updated. Bring it back and listen for the request from the host on
the config channel.
Implement the feature analogous to other config requests. In
particular, acknowledge the request in the same manner as the
NET_S_ANNOUNCE link announce request, by responding with a new
VIRTIO_NET_CTRL_${TYPE} command. On reception, the host must check
the config status register for success or failure.
The existing freeze handler verifies that no config changes are
running concurrently. Elide this check for reset. The request is
always handled from the config workqueue. No other config requests
can be active or scheduled concurrently on vi->config.
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
drivers/net/virtio_net.c | 69 +++++++++++++++++++++++++++++++++++------
include/uapi/linux/virtio_net.h | 4 +++
2 files changed, 64 insertions(+), 9 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 52ae78ca3d38..5e349226f7c1 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1458,12 +1458,11 @@ static void virtnet_netpoll(struct net_device *dev)
}
#endif
-static void virtnet_ack_link_announce(struct virtnet_info *vi)
+static void virtnet_ack(struct virtnet_info *vi, u8 class, u8 cmd)
{
rtnl_lock();
- if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
- VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL))
- dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
+ if (!virtnet_send_command(vi, class, cmd, NULL))
+ dev_warn(&vi->dev->dev, "Failed to ack %u.%u\n", class, cmd);
rtnl_unlock();
}
@@ -1857,13 +1856,16 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
.set_link_ksettings = virtnet_set_link_ksettings,
};
-static void virtnet_freeze_down(struct virtio_device *vdev)
+static void virtnet_freeze_down(struct virtio_device *vdev, bool in_reset)
{
struct virtnet_info *vi = vdev->priv;
int i;
- /* Make sure no work handler is accessing the device */
- flush_work(&vi->config_work);
+ /* Make sure no work handler is accessing the device,
+ * unless this call is made from the reset work handler itself.
+ */
+ if (!in_reset)
+ flush_work(&vi->config_work);
netif_device_detach(vi->dev);
netif_tx_disable(vi->dev);
@@ -1878,6 +1880,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
}
static int init_vqs(struct virtnet_info *vi);
+static void remove_vq_common(struct virtnet_info *vi);
static int virtnet_restore_up(struct virtio_device *vdev)
{
@@ -1906,6 +1909,45 @@ static int virtnet_restore_up(struct virtio_device *vdev)
return err;
}
+static int virtnet_reset(struct virtnet_info *vi)
+{
+ struct virtio_device *dev = vi->vdev;
+ bool failed;
+ int ret;
+
+ virtio_config_disable(dev);
+ failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
+ virtnet_freeze_down(dev, true);
+ remove_vq_common(vi);
+
+ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
+ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+
+ /* Restore the failed status (see virtio_device_restore). */
+ if (failed)
+ virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
+
+ ret = virtio_finalize_features(dev);
+ if (ret)
+ goto err;
+
+ ret = virtnet_restore_up(dev);
+ if (ret)
+ goto err;
+
+ ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
+ if (ret)
+ goto err;
+
+ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+ virtio_config_enable(dev);
+ return 0;
+
+err:
+ virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
+ return ret;
+}
+
static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
{
struct scatterlist sg;
@@ -2085,7 +2127,16 @@ static void virtnet_config_changed_work(struct work_struct *work)
if (v & VIRTIO_NET_S_ANNOUNCE) {
netdev_notify_peers(vi->dev);
- virtnet_ack_link_announce(vi);
+ virtnet_ack(vi, VIRTIO_NET_CTRL_ANNOUNCE,
+ VIRTIO_NET_CTRL_ANNOUNCE_ACK);
+ }
+
+ if (vi->vdev->config->get_status(vi->vdev) &
+ VIRTIO_CONFIG_S_NEEDS_RESET) {
+ virtnet_reset(vi);
+ virtnet_ack(vi, VIRTIO_NET_CTRL_RESET,
+ VIRTIO_NET_CTRL_RESET_ACK);
+
}
/* Ignore unknown (future) status bits */
@@ -2708,7 +2759,7 @@ static __maybe_unused int virtnet_freeze(struct virtio_device *vdev)
struct virtnet_info *vi = vdev->priv;
virtnet_cpu_notif_remove(vi);
- virtnet_freeze_down(vdev);
+ virtnet_freeze_down(vdev, false);
remove_vq_common(vi);
return 0;
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index fc353b518288..188fdc528f13 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -245,4 +245,8 @@ struct virtio_net_ctrl_mq {
#define VIRTIO_NET_CTRL_GUEST_OFFLOADS 5
#define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET 0
+/* Signal that the driver received and executed the reset command. */
+#define VIRTIO_NET_CTRL_RESET 6
+#define VIRTIO_NET_CTRL_RESET_ACK 0
+
#endif /* _UAPI_LINUX_VIRTIO_NET_H */
--
2.14.1.342.g6490525c54-goog
^ permalink raw reply related
* [PATCH RFC 0/2] virtio-net: implement reset request
From: Willem de Bruijn @ 2017-08-29 20:07 UTC (permalink / raw)
To: netdev; +Cc: virtualization, Willem de Bruijn, mst
From: Willem de Bruijn <willemb@google.com>
Implement VIRTIO_CONFIG_S_NEEDS_RESET (patch 1) and
add a feature bit to signal support (patch 2)
Further details in the individual patches.
Willem de Bruijn (2):
virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
virtio_net: enable probing for NEEDS_RESET support
drivers/net/virtio_net.c | 71 +++++++++++++++++++++++++++++++++-----
include/uapi/linux/virtio_config.h | 3 ++
include/uapi/linux/virtio_net.h | 4 +++
3 files changed, 69 insertions(+), 9 deletions(-)
--
2.14.1.342.g6490525c54-goog
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-29 19:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <20170829224136-mutt-send-email-mst@kernel.org>
On Tue, Aug 29, 2017 at 3:42 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Aug 29, 2017 at 03:35:38PM -0400, Willem de Bruijn wrote:
>> By the way, I have had an unrelated patch outstanding for a while
>> to have virtio-net support the VIRTIO_CONFIG_S_NEEDS_RESET
>> command. Will send that as RFC.
>
> Oh nice.
Great :)
> One needs to be careful about locking there which is why
> no devices support that yet.
I originally wrote it based on the virtnet_reset function introduced
for xdp. Calling this from virtnet_config_changed_work is non trivial,
as virtnet_freeze_down waits until no config worker is running.
Otherwise, I could not find any constraints on when freeze may be
called, and it largely follows the same path. I hope I didn't miss anything.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Michael S. Tsirkin @ 2017-08-29 19:42 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-+Wk9sc9dXMUq1+x_hh=3ThTXa6BnZkygP3tgVpjbp93g@mail.gmail.com>
On Tue, Aug 29, 2017 at 03:35:38PM -0400, Willem de Bruijn wrote:
> By the way, I have had an unrelated patch outstanding for a while
> to have virtio-net support the VIRTIO_CONFIG_S_NEEDS_RESET
> command. Will send that as RFC.
Oh nice. One needs to be careful about locking there which is why
no devices support that yet.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-29 19:35 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-Lre0NATUVH8p0zS=G5CiY=WfKSefLmOZw940jZFgECgQ@mail.gmail.com>
On Fri, Aug 25, 2017 at 9:03 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, Aug 25, 2017 at 7:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Fri, Aug 25, 2017 at 06:44:36PM -0400, Willem de Bruijn wrote:
>>> >> >> > We don't enable network watchdog on virtio but we could and maybe
>>> >> >> > should.
>>> >> >>
>>> >> >> Can you elaborate?
>>> >> >
>>> >> > The issue is that holding onto buffers for very long times makes guests
>>> >> > think they are stuck. This is funamentally because from guest point of
>>> >> > view this is a NIC, so it is supposed to transmit things out in
>>> >> > a timely manner. If host backs the virtual NIC by something that is not
>>> >> > a NIC, with traffic shaping etc introducing unbounded latencies,
>>> >> > guest will be confused.
>>> >>
>>> >> That assumes that guests are fragile in this regard. A linux guest
>>> >> does not make such assumptions.
>>> >
>>> > Yes it does. Examples above:
>>> > > > - a single slow flow can occupy the whole ring, you will not
>>> > > > be able to make any new buffers available for the fast flow
>>>
>>> Oh, right. Though those are due to vring_desc pool exhaustion
>>> rather than an upper bound on latency of any single packet.
>>>
>>> Limiting the number of zerocopy packets in flight to some fraction
>>> of the ring ensures that fast flows can always grab a slot.
>>> Running
>>> out of ubuf_info slots reverts to copy, so indirectly does this. But
>>> I read it correclty the zerocopy pool may be equal to or larger than
>>> the descriptor pool. Should we refine the zcopy_used test
>>>
>>> (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx
>>>
>>> to also return false if the number of outstanding ubuf_info is greater
>>> than, say, vq->num >> 1?
>>
>>
>> We'll need to think about where to put the threshold, but I think it's
>> a good idea.
>>
>> Maybe even a fixed number, e.g. max(vq->num >> 1, X) to limit host
>> resources.
>>
>> In a sense it still means once you run out of slots zcopt gets disabled possibly permanently.
>>
>> Need to experiment with some numbers.
>
> I can take a stab with two flows, one delayed in a deep host qdisc
> queue. See how this change affects the other flow and also how
> sensitive that is to the chosen threshold value.
Incomplete results at this stage, but I do see this correlation between
flows. It occurs even while not running out of zerocopy descriptors,
which I cannot yet explain.
Running two threads in a guest, each with a udp socket, each
sending up to 100 datagrams, or until EAGAIN, every msec.
Sender A sends 1B datagrams.
Sender B sends VHOST_GOODCOPY_LEN, which is enough
to trigger zcopy_used in vhost net.
A local receive process on the host receives both flows. To avoid
a deep copy when looping the packet onto the receive path,
changed skb_orphan_frags_rx to always return false (gross hack).
The flow with the larger packets is redirected through netem on ifb0:
modprobe ifb
ip link set dev ifb0 up
tc qdisc add dev ifb0 root netem limit $LIMIT rate 1MBit
tc qdisc add dev tap0 ingress
tc filter add dev tap0 parent ffff: protocol ip \
u32 match ip dport 8000 0xffff \
action mirred egress redirect dev ifb0
For 10 second run, packet count with various ifb0 queue lengths $LIMIT:
no filter
rx.A: ~840,000
rx.B: ~840,000
limit 1
rx.A: ~500,000
rx.B: ~3100
ifb0: 3273 sent, 371141 dropped
limit 100
rx.A: ~9000
rx.B: ~4200
ifb0: 4630 sent, 1491 dropped
limit 1000
rx.A: ~6800
rx.B: ~4200
ifb0: 4651 sent, 0 dropped
Sender B is always correctly rate limited to 1 MBps or less. With a
short queue, it ends up dropping a lot and sending even less.
When a queue builds up for sender B, sender A throughput is strongly
correlated with queue length. With queue length 1, it can send almost
at unthrottled speed. But even at limit 100 its throughput is on the
same order as sender B.
What is surprising to me is that this happens even though the number
of ubuf_info in use at limit 100 is around 100 at all times. In other words,
it does not exhaust the pool.
When forcing zcopy_used to be false for all packets, this effect of
sender A throughput being correlated with sender B does not happen.
no filter
rx.A: ~850,000
rx.B: ~850,000
limit 100
rx.A: ~850,000
rx.B: ~4200
ifb0: 4518 sent, 876182 dropped
Also relevant is that with zerocopy, the sender processes back off
and report the same count as the receiver. Without zerocopy,
both senders send at full speed, even if only 4200 packets from flow
B arrive at the receiver.
This is with the default virtio_net driver, so without napi-tx.
It appears that the zerocopy notifications are pausing the guest.
Will look at that now.
By the way, I have had an unrelated patch outstanding for a while
to have virtio-net support the VIRTIO_CONFIG_S_NEEDS_RESET
command. Will send that as RFC.
^ permalink raw reply
* Re: [RFC PATCH v2 5/7] Documentation: Add three sysctls for smart idle poll
From: Luis R. Rodriguez @ 2017-08-29 17:20 UTC (permalink / raw)
To: Yang Zhang
Cc: Jeremy Fitzhardinge, rkrcmar, kvm, mst, peterz, virtualization,
H. Peter Anvin, Alok Kataria, wanpeng.li, Jessica Yu, Baoquan He,
Jonathan Corbet, x86, Krzysztof Kozlowski, Ingo Molnar, zijun_hu,
Petr Mladek, Kees Cook, Johannes Berg, Rusty Russell,
Chris Wright, Ian Abbott, Josh Poimboeuf, dmatlack, tglx,
Mauro Carvalho Chehab, Juergen Gross, Quan Xu, linux-doc
In-Reply-To: <1504007201-12904-6-git-send-email-yang.zhang.wz@gmail.com>
On Tue, Aug 29, 2017 at 11:46:39AM +0000, Yang Zhang wrote:
> To reduce the cost of poll, we introduce three sysctl to control the
> poll time.
This commit does not describe in any way the fact that these knobs are
all for and only for PARAVIRT.
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index bac23c1..67447b8 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -63,6 +63,9 @@ show up in /proc/sys/kernel:
> - perf_event_max_stack
> - perf_event_max_contexts_per_stack
> - pid_max
> +- poll_grow [ X86 only ]
> +- poll_shrink [ X86 only ]
> +- poll_threshold_ns [ X86 only ]
How about paravirt_ prefix?
> - powersave-nap [ PPC only ]
> - printk
> - printk_delay
> @@ -702,6 +705,28 @@ kernel tries to allocate a number starting from this one.
>
> ==============================================================
>
> +poll_grow: (X86 only)
> +
> +This parameter is multiplied in the grow_poll_ns() to increase the poll time.
> +By default, the values is 2.
> +
> +==============================================================
> +poll_shrink: (X86 only)
> +
> +This parameter is divided in the shrink_poll_ns() to reduce the poll time.
> +By default, the values is 2.
These don't say anything about this being related to paravirt.
> +
> +==============================================================
> +poll_threshold_ns: (X86 only)
> +
> +This parameter controls the max poll time before entering real idle path.
> +This parameter is expected to take effect only when running inside a VM.
> +It would make no sense to turn on it in bare mental.
turn it on? Or modify, or use it?
> +By default, the values is 0 means don't poll. It is recommended to change
> +the value to non-zero if running latency-bound workloads inside VM.
> +
> +==============================================================
> +
> powersave-nap: (PPC only)
>
> If set, Linux-PPC will use the 'nap' mode of powersaving,
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index a11b2c2..0b92f8f 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -318,6 +318,10 @@ struct pv_idle_ops pv_idle_ops = {
> .update = paravirt_nop,
> };
>
> +unsigned long poll_threshold_ns;
> +unsigned int poll_shrink = 2;
> +unsigned int poll_grow = 2;
> +
> __visible struct pv_irq_ops pv_irq_ops = {
> .save_fl = __PV_IS_CALLEE_SAVE(native_save_fl),
> .restore_fl = __PV_IS_CALLEE_SAVE(native_restore_fl),
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index bd6d96c..6cb2820 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -462,6 +462,12 @@ extern __scanf(2, 0)
>
> extern bool crash_kexec_post_notifiers;
>
> +#ifdef CONFIG_PARAVIRT
> +extern unsigned long poll_threshold_ns;
> +extern unsigned int poll_shrink;
> +extern unsigned int poll_grow;
> +#endif
What are these if we are on bare metal but support
paravirt? The docs are not clear in any way about it.
> +
> /*
> * panic_cpu is used for synchronizing panic() and crash_kexec() execution. It
> * holds a CPU number which is executing panic() currently. A value of
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 6648fbb..9b86a8f 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1229,6 +1229,29 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
> .extra2 = &one,
> },
> #endif
> +#ifdef CONFIG_PARAVIRT
> + {
> + .procname = "halt_poll_threshold",
> + .data = &poll_threshold_ns,
> + .maxlen = sizeof(unsigned long),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
proc_doulongvec_minmax() may be more appropriate here? What is the range?
Also what user did you see used proc_dointvec() but had unsigned long?
If the test below reveal this is invalid we would need to go fix them
as well.
> + {
> + .procname = "halt_poll_grow",
> + .data = &poll_grow,
> + .maxlen = sizeof(unsigned int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
> + {
> + .procname = "halt_poll_shrink",
> + .data = &poll_shrink,
> + .maxlen = sizeof(unsigned int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
We have now a much more appropriate proc_douintvec() proc_douintvec_minmax().
It seems you want to support only unsigned int for two of these so that would
be more appropriate.
To test things out you can replicate your values using or extending the
test driver lib/test_sysctl.c CONFIG_TEST_SYSCTL=m with your type of
values and then fuzz testing them with arbitrary values to ensure you get
only as input what you really think these values should get.
Once done please extend the script:
tools/testing/selftests/sysctl/sysctl.sh
to cover the different tests you have run, we want tests to be generic.
Luis
^ permalink raw reply
* Re: [RFC PATCH v2 1/7] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Konrad Rzeszutek Wilk @ 2017-08-29 13:55 UTC (permalink / raw)
To: Yang Zhang, xen-devel, jgross, Boris Ostrovsky
Cc: Jeremy Fitzhardinge, rkrcmar, kvm, mst, peterz, Pan Xinhui,
virtualization, H. Peter Anvin, Alok Kataria, wanpeng.li, x86,
Ingo Molnar, Kees Cook, Chris Wright, Andy Lutomirski, dmatlack,
tglx, Quan Xu, linux-doc, linux-kernel, pbonzini,
Kirill A. Shutemov
In-Reply-To: <1504007201-12904-2-git-send-email-yang.zhang.wz@gmail.com>
On Tue, Aug 29, 2017 at 11:46:35AM +0000, Yang Zhang wrote:
> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called in
> idle path which will polling for a while before we enter the real idle
> state.
>
> In virtualization, idle path includes several heavy operations
> includes timer access(LAPIC timer or TSC deadline timer) which will hurt
> performance especially for latency intensive workload like message
> passing task. The cost is mainly come from the vmexit which is a
> hardware context switch between VM and hypervisor. Our solution is to
> poll for a while and do not enter real idle path if we can get the
> schedule event during polling.
>
> Poll may cause the CPU waste so we adopt a smart polling mechanism to
> reduce the useless poll.
>
> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Chris Wright <chrisw@sous-sol.org>
> Cc: Alok Kataria <akataria@vmware.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
Adding xen-devel.
Juergen, we really should replace Jeremy's name with xen-devel or
your name.. Wasn't there an patch by you that took some of the
mainternship over it?
> ---
> arch/x86/include/asm/paravirt.h | 5 +++++
> arch/x86/include/asm/paravirt_types.h | 6 ++++++
> arch/x86/kernel/paravirt.c | 6 ++++++
> 3 files changed, 17 insertions(+)
>
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index 9ccac19..6d46760 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -202,6 +202,11 @@ static inline unsigned long long paravirt_read_pmc(int counter)
>
> #define rdpmcl(counter, val) ((val) = paravirt_read_pmc(counter))
>
> +static inline void paravirt_idle_poll(void)
> +{
> + PVOP_VCALL0(pv_idle_ops.poll);
> +}
> +
> static inline void paravirt_alloc_ldt(struct desc_struct *ldt, unsigned entries)
> {
> PVOP_VCALL2(pv_cpu_ops.alloc_ldt, ldt, entries);
> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
> index 9ffc36b..cf45726 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h
> @@ -324,6 +324,10 @@ struct pv_lock_ops {
> struct paravirt_callee_save vcpu_is_preempted;
> } __no_randomize_layout;
>
> +struct pv_idle_ops {
> + void (*poll)(void);
> +} __no_randomize_layout;
> +
> /* This contains all the paravirt structures: we get a convenient
> * number for each function using the offset which we use to indicate
> * what to patch. */
> @@ -334,6 +338,7 @@ struct paravirt_patch_template {
> struct pv_irq_ops pv_irq_ops;
> struct pv_mmu_ops pv_mmu_ops;
> struct pv_lock_ops pv_lock_ops;
> + struct pv_idle_ops pv_idle_ops;
> } __no_randomize_layout;
>
> extern struct pv_info pv_info;
> @@ -343,6 +348,7 @@ struct paravirt_patch_template {
> extern struct pv_irq_ops pv_irq_ops;
> extern struct pv_mmu_ops pv_mmu_ops;
> extern struct pv_lock_ops pv_lock_ops;
> +extern struct pv_idle_ops pv_idle_ops;
>
> #define PARAVIRT_PATCH(x) \
> (offsetof(struct paravirt_patch_template, x) / sizeof(void *))
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index bc0a849..1b5b247 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -128,6 +128,7 @@ static void *get_call_destination(u8 type)
> #ifdef CONFIG_PARAVIRT_SPINLOCKS
> .pv_lock_ops = pv_lock_ops,
> #endif
> + .pv_idle_ops = pv_idle_ops,
> };
> return *((void **)&tmpl + type);
> }
> @@ -312,6 +313,10 @@ struct pv_time_ops pv_time_ops = {
> .steal_clock = native_steal_clock,
> };
>
> +struct pv_idle_ops pv_idle_ops = {
> + .poll = paravirt_nop,
> +};
> +
> __visible struct pv_irq_ops pv_irq_ops = {
> .save_fl = __PV_IS_CALLEE_SAVE(native_save_fl),
> .restore_fl = __PV_IS_CALLEE_SAVE(native_restore_fl),
> @@ -471,3 +476,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_idle_ops);
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [Qemu-devel] [RFC] Buffers/caches in VirtIO Balloon driver stats
From: Wei Wang @ 2017-08-29 12:01 UTC (permalink / raw)
To: Stefan Hajnoczi, Tomáš Golembiovský
Cc: virtio-dev, Michael S. Tsirkin, qemu-devel, kvm, virtualization
In-Reply-To: <20170829095726.GL25960@stefanha-x1.localdomain>
On 08/29/2017 05:57 PM, Stefan Hajnoczi wrote:
> On Sun, Aug 27, 2017 at 11:30:33PM +0200, Tomáš Golembiovský wrote:
>> Hi,
> I have CCed the relevant mailing lists and people most recently involved
> in virtio-balloon discussions. Hopefully this will help get the right
> people to see your questions.
>
>> We'd like to include information about reclaimable memory into the
>> statistics in VirtiO Balloon driver. Namely, we'd like to include
>> counters for bufferes and caches of Linux kernel. The patch itself is
>> pretty trivial -- no problem there. But before we do that I'd like to
>> get some input from the QEMU community.
>>
>> 1) Is there any reason not to have the stats there?
Could you please share the usages of reclaimable memory via the stats?
>>
>> 2) Considering the balloon device is multiplatform (Linux, BSD,
>> Windows), is there a problem with including buffers/caches? These seem
>> to be specific to the Linux virtual memory subsystem. Of course, other
>> OSes could just report zeros. Are there some internal stats on those
>> OSes that could be filled in? I don't now if such or similar statistic
>> are available on BSD. On Windows only SystemCache stat looks like
>> something relevant. Anyone familiar with those OSes has any suggestions?
>>
>>
One of the solutions that I'm thinking about is to make virtio
platform-ware.
That is, the device by default supports
VIRTIO_F_LINUX,
VIRTIO_F_WINDOWS,
VIRTIO_F_BSD.
For the Linux driver, only VIRTIO_F_LINUX is supported, then we can
have Linux specific driver implementations under that feature.
Best,
Wei
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [RFC PATCH v2 5/7] Documentation: Add three sysctls for smart idle poll
From: Yang Zhang @ 2017-08-29 11:46 UTC (permalink / raw)
To: linux-kernel
Cc: Yang Zhang, Jeremy Fitzhardinge, kvm, rkrcmar, peterz,
virtualization, H. Peter Anvin, Alok Kataria, wanpeng.li,
Jessica Yu, Baoquan He, Jonathan Corbet, x86, Krzysztof Kozlowski,
Ingo Molnar, zijun_hu, Petr Mladek, Kees Cook, Johannes Berg,
Chris Wright, Ian Abbott, Josh Poimboeuf, dmatlack, tglx,
Mauro Carvalho Chehab, Quan Xu, linux-doc, Luis R. Rodriguez
In-Reply-To: <1504007201-12904-1-git-send-email-yang.zhang.wz@gmail.com>
To reduce the cost of poll, we introduce three sysctl to control the
poll time.
Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
Signed-off-by: Quan Xu <quan.xu0@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Alok Kataria <akataria@vmware.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jessica Yu <jeyu@redhat.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: zijun_hu <zijun_hu@htc.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
Cc: linux-fsdevel@vger.kernel.org
---
Documentation/sysctl/kernel.txt | 25 +++++++++++++++++++++++++
arch/x86/kernel/paravirt.c | 4 ++++
include/linux/kernel.h | 6 ++++++
kernel/sysctl.c | 23 +++++++++++++++++++++++
4 files changed, 58 insertions(+)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index bac23c1..67447b8 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -63,6 +63,9 @@ show up in /proc/sys/kernel:
- perf_event_max_stack
- perf_event_max_contexts_per_stack
- pid_max
+- poll_grow [ X86 only ]
+- poll_shrink [ X86 only ]
+- poll_threshold_ns [ X86 only ]
- powersave-nap [ PPC only ]
- printk
- printk_delay
@@ -702,6 +705,28 @@ kernel tries to allocate a number starting from this one.
==============================================================
+poll_grow: (X86 only)
+
+This parameter is multiplied in the grow_poll_ns() to increase the poll time.
+By default, the values is 2.
+
+==============================================================
+poll_shrink: (X86 only)
+
+This parameter is divided in the shrink_poll_ns() to reduce the poll time.
+By default, the values is 2.
+
+==============================================================
+poll_threshold_ns: (X86 only)
+
+This parameter controls the max poll time before entering real idle path.
+This parameter is expected to take effect only when running inside a VM.
+It would make no sense to turn on it in bare mental.
+By default, the values is 0 means don't poll. It is recommended to change
+the value to non-zero if running latency-bound workloads inside VM.
+
+==============================================================
+
powersave-nap: (PPC only)
If set, Linux-PPC will use the 'nap' mode of powersaving,
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index a11b2c2..0b92f8f 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -318,6 +318,10 @@ struct pv_idle_ops pv_idle_ops = {
.update = paravirt_nop,
};
+unsigned long poll_threshold_ns;
+unsigned int poll_shrink = 2;
+unsigned int poll_grow = 2;
+
__visible struct pv_irq_ops pv_irq_ops = {
.save_fl = __PV_IS_CALLEE_SAVE(native_save_fl),
.restore_fl = __PV_IS_CALLEE_SAVE(native_restore_fl),
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index bd6d96c..6cb2820 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -462,6 +462,12 @@ extern __scanf(2, 0)
extern bool crash_kexec_post_notifiers;
+#ifdef CONFIG_PARAVIRT
+extern unsigned long poll_threshold_ns;
+extern unsigned int poll_shrink;
+extern unsigned int poll_grow;
+#endif
+
/*
* panic_cpu is used for synchronizing panic() and crash_kexec() execution. It
* holds a CPU number which is executing panic() currently. A value of
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6648fbb..9b86a8f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1229,6 +1229,29 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
.extra2 = &one,
},
#endif
+#ifdef CONFIG_PARAVIRT
+ {
+ .procname = "halt_poll_threshold",
+ .data = &poll_threshold_ns,
+ .maxlen = sizeof(unsigned long),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+ {
+ .procname = "halt_poll_grow",
+ .data = &poll_grow,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+ {
+ .procname = "halt_poll_shrink",
+ .data = &poll_shrink,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+#endif
{ }
};
--
1.8.3.1
^ permalink raw reply related
* [RFC PATCH v2 4/7] x86/paravirt: Add update in x86/paravirt pv_idle_ops
From: Yang Zhang @ 2017-08-29 11:46 UTC (permalink / raw)
To: linux-kernel
Cc: Yang Zhang, Jeremy Fitzhardinge, kvm, rkrcmar, peterz, Pan Xinhui,
virtualization, H. Peter Anvin, Alok Kataria, wanpeng.li, x86,
Ingo Molnar, Kees Cook, Chris Wright, Andy Lutomirski, dmatlack,
tglx, Quan Xu, linux-doc, mst, pbonzini, Kirill A. Shutemov
In-Reply-To: <1504007201-12904-1-git-send-email-yang.zhang.wz@gmail.com>
.update is used to adjust the next poll time.
Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
Signed-off-by: Quan Xu <quan.xu0@gmail.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Alok Kataria <akataria@vmware.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
---
arch/x86/include/asm/paravirt.h | 5 +++++
arch/x86/include/asm/paravirt_types.h | 1 +
arch/x86/kernel/paravirt.c | 1 +
3 files changed, 7 insertions(+)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 6d46760..32e1c06 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -207,6 +207,11 @@ static inline void paravirt_idle_poll(void)
PVOP_VCALL0(pv_idle_ops.poll);
}
+static inline void paravirt_idle_update_poll_duration(unsigned long duration)
+{
+ PVOP_VCALL1(pv_idle_ops.update, duration);
+}
+
static inline void paravirt_alloc_ldt(struct desc_struct *ldt, unsigned entries)
{
PVOP_VCALL2(pv_cpu_ops.alloc_ldt, ldt, entries);
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index cf45726..3b4f95a 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -326,6 +326,7 @@ struct pv_lock_ops {
struct pv_idle_ops {
void (*poll)(void);
+ void (*update)(unsigned long);
} __no_randomize_layout;
/* This contains all the paravirt structures: we get a convenient
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 1b5b247..a11b2c2 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -315,6 +315,7 @@ struct pv_time_ops pv_time_ops = {
struct pv_idle_ops pv_idle_ops = {
.poll = paravirt_nop,
+ .update = paravirt_nop,
};
__visible struct pv_irq_ops pv_irq_ops = {
--
1.8.3.1
^ permalink raw reply related
* [RFC PATCH v2 1/7] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Yang Zhang @ 2017-08-29 11:46 UTC (permalink / raw)
To: linux-kernel
Cc: Yang Zhang, Jeremy Fitzhardinge, kvm, rkrcmar, peterz, Pan Xinhui,
virtualization, H. Peter Anvin, Alok Kataria, wanpeng.li, x86,
Ingo Molnar, Kees Cook, Chris Wright, Andy Lutomirski, dmatlack,
tglx, Quan Xu, linux-doc, mst, pbonzini, Kirill A. Shutemov
In-Reply-To: <1504007201-12904-1-git-send-email-yang.zhang.wz@gmail.com>
So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called in
idle path which will polling for a while before we enter the real idle
state.
In virtualization, idle path includes several heavy operations
includes timer access(LAPIC timer or TSC deadline timer) which will hurt
performance especially for latency intensive workload like message
passing task. The cost is mainly come from the vmexit which is a
hardware context switch between VM and hypervisor. Our solution is to
poll for a while and do not enter real idle path if we can get the
schedule event during polling.
Poll may cause the CPU waste so we adopt a smart polling mechanism to
reduce the useless poll.
Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
Signed-off-by: Quan Xu <quan.xu0@gmail.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Alok Kataria <akataria@vmware.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
---
arch/x86/include/asm/paravirt.h | 5 +++++
arch/x86/include/asm/paravirt_types.h | 6 ++++++
arch/x86/kernel/paravirt.c | 6 ++++++
3 files changed, 17 insertions(+)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 9ccac19..6d46760 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -202,6 +202,11 @@ static inline unsigned long long paravirt_read_pmc(int counter)
#define rdpmcl(counter, val) ((val) = paravirt_read_pmc(counter))
+static inline void paravirt_idle_poll(void)
+{
+ PVOP_VCALL0(pv_idle_ops.poll);
+}
+
static inline void paravirt_alloc_ldt(struct desc_struct *ldt, unsigned entries)
{
PVOP_VCALL2(pv_cpu_ops.alloc_ldt, ldt, entries);
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 9ffc36b..cf45726 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -324,6 +324,10 @@ struct pv_lock_ops {
struct paravirt_callee_save vcpu_is_preempted;
} __no_randomize_layout;
+struct pv_idle_ops {
+ void (*poll)(void);
+} __no_randomize_layout;
+
/* This contains all the paravirt structures: we get a convenient
* number for each function using the offset which we use to indicate
* what to patch. */
@@ -334,6 +338,7 @@ struct paravirt_patch_template {
struct pv_irq_ops pv_irq_ops;
struct pv_mmu_ops pv_mmu_ops;
struct pv_lock_ops pv_lock_ops;
+ struct pv_idle_ops pv_idle_ops;
} __no_randomize_layout;
extern struct pv_info pv_info;
@@ -343,6 +348,7 @@ struct paravirt_patch_template {
extern struct pv_irq_ops pv_irq_ops;
extern struct pv_mmu_ops pv_mmu_ops;
extern struct pv_lock_ops pv_lock_ops;
+extern struct pv_idle_ops pv_idle_ops;
#define PARAVIRT_PATCH(x) \
(offsetof(struct paravirt_patch_template, x) / sizeof(void *))
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index bc0a849..1b5b247 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -128,6 +128,7 @@ static void *get_call_destination(u8 type)
#ifdef CONFIG_PARAVIRT_SPINLOCKS
.pv_lock_ops = pv_lock_ops,
#endif
+ .pv_idle_ops = pv_idle_ops,
};
return *((void **)&tmpl + type);
}
@@ -312,6 +313,10 @@ struct pv_time_ops pv_time_ops = {
.steal_clock = native_steal_clock,
};
+struct pv_idle_ops pv_idle_ops = {
+ .poll = paravirt_nop,
+};
+
__visible struct pv_irq_ops pv_irq_ops = {
.save_fl = __PV_IS_CALLEE_SAVE(native_save_fl),
.restore_fl = __PV_IS_CALLEE_SAVE(native_restore_fl),
@@ -471,3 +476,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_idle_ops);
--
1.8.3.1
^ permalink raw reply related
* Re: [Qemu-devel] [RFC] Buffers/caches in VirtIO Balloon driver stats
From: Stefan Hajnoczi @ 2017-08-29 9:57 UTC (permalink / raw)
To: Tomáš Golembiovský
Cc: virtio-dev, kvm, Michael S. Tsirkin, qemu-devel, virtualization
In-Reply-To: <20170827233033.7afa49b9@fiorina>
On Sun, Aug 27, 2017 at 11:30:33PM +0200, Tomáš Golembiovský wrote:
> Hi,
I have CCed the relevant mailing lists and people most recently involved
in virtio-balloon discussions. Hopefully this will help get the right
people to see your questions.
> We'd like to include information about reclaimable memory into the
> statistics in VirtiO Balloon driver. Namely, we'd like to include
> counters for bufferes and caches of Linux kernel. The patch itself is
> pretty trivial -- no problem there. But before we do that I'd like to
> get some input from the QEMU community.
>
> 1) Is there any reason not to have the stats there?
>
> 2) Considering the balloon device is multiplatform (Linux, BSD,
> Windows), is there a problem with including buffers/caches? These seem
> to be specific to the Linux virtual memory subsystem. Of course, other
> OSes could just report zeros. Are there some internal stats on those
> OSes that could be filled in? I don't now if such or similar statistic
> are available on BSD. On Windows only SystemCache stat looks like
> something relevant. Anyone familiar with those OSes has any suggestions?
>
> Tomas
>
> --
> 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 v15 4/5] mm: support reporting free page blocks
From: Wei Wang @ 2017-08-29 3:23 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, mawilcox, linux-kernel, willy, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mgorman
In-Reply-To: <20170828133326.GN17097@dhcp22.suse.cz>
On 08/28/2017 09:33 PM, Michal Hocko wrote:
> On Mon 28-08-17 18:08:32, Wei Wang wrote:
>> This patch adds support to walk through the free page blocks in the
>> system and report them via a callback function. Some page blocks may
>> leave the free list after zone->lock is released, so it is the caller's
>> responsibility to either detect or prevent the use of such pages.
>>
>> One use example of this patch is to accelerate live migration by skipping
>> the transfer of free pages reported from the guest. A popular method used
>> by the hypervisor to track which part of memory is written during live
>> migration is to write-protect all the guest memory. So, those pages that
>> are reported as free pages but are written after the report function
>> returns will be captured by the hypervisor, and they will be added to the
>> next round of memory transfer.
> OK, looks much better. I still have few nits.
>
>> +extern void walk_free_mem_block(void *opaque,
>> + int min_order,
>> + bool (*report_page_block)(void *, unsigned long,
>> + unsigned long));
>> +
> please add names to arguments of the prototype
>
>> /*
>> * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
>> * into the buddy system. The freed pages will be poisoned with pattern
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 6d00f74..81eedc7 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -4762,6 +4762,71 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
>> show_swap_cache_info();
>> }
>>
>> +/**
>> + * walk_free_mem_block - Walk through the free page blocks in the system
>> + * @opaque: the context passed from the caller
>> + * @min_order: the minimum order of free lists to check
>> + * @report_page_block: the callback function to report free page blocks
> page_block has meaning in the core MM which doesn't strictly match its
> usage here. Moreover we are reporting pfn ranges rather than struct page
> range. So report_pfn_range would suit better.
>
> [...]
>> + for_each_populated_zone(zone) {
>> + for (order = MAX_ORDER - 1; order >= min_order; order--) {
>> + for (mt = 0; !stop && mt < MIGRATE_TYPES; mt++) {
>> + spin_lock_irqsave(&zone->lock, flags);
>> + list = &zone->free_area[order].free_list[mt];
>> + list_for_each_entry(page, list, lru) {
>> + pfn = page_to_pfn(page);
>> + stop = report_page_block(opaque, pfn,
>> + 1 << order);
>> + if (stop)
>> + break;
> if (stop) {
> spin_unlock_irqrestore(&zone->lock, flags);
> return;
> }
>
> would be both easier and less error prone. E.g. You wouldn't pointlessly
> iterate over remaining orders just to realize there is nothing to be
> done for those...
>
Yes, that's better, thanks. I will take other suggestions as well.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v15 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wei Wang @ 2017-08-29 3:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <20170828204659-mutt-send-email-mst@kernel.org>
On 08/29/2017 02:03 AM, Michael S. Tsirkin wrote:
> On Mon, Aug 28, 2017 at 06:08:31PM +0800, Wei Wang wrote:
>> Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer
>> of balloon (i.e. inflated/deflated) pages using scatter-gather lists
>> to the host.
>>
>> The implementation of the previous virtio-balloon is not very
>> efficient, because the balloon pages are transferred to the
>> host one by one. Here is the breakdown of the time in percentage
>> spent on each step of the balloon inflating process (inflating
>> 7GB of an 8GB idle guest).
>>
>> 1) allocating pages (6.5%)
>> 2) sending PFNs to host (68.3%)
>> 3) address translation (6.1%)
>> 4) madvise (19%)
>>
>> It takes about 4126ms for the inflating process to complete.
>> The above profiling shows that the bottlenecks are stage 2)
>> and stage 4).
>>
>> This patch optimizes step 2) by transferring pages to the host in
>> sgs. An sg describes a chunk of guest physically continuous pages.
>> With this mechanism, step 4) can also be optimized by doing address
>> translation and madvise() in chunks rather than page by page.
>>
>> With this new feature, the above ballooning process takes ~597ms
>> resulting in an improvement of ~86%.
>>
>> TODO: optimize stage 1) by allocating/freeing a chunk of pages
>> instead of a single page each time.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Signed-off-by: Liang Li <liang.z.li@intel.com>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 171 ++++++++++++++++++++++++++++++++----
>> include/uapi/linux/virtio_balloon.h | 1 +
>> 2 files changed, 155 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index f0b3a0b..8ecc1d4 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -32,6 +32,8 @@
>> #include <linux/mm.h>
>> #include <linux/mount.h>
>> #include <linux/magic.h>
>> +#include <linux/xbitmap.h>
>> +#include <asm/page.h>
>>
>> /*
>> * Balloon device works in 4K page units. So each page is pointed to by
>> @@ -79,6 +81,9 @@ struct virtio_balloon {
>> /* Synchronize access/update to this struct virtio_balloon elements */
>> struct mutex balloon_lock;
>>
>> + /* The xbitmap used to record balloon pages */
>> + struct xb page_xb;
>> +
>> /* The array of pfns we tell the Host about. */
>> unsigned int num_pfns;
>> __virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
>> @@ -141,13 +146,111 @@ static void set_page_pfns(struct virtio_balloon *vb,
>> page_to_balloon_pfn(page) + i);
>> }
>>
>> +static int add_one_sg(struct virtqueue *vq, void *addr, uint32_t size)
>> +{
>> + struct scatterlist sg;
>> +
>> + sg_init_one(&sg, addr, size);
>> + return virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
>> +}
>> +
>> +static void send_balloon_page_sg(struct virtio_balloon *vb,
>> + struct virtqueue *vq,
>> + void *addr,
>> + uint32_t size,
>> + bool batch)
>> +{
>> + unsigned int len;
>> + int err;
>> +
>> + err = add_one_sg(vq, addr, size);
>> + /* Sanity check: this can't really happen */
>> + WARN_ON(err);
> It might be cleaner to detect that add failed due to
> ring full and kick then. Just an idea, up to you
> whether to do it.
>
>> +
>> + /* If batching is in use, we batch the sgs till the vq is full. */
>> + if (!batch || !vq->num_free) {
>> + virtqueue_kick(vq);
>> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
>> + /* Release all the entries if there are */
> Meaning
> Account for all used entries if any
> ?
>
>> + while (virtqueue_get_buf(vq, &len))
>> + ;
>
> Above code is reused below. Add a function?
>
>> + }
>> +}
>> +
>> +/*
>> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
>> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
>> + * The page xbitmap is searched for continuous "1" bits, which correspond
>> + * to continuous pages, to chunk into sgs.
>> + *
>> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
>> + * need to be searched.
>> + */
>> +static void tell_host_sgs(struct virtio_balloon *vb,
>> + struct virtqueue *vq,
>> + unsigned long page_xb_start,
>> + unsigned long page_xb_end)
>> +{
>> + unsigned long sg_pfn_start, sg_pfn_end;
>> + void *sg_addr;
>> + uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
>> +
>> + sg_pfn_start = page_xb_start;
>> + while (sg_pfn_start < page_xb_end) {
>> + sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
>> + page_xb_end, 1);
>> + if (sg_pfn_start == page_xb_end + 1)
>> + break;
>> + sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
>> + page_xb_end, 0);
>> + sg_addr = (void *)pfn_to_kaddr(sg_pfn_start);
>> + sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
>> + while (sg_len > sg_max_len) {
>> + send_balloon_page_sg(vb, vq, sg_addr, sg_max_len, 1);
> Last argument should be true, not 1.
>
>> + sg_addr += sg_max_len;
>> + sg_len -= sg_max_len;
>> + }
>> + send_balloon_page_sg(vb, vq, sg_addr, sg_len, 1);
>> + xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
>> + sg_pfn_start = sg_pfn_end + 1;
>> + }
>> +
>> + /*
>> + * The last few sgs may not reach the batch size, but need a kick to
>> + * notify the device to handle them.
>> + */
>> + if (vq->num_free != virtqueue_get_vring_size(vq)) {
>> + virtqueue_kick(vq);
>> + wait_event(vb->acked, virtqueue_get_buf(vq, &sg_len));
>> + while (virtqueue_get_buf(vq, &sg_len))
>> + ;
> Some entries can get used after a pause. Looks like they will leak then?
> One fix would be to convert above if to a while loop.
> I don't know whether to do it like this in send_balloon_page_sg too.
>
Thanks for the above comments. I've re-written this part of code.
Please have a check below if there is anything more we could improve:
static void kick_and_wait(struct virtqueue *vq, wait_queue_head_t wq_head)
{
unsigned int len;
virtqueue_kick(vq);
wait_event(wq_head, virtqueue_get_buf(vq, &len));
/* Detach all the used buffers from the vq */
while (virtqueue_get_buf(vq, &len))
;
}
static int add_one_sg(struct virtqueue *vq, void *addr, uint32_t size)
{
struct scatterlist sg;
int ret;
sg_init_one(&sg, addr, size);
ret = virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
if (unlikely(ret == -ENOSPC))
dev_warn(&vq->vdev->dev, "%s: failed due to ring full\n",
__func__);
return ret;
}
static void send_balloon_page_sg(struct virtio_balloon *vb,
struct virtqueue *vq,
void *addr,
uint32_t size,
bool batch)
{
int err;
do {
err = add_one_sg(vq, addr, size);
if (err == -ENOSPC || !batch || !vq->num_free)
kick_and_wait(vq, vb->acked);
} while (err == -ENOSPC);
}
Best,
Wei
^ permalink raw reply
* Re: [PATCH v15 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-08-28 18:03 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <1503914913-28893-4-git-send-email-wei.w.wang@intel.com>
On Mon, Aug 28, 2017 at 06:08:31PM +0800, Wei Wang wrote:
> Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer
> of balloon (i.e. inflated/deflated) pages using scatter-gather lists
> to the host.
>
> The implementation of the previous virtio-balloon is not very
> efficient, because the balloon pages are transferred to the
> host one by one. Here is the breakdown of the time in percentage
> spent on each step of the balloon inflating process (inflating
> 7GB of an 8GB idle guest).
>
> 1) allocating pages (6.5%)
> 2) sending PFNs to host (68.3%)
> 3) address translation (6.1%)
> 4) madvise (19%)
>
> It takes about 4126ms for the inflating process to complete.
> The above profiling shows that the bottlenecks are stage 2)
> and stage 4).
>
> This patch optimizes step 2) by transferring pages to the host in
> sgs. An sg describes a chunk of guest physically continuous pages.
> With this mechanism, step 4) can also be optimized by doing address
> translation and madvise() in chunks rather than page by page.
>
> With this new feature, the above ballooning process takes ~597ms
> resulting in an improvement of ~86%.
>
> TODO: optimize stage 1) by allocating/freeing a chunk of pages
> instead of a single page each time.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 171 ++++++++++++++++++++++++++++++++----
> include/uapi/linux/virtio_balloon.h | 1 +
> 2 files changed, 155 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b..8ecc1d4 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -32,6 +32,8 @@
> #include <linux/mm.h>
> #include <linux/mount.h>
> #include <linux/magic.h>
> +#include <linux/xbitmap.h>
> +#include <asm/page.h>
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -79,6 +81,9 @@ struct virtio_balloon {
> /* Synchronize access/update to this struct virtio_balloon elements */
> struct mutex balloon_lock;
>
> + /* The xbitmap used to record balloon pages */
> + struct xb page_xb;
> +
> /* The array of pfns we tell the Host about. */
> unsigned int num_pfns;
> __virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
> @@ -141,13 +146,111 @@ static void set_page_pfns(struct virtio_balloon *vb,
> page_to_balloon_pfn(page) + i);
> }
>
> +static int add_one_sg(struct virtqueue *vq, void *addr, uint32_t size)
> +{
> + struct scatterlist sg;
> +
> + sg_init_one(&sg, addr, size);
> + return virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
> +}
> +
> +static void send_balloon_page_sg(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + void *addr,
> + uint32_t size,
> + bool batch)
> +{
> + unsigned int len;
> + int err;
> +
> + err = add_one_sg(vq, addr, size);
> + /* Sanity check: this can't really happen */
> + WARN_ON(err);
It might be cleaner to detect that add failed due to
ring full and kick then. Just an idea, up to you
whether to do it.
> +
> + /* If batching is in use, we batch the sgs till the vq is full. */
> + if (!batch || !vq->num_free) {
> + virtqueue_kick(vq);
> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> + /* Release all the entries if there are */
Meaning
Account for all used entries if any
?
> + while (virtqueue_get_buf(vq, &len))
> + ;
Above code is reused below. Add a function?
> + }
> +}
> +
> +/*
> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> + * The page xbitmap is searched for continuous "1" bits, which correspond
> + * to continuous pages, to chunk into sgs.
> + *
> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
> + * need to be searched.
> + */
> +static void tell_host_sgs(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + unsigned long page_xb_start,
> + unsigned long page_xb_end)
> +{
> + unsigned long sg_pfn_start, sg_pfn_end;
> + void *sg_addr;
> + uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
> +
> + sg_pfn_start = page_xb_start;
> + while (sg_pfn_start < page_xb_end) {
> + sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
> + page_xb_end, 1);
> + if (sg_pfn_start == page_xb_end + 1)
> + break;
> + sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
> + page_xb_end, 0);
> + sg_addr = (void *)pfn_to_kaddr(sg_pfn_start);
> + sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
> + while (sg_len > sg_max_len) {
> + send_balloon_page_sg(vb, vq, sg_addr, sg_max_len, 1);
Last argument should be true, not 1.
> + sg_addr += sg_max_len;
> + sg_len -= sg_max_len;
> + }
> + send_balloon_page_sg(vb, vq, sg_addr, sg_len, 1);
> + xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
> + sg_pfn_start = sg_pfn_end + 1;
> + }
> +
> + /*
> + * The last few sgs may not reach the batch size, but need a kick to
> + * notify the device to handle them.
> + */
> + if (vq->num_free != virtqueue_get_vring_size(vq)) {
> + virtqueue_kick(vq);
> + wait_event(vb->acked, virtqueue_get_buf(vq, &sg_len));
> + while (virtqueue_get_buf(vq, &sg_len))
> + ;
Some entries can get used after a pause. Looks like they will leak then?
One fix would be to convert above if to a while loop.
I don't know whether to do it like this in send_balloon_page_sg too.
> + }
> +}
> +
> +static inline void xb_set_page(struct virtio_balloon *vb,
> + struct page *page,
> + unsigned long *pfn_min,
> + unsigned long *pfn_max)
> +{
> + unsigned long pfn = page_to_pfn(page);
> +
> + *pfn_min = min(pfn, *pfn_min);
> + *pfn_max = max(pfn, *pfn_max);
> + xb_preload(GFP_KERNEL);
> + xb_set_bit(&vb->page_xb, pfn);
> + xb_preload_end();
> +}
> +
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> @@ -162,7 +265,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> msleep(200);
> break;
> }
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> + if (use_sg)
> + xb_set_page(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> @@ -171,8 +279,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>
> num_allocated_pages = vb->num_pfns;
> /* Did we get any? */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->inflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->inflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->inflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> return num_allocated_pages;
> @@ -198,9 +310,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> struct page *page;
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> LIST_HEAD(pages);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> - /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + /* Traditionally, we can only do one array worth at a time. */
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> /* We can't release more pages than taken */
> @@ -210,7 +325,11 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> page = balloon_page_dequeue(vb_dev_info);
> if (!page)
> break;
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> + if (use_sg)
> + xb_set_page(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> list_add(&page->lru, &pages);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> @@ -221,8 +340,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->deflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->deflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->deflate_vq);
> + }
> release_pages_balloon(vb, &pages);
> mutex_unlock(&vb->balloon_lock);
> return num_freed_pages;
> @@ -441,6 +564,7 @@ static int init_vqs(struct virtio_balloon *vb)
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> +
> /*
> * virtballoon_migratepage - perform the balloon page migration on behalf of
> * a compation thread. (called under page lock)
> @@ -464,6 +588,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> {
> struct virtio_balloon *vb = container_of(vb_dev_info,
> struct virtio_balloon, vb_dev_info);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> unsigned long flags;
>
> /*
> @@ -485,16 +610,24 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> vb_dev_info->isolated_pages--;
> __count_vm_event(BALLOON_MIGRATE);
> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, newpage);
> - tell_host(vb, vb->inflate_vq);
> -
> + if (use_sg) {
> + send_balloon_page_sg(vb, vb->inflate_vq, page_address(newpage),
> + PAGE_SIZE, 0);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> + }
> /* balloon's page migration 2nd step -- deflate "page" */
> balloon_page_delete(page);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, page);
> - tell_host(vb, vb->deflate_vq);
> -
> + if (use_sg) {
> + send_balloon_page_sg(vb, vb->deflate_vq, page_address(page),
> + PAGE_SIZE, 0);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> put_page(page); /* balloon reference */
> @@ -553,6 +686,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (err)
> goto out_free_vb;
>
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> + xb_init(&vb->page_xb);
> +
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -669,6 +805,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_MUST_TELL_HOST,
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> + VIRTIO_BALLOON_F_SG,
> };
>
> static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..37780a7 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> --
> 2.7.4
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox