* [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Jason Baron via Virtualization @ 2018-01-04 5:16 UTC (permalink / raw)
To: mst, davem; +Cc: netdev, virtio-dev, qemu-devel, virtualization
In-Reply-To: <cover.1515041373.git.jbaron@akamai.com>
The ability to set speed and duplex for virtio_net is useful in various
scenarios as described here:
16032be virtio_net: add ethtool support for set and get of settings
However, it would be nice to be able to set this from the hypervisor,
such that virtio_net doesn't require custom guest ethtool commands.
Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
the hypervisor to export a linkspeed and duplex setting. The user can
subsequently overwrite it later if desired via: 'ethtool -s'.
Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
is that device feature bits are to grow down from bit 63, since the
transports are starting from bit 24 and growing up.
Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: virtio-dev@lists.oasis-open.org
---
drivers/net/virtio_net.c | 19 ++++++++++++++++++-
include/uapi/linux/virtio_net.h | 13 +++++++++++++
2 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6fb7b65..0b2d314 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
vi->status = v;
+ if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
+ u32 speed;
+ u8 duplex;
+
+ speed = virtio_cread32(vi->vdev,
+ offsetof(struct virtio_net_config,
+ speed));
+ if (ethtool_validate_speed(speed))
+ vi->speed = speed;
+ duplex = virtio_cread8(vi->vdev,
+ offsetof(struct virtio_net_config,
+ duplex));
+ if (ethtool_validate_duplex(duplex))
+ vi->duplex = duplex;
+ }
+
if (vi->status & VIRTIO_NET_S_LINK_UP) {
netif_carrier_on(vi->dev);
netif_tx_wake_all_queues(vi->dev);
@@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
VIRTIO_NET_F_CTRL_MAC_ADDR, \
- VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
+ VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
+ VIRTIO_NET_F_SPEED_DUPLEX
static unsigned int features[] = {
VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index fc353b5..5de6ed3 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -57,6 +57,8 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
+
#ifndef VIRTIO_NET_NO_LEGACY
#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
#endif /* VIRTIO_NET_NO_LEGACY */
@@ -76,6 +78,17 @@ struct virtio_net_config {
__u16 max_virtqueue_pairs;
/* Default maximum transmit unit advice */
__u16 mtu;
+ /*
+ * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
+ * Any other value stands for unknown.
+ */
+ __u32 speed;
+ /*
+ * 0x00 - half duplex
+ * 0x01 - full duplex
+ * Any other value stands for unknown.
+ */
+ __u8 duplex;
} __attribute__((packed));
/*
--
2.6.1
^ permalink raw reply related
* [PATCH v3 3/3] qemu: add linkspeed and duplex settings to virtio-net
From: Jason Baron via Virtualization @ 2018-01-04 5:16 UTC (permalink / raw)
To: mst, davem; +Cc: netdev, virtio-dev, qemu-devel, virtualization
In-Reply-To: <cover.1515041373.git.jbaron@akamai.com>
Although linkspeed and duplex can be set in a linux guest via 'ethtool -s',
this requires custom ethtool commands for virtio-net by default.
Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
the hypervisor to export a linkspeed and duplex setting. The user can
subsequently overwrite it later if desired via: 'ethtool -s'.
Linkspeed and duplex settings can be set as:
'-device virtio-net,speed=10000,duplex=full'
where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: virtio-dev@lists.oasis-open.org
---
hw/net/virtio-net.c | 35 +++++++++++++++++++++++++++++
include/hw/virtio/virtio-net.h | 3 +++
include/standard-headers/linux/virtio_net.h | 13 +++++++++++
3 files changed, 51 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index adc20df..eec8422 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -40,6 +40,12 @@
#define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
#define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
+/* duplex and speed */
+#define DUPLEX_UNKNOWN 0xff
+#define DUPLEX_HALF 0x00
+#define DUPLEX_FULL 0x01
+#define SPEED_UNKNOWN -1
+
/*
* Calculate the number of bytes up to and including the given 'field' of
* 'container'.
@@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
.end = endof(struct virtio_net_config, max_virtqueue_pairs)},
{.flags = 1ULL << VIRTIO_NET_F_MTU,
.end = endof(struct virtio_net_config, mtu)},
+ {.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX,
+ .end = endof(struct virtio_net_config, duplex)},
{}
};
@@ -89,6 +97,14 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
memcpy(netcfg.mac, n->mac, ETH_ALEN);
+ if (n->status & VIRTIO_NET_S_LINK_UP) {
+ virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
+ netcfg.duplex = n->net_conf.duplex;
+ } else {
+ virtio_stl_p(vdev, &netcfg.speed, SPEED_UNKNOWN);
+ netcfg.duplex = DUPLEX_UNKNOWN;
+ }
+
memcpy(config, &netcfg, n->config_size);
}
@@ -1941,6 +1957,23 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
}
+ n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
+ if (n->net_conf.duplex_str) {
+ if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
+ n->net_conf.duplex = DUPLEX_HALF;
+ } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
+ n->net_conf.duplex = DUPLEX_FULL;
+ } else {
+ error_setg(errp, "'duplex' must be 'half' or 'full'");
+ }
+ } else {
+ n->net_conf.duplex = DUPLEX_UNKNOWN;
+ }
+ if (n->net_conf.speed < SPEED_UNKNOWN) {
+ error_setg(errp, "'speed' must be between -1 (SPEED_UNKOWN) and "
+ "INT_MAX");
+ }
+
virtio_net_set_config_size(n, n->host_features);
virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
@@ -2160,6 +2193,8 @@ static Property virtio_net_properties[] = {
DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
true),
+ DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
+ DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index e7634c9..02484dc 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -38,6 +38,9 @@ typedef struct virtio_net_conf
uint16_t rx_queue_size;
uint16_t tx_queue_size;
uint16_t mtu;
+ int32_t speed;
+ char *duplex_str;
+ uint8_t duplex;
} virtio_net_conf;
/* Maximum packet size we can receive from tap device: header + 64k */
diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
index 30ff249..17c8531 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -57,6 +57,8 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
+
#ifndef VIRTIO_NET_NO_LEGACY
#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
#endif /* VIRTIO_NET_NO_LEGACY */
@@ -76,6 +78,17 @@ struct virtio_net_config {
uint16_t max_virtqueue_pairs;
/* Default maximum transmit unit advice */
uint16_t mtu;
+ /*
+ * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
+ * Any other value stands for unknown.
+ */
+ uint32_t speed;
+ /*
+ * 0x00 - half duplex
+ * 0x01 - full duplex
+ * Any other value stands for unknown.
+ */
+ uint8_t duplex;
} QEMU_PACKED;
/*
^ permalink raw reply related
* Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Michael S. Tsirkin @ 2018-01-04 16:27 UTC (permalink / raw)
To: Jason Baron; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <e185a03168d0ab68150d21770bf0d02c5ef537c2.1515041373.git.jbaron@akamai.com>
On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote:
> The ability to set speed and duplex for virtio_net is useful in various
> scenarios as described here:
>
> 16032be virtio_net: add ethtool support for set and get of settings
>
> However, it would be nice to be able to set this from the hypervisor,
> such that virtio_net doesn't require custom guest ethtool commands.
>
> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
> the hypervisor to export a linkspeed and duplex setting. The user can
> subsequently overwrite it later if desired via: 'ethtool -s'.
>
> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
> is that device feature bits are to grow down from bit 63, since the
> transports are starting from bit 24 and growing up.
>
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: virtio-dev@lists.oasis-open.org
> ---
> drivers/net/virtio_net.c | 19 ++++++++++++++++++-
> include/uapi/linux/virtio_net.h | 13 +++++++++++++
> 2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6fb7b65..0b2d314 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
>
> vi->status = v;
>
> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> + u32 speed;
> + u8 duplex;
> +
> + speed = virtio_cread32(vi->vdev,
> + offsetof(struct virtio_net_config,
> + speed));
> + if (ethtool_validate_speed(speed))
> + vi->speed = speed;
> + duplex = virtio_cread8(vi->vdev,
> + offsetof(struct virtio_net_config,
> + duplex));
> + if (ethtool_validate_duplex(duplex))
> + vi->duplex = duplex;
> + }
> +
> if (vi->status & VIRTIO_NET_S_LINK_UP) {
> netif_carrier_on(vi->dev);
> netif_tx_wake_all_queues(vi->dev);
> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> VIRTIO_NET_F_CTRL_MAC_ADDR, \
> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> + VIRTIO_NET_F_SPEED_DUPLEX
>
> static unsigned int features[] = {
> VIRTNET_FEATURES,
Still missing the update from virtnet_config_changed_work, and I think
it's important to reflex host changes within guest when the
feature bit has been acked.
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index fc353b5..5de6ed3 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -57,6 +57,8 @@
> * Steering */
> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>
> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
> +
> #ifndef VIRTIO_NET_NO_LEGACY
> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
> #endif /* VIRTIO_NET_NO_LEGACY */
> @@ -76,6 +78,17 @@ struct virtio_net_config {
> __u16 max_virtqueue_pairs;
> /* Default maximum transmit unit advice */
> __u16 mtu;
> + /*
> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
> + * Any other value stands for unknown.
> + */
> + __u32 speed;
> + /*
> + * 0x00 - half duplex
> + * 0x01 - full duplex
> + * Any other value stands for unknown.
> + */
> + __u8 duplex;
> } __attribute__((packed));
>
> /*
> --
> 2.6.1
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Jason Baron via Virtualization @ 2018-01-04 16:57 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <20180104182602-mutt-send-email-mst@kernel.org>
On 01/04/2018 11:27 AM, Michael S. Tsirkin wrote:
> On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote:
>> The ability to set speed and duplex for virtio_net is useful in various
>> scenarios as described here:
>>
>> 16032be virtio_net: add ethtool support for set and get of settings
>>
>> However, it would be nice to be able to set this from the hypervisor,
>> such that virtio_net doesn't require custom guest ethtool commands.
>>
>> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
>> the hypervisor to export a linkspeed and duplex setting. The user can
>> subsequently overwrite it later if desired via: 'ethtool -s'.
>>
>> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
>> is that device feature bits are to grow down from bit 63, since the
>> transports are starting from bit 24 and growing up.
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: virtio-dev@lists.oasis-open.org
>> ---
>> drivers/net/virtio_net.c | 19 ++++++++++++++++++-
>> include/uapi/linux/virtio_net.h | 13 +++++++++++++
>> 2 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 6fb7b65..0b2d314 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
>>
>> vi->status = v;
>>
>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
>> + u32 speed;
>> + u8 duplex;
>> +
>> + speed = virtio_cread32(vi->vdev,
>> + offsetof(struct virtio_net_config,
>> + speed));
>> + if (ethtool_validate_speed(speed))
>> + vi->speed = speed;
>> + duplex = virtio_cread8(vi->vdev,
>> + offsetof(struct virtio_net_config,
>> + duplex));
>> + if (ethtool_validate_duplex(duplex))
>> + vi->duplex = duplex;
>> + }
>> +
>> if (vi->status & VIRTIO_NET_S_LINK_UP) {
>> netif_carrier_on(vi->dev);
>> netif_tx_wake_all_queues(vi->dev);
>> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>> VIRTIO_NET_F_CTRL_MAC_ADDR, \
>> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>> + VIRTIO_NET_F_SPEED_DUPLEX
>>
>> static unsigned int features[] = {
>> VIRTNET_FEATURES,
>
> Still missing the update from virtnet_config_changed_work, and I think
> it's important to reflex host changes within guest when the
> feature bit has been acked.
>
I update vi->speed and vi->duplex in virtnet_config_changed_work(). And
I tested using the 'set_link' on/off from the qemu monitor.
Specifically, an 'off' sets the speed and link to 'unknown', and an 'on'
returns the speed and link to the configured speed and duplex. So they
are being updated dynamically now. What host changes are you referring
to that are not reflected?
Thanks,
-Jason
>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>> index fc353b5..5de6ed3 100644
>> --- a/include/uapi/linux/virtio_net.h
>> +++ b/include/uapi/linux/virtio_net.h
>> @@ -57,6 +57,8 @@
>> * Steering */
>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>>
>> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
>> +
>> #ifndef VIRTIO_NET_NO_LEGACY
>> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
>> #endif /* VIRTIO_NET_NO_LEGACY */
>> @@ -76,6 +78,17 @@ struct virtio_net_config {
>> __u16 max_virtqueue_pairs;
>> /* Default maximum transmit unit advice */
>> __u16 mtu;
>> + /*
>> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
>> + * Any other value stands for unknown.
>> + */
>> + __u32 speed;
>> + /*
>> + * 0x00 - half duplex
>> + * 0x01 - full duplex
>> + * Any other value stands for unknown.
>> + */
>> + __u8 duplex;
>> } __attribute__((packed));
>>
>> /*
>> --
>> 2.6.1
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Michael S. Tsirkin @ 2018-01-04 17:02 UTC (permalink / raw)
To: Jason Baron; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <bda195e5-70a3-aecc-260b-7979522d471a@akamai.com>
On Thu, Jan 04, 2018 at 11:57:44AM -0500, Jason Baron wrote:
>
>
> On 01/04/2018 11:27 AM, Michael S. Tsirkin wrote:
> > On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote:
> >> The ability to set speed and duplex for virtio_net is useful in various
> >> scenarios as described here:
> >>
> >> 16032be virtio_net: add ethtool support for set and get of settings
> >>
> >> However, it would be nice to be able to set this from the hypervisor,
> >> such that virtio_net doesn't require custom guest ethtool commands.
> >>
> >> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
> >> the hypervisor to export a linkspeed and duplex setting. The user can
> >> subsequently overwrite it later if desired via: 'ethtool -s'.
> >>
> >> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
> >> is that device feature bits are to grow down from bit 63, since the
> >> transports are starting from bit 24 and growing up.
> >>
> >> Signed-off-by: Jason Baron <jbaron@akamai.com>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Jason Wang <jasowang@redhat.com>
> >> Cc: virtio-dev@lists.oasis-open.org
> >> ---
> >> drivers/net/virtio_net.c | 19 ++++++++++++++++++-
> >> include/uapi/linux/virtio_net.h | 13 +++++++++++++
> >> 2 files changed, 31 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 6fb7b65..0b2d314 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
> >>
> >> vi->status = v;
> >>
> >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> >> + u32 speed;
> >> + u8 duplex;
> >> +
> >> + speed = virtio_cread32(vi->vdev,
> >> + offsetof(struct virtio_net_config,
> >> + speed));
> >> + if (ethtool_validate_speed(speed))
> >> + vi->speed = speed;
> >> + duplex = virtio_cread8(vi->vdev,
> >> + offsetof(struct virtio_net_config,
> >> + duplex));
> >> + if (ethtool_validate_duplex(duplex))
> >> + vi->duplex = duplex;
> >> + }
> >> +
> >> if (vi->status & VIRTIO_NET_S_LINK_UP) {
> >> netif_carrier_on(vi->dev);
> >> netif_tx_wake_all_queues(vi->dev);
> >> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
> >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> >> VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >> + VIRTIO_NET_F_SPEED_DUPLEX
> >>
> >> static unsigned int features[] = {
> >> VIRTNET_FEATURES,
> >
> > Still missing the update from virtnet_config_changed_work, and I think
> > it's important to reflex host changes within guest when the
> > feature bit has been acked.
> >
>
> I update vi->speed and vi->duplex in virtnet_config_changed_work(). And
> I tested using the 'set_link' on/off from the qemu monitor.
> Specifically, an 'off' sets the speed and link to 'unknown', and an 'on'
> returns the speed and link to the configured speed and duplex. So they
> are being updated dynamically now. What host changes are you referring
> to that are not reflected?
>
> Thanks,
>
> -Jason
Ouch, I was reviewing an old version and replied to this one.
Sorry, will re-read now.
>
> >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> >> index fc353b5..5de6ed3 100644
> >> --- a/include/uapi/linux/virtio_net.h
> >> +++ b/include/uapi/linux/virtio_net.h
> >> @@ -57,6 +57,8 @@
> >> * Steering */
> >> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
> >>
> >> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
> >> +
> >> #ifndef VIRTIO_NET_NO_LEGACY
> >> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
> >> #endif /* VIRTIO_NET_NO_LEGACY */
> >> @@ -76,6 +78,17 @@ struct virtio_net_config {
> >> __u16 max_virtqueue_pairs;
> >> /* Default maximum transmit unit advice */
> >> __u16 mtu;
> >> + /*
> >> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
> >> + * Any other value stands for unknown.
> >> + */
> >> + __u32 speed;
> >> + /*
> >> + * 0x00 - half duplex
> >> + * 0x01 - full duplex
> >> + * Any other value stands for unknown.
> >> + */
> >> + __u8 duplex;
> >> } __attribute__((packed));
> >>
> >> /*
> >> --
> >> 2.6.1
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Michael S. Tsirkin @ 2018-01-04 17:05 UTC (permalink / raw)
To: Jason Baron; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <e185a03168d0ab68150d21770bf0d02c5ef537c2.1515041373.git.jbaron@akamai.com>
On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote:
> The ability to set speed and duplex for virtio_net is useful in various
> scenarios as described here:
>
> 16032be virtio_net: add ethtool support for set and get of settings
>
> However, it would be nice to be able to set this from the hypervisor,
> such that virtio_net doesn't require custom guest ethtool commands.
>
> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
> the hypervisor to export a linkspeed and duplex setting. The user can
> subsequently overwrite it later if desired via: 'ethtool -s'.
>
> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
> is that device feature bits are to grow down from bit 63, since the
> transports are starting from bit 24 and growing up.
>
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: virtio-dev@lists.oasis-open.org
> ---
> drivers/net/virtio_net.c | 19 ++++++++++++++++++-
> include/uapi/linux/virtio_net.h | 13 +++++++++++++
> 2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6fb7b65..0b2d314 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
>
> vi->status = v;
>
> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
BTW we can avoid this read for when link goes down.
Not a big deal but still.
> + u32 speed;
> + u8 duplex;
> +
> + speed = virtio_cread32(vi->vdev,
> + offsetof(struct virtio_net_config,
> + speed));
> + if (ethtool_validate_speed(speed))
> + vi->speed = speed;
> + duplex = virtio_cread8(vi->vdev,
> + offsetof(struct virtio_net_config,
> + duplex));
> + if (ethtool_validate_duplex(duplex))
> + vi->duplex = duplex;
> + }
> +
> if (vi->status & VIRTIO_NET_S_LINK_UP) {
> netif_carrier_on(vi->dev);
> netif_tx_wake_all_queues(vi->dev);
OK so this handles the case when VIRTIO_NET_F_STATUS is set,
but when it's clear we need to call this from virtnet_probe.
I propose moving this chunk to a function and calling from two places.
> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> VIRTIO_NET_F_CTRL_MAC_ADDR, \
> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> + VIRTIO_NET_F_SPEED_DUPLEX
>
> static unsigned int features[] = {
> VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index fc353b5..5de6ed3 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -57,6 +57,8 @@
> * Steering */
> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>
> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
> +
> #ifndef VIRTIO_NET_NO_LEGACY
> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
> #endif /* VIRTIO_NET_NO_LEGACY */
> @@ -76,6 +78,17 @@ struct virtio_net_config {
> __u16 max_virtqueue_pairs;
> /* Default maximum transmit unit advice */
> __u16 mtu;
> + /*
> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
> + * Any other value stands for unknown.
> + */
> + __u32 speed;
> + /*
> + * 0x00 - half duplex
> + * 0x01 - full duplex
> + * Any other value stands for unknown.
> + */
> + __u8 duplex;
> } __attribute__((packed));
>
> /*
> --
> 2.6.1
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Jason Baron via Virtualization @ 2018-01-04 18:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <20180104190309-mutt-send-email-mst@kernel.org>
On 01/04/2018 12:05 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote:
>> The ability to set speed and duplex for virtio_net is useful in various
>> scenarios as described here:
>>
>> 16032be virtio_net: add ethtool support for set and get of settings
>>
>> However, it would be nice to be able to set this from the hypervisor,
>> such that virtio_net doesn't require custom guest ethtool commands.
>>
>> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
>> the hypervisor to export a linkspeed and duplex setting. The user can
>> subsequently overwrite it later if desired via: 'ethtool -s'.
>>
>> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
>> is that device feature bits are to grow down from bit 63, since the
>> transports are starting from bit 24 and growing up.
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: virtio-dev@lists.oasis-open.org
>> ---
>> drivers/net/virtio_net.c | 19 ++++++++++++++++++-
>> include/uapi/linux/virtio_net.h | 13 +++++++++++++
>> 2 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 6fb7b65..0b2d314 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
>>
>> vi->status = v;
>>
>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
>
> BTW we can avoid this read for when link goes down.
> Not a big deal but still.
So you are saying that we can just set vi->speed and vi->duplex to
'unknown' when the link goes down and not check for the presence of
VIRTIO_NET_F_SPEED_DUPLEX?
If so, that could over-write what the user may have configured in the
guest via 'ethtool -s' when the link goes down, so that would be a
change in behavior, but perhaps that is ok?
I think I would prefer to have the link down event still check for
VIRTIO_NET_F_SPEED_DUPLEX before changing speed/duplex. That way we
still have 2 modes for updating the fields:
1) completely guest controlled. Same as we have now and host does not
change any values and does not set VIRTIO_NET_F_SPEED_DUPLEX flag (hence
don't remove above check).
2) if speed or duplex or speed is set in the qemu command line, then set
the VIRTIO_NET_F_SPEED_DUPLEX and have host control the settings of
speed/duplex (with ability of guest to over-write if it wanted to).
>
>> + u32 speed;
>> + u8 duplex;
>> +
>> + speed = virtio_cread32(vi->vdev,
>> + offsetof(struct virtio_net_config,
>> + speed));
>> + if (ethtool_validate_speed(speed))
>> + vi->speed = speed;
>> + duplex = virtio_cread8(vi->vdev,
>> + offsetof(struct virtio_net_config,
>> + duplex));
>> + if (ethtool_validate_duplex(duplex))
>> + vi->duplex = duplex;
>> + }
>> +
>> if (vi->status & VIRTIO_NET_S_LINK_UP) {
>> netif_carrier_on(vi->dev);
>> netif_tx_wake_all_queues(vi->dev);
>
> OK so this handles the case when VIRTIO_NET_F_STATUS is set,
> but when it's clear we need to call this from virtnet_probe.
>
> I propose moving this chunk to a function and calling from two places.
>
good point. will update.
Thanks,
-Jason
>
>> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>> VIRTIO_NET_F_CTRL_MAC_ADDR, \
>> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>> + VIRTIO_NET_F_SPEED_DUPLEX
>>
>> static unsigned int features[] = {
>> VIRTNET_FEATURES,
>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>> index fc353b5..5de6ed3 100644
>> --- a/include/uapi/linux/virtio_net.h
>> +++ b/include/uapi/linux/virtio_net.h
>> @@ -57,6 +57,8 @@
>> * Steering */
>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>>
>> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
>> +
>> #ifndef VIRTIO_NET_NO_LEGACY
>> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
>> #endif /* VIRTIO_NET_NO_LEGACY */
>> @@ -76,6 +78,17 @@ struct virtio_net_config {
>> __u16 max_virtqueue_pairs;
>> /* Default maximum transmit unit advice */
>> __u16 mtu;
>> + /*
>> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
>> + * Any other value stands for unknown.
>> + */
>> + __u32 speed;
>> + /*
>> + * 0x00 - half duplex
>> + * 0x01 - full duplex
>> + * Any other value stands for unknown.
>> + */
>> + __u8 duplex;
>> } __attribute__((packed));
>>
>> /*
>> --
>> 2.6.1
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Michael S. Tsirkin @ 2018-01-04 18:22 UTC (permalink / raw)
To: Jason Baron; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <c5e2f8d5-3961-606a-8c60-168f1465bd6e@akamai.com>
On Thu, Jan 04, 2018 at 01:12:30PM -0500, Jason Baron wrote:
>
>
> On 01/04/2018 12:05 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote:
> >> The ability to set speed and duplex for virtio_net is useful in various
> >> scenarios as described here:
> >>
> >> 16032be virtio_net: add ethtool support for set and get of settings
> >>
> >> However, it would be nice to be able to set this from the hypervisor,
> >> such that virtio_net doesn't require custom guest ethtool commands.
> >>
> >> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
> >> the hypervisor to export a linkspeed and duplex setting. The user can
> >> subsequently overwrite it later if desired via: 'ethtool -s'.
> >>
> >> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
> >> is that device feature bits are to grow down from bit 63, since the
> >> transports are starting from bit 24 and growing up.
> >>
> >> Signed-off-by: Jason Baron <jbaron@akamai.com>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Jason Wang <jasowang@redhat.com>
> >> Cc: virtio-dev@lists.oasis-open.org
> >> ---
> >> drivers/net/virtio_net.c | 19 ++++++++++++++++++-
> >> include/uapi/linux/virtio_net.h | 13 +++++++++++++
> >> 2 files changed, 31 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 6fb7b65..0b2d314 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
> >>
> >> vi->status = v;
> >>
> >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> >
> > BTW we can avoid this read for when link goes down.
> > Not a big deal but still.
>
> So you are saying that we can just set vi->speed and vi->duplex to
> 'unknown' when the link goes down and not check for the presence of
> VIRTIO_NET_F_SPEED_DUPLEX?
>
> If so, that could over-write what the user may have configured in the
> guest via 'ethtool -s' when the link goes down, so that would be a
> change in behavior, but perhaps that is ok?
No - what I am saying is that your patch overwrites the values
set by user when link goes down.
I suggest limiting this call to when
if (vi->status & VIRTIO_NET_S_LINK_UP)
and then the values are overwritten when link goes up
which seems closer to what a user might expect.
>
> I think I would prefer to have the link down event still check for
> VIRTIO_NET_F_SPEED_DUPLEX before changing speed/duplex. That way we
> still have 2 modes for updating the fields:
>
> 1) completely guest controlled. Same as we have now and host does not
> change any values and does not set VIRTIO_NET_F_SPEED_DUPLEX flag (hence
> don't remove above check).
>
> 2) if speed or duplex or speed is set in the qemu command line, then set
> the VIRTIO_NET_F_SPEED_DUPLEX and have host control the settings of
> speed/duplex (with ability of guest to over-write if it wanted to).
>
>
> >
> >> + u32 speed;
> >> + u8 duplex;
> >> +
> >> + speed = virtio_cread32(vi->vdev,
> >> + offsetof(struct virtio_net_config,
> >> + speed));
> >> + if (ethtool_validate_speed(speed))
> >> + vi->speed = speed;
> >> + duplex = virtio_cread8(vi->vdev,
> >> + offsetof(struct virtio_net_config,
> >> + duplex));
> >> + if (ethtool_validate_duplex(duplex))
> >> + vi->duplex = duplex;
> >> + }
> >> +
> >> if (vi->status & VIRTIO_NET_S_LINK_UP) {
> >> netif_carrier_on(vi->dev);
> >> netif_tx_wake_all_queues(vi->dev);
> >
> > OK so this handles the case when VIRTIO_NET_F_STATUS is set,
> > but when it's clear we need to call this from virtnet_probe.
> >
> > I propose moving this chunk to a function and calling from two places.
> >
>
> good point. will update.
>
> Thanks,
>
> -Jason
>
> >
> >> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
> >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> >> VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >> + VIRTIO_NET_F_SPEED_DUPLEX
> >>
> >> static unsigned int features[] = {
> >> VIRTNET_FEATURES,
> >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> >> index fc353b5..5de6ed3 100644
> >> --- a/include/uapi/linux/virtio_net.h
> >> +++ b/include/uapi/linux/virtio_net.h
> >> @@ -57,6 +57,8 @@
> >> * Steering */
> >> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
> >>
> >> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
> >> +
> >> #ifndef VIRTIO_NET_NO_LEGACY
> >> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
> >> #endif /* VIRTIO_NET_NO_LEGACY */
> >> @@ -76,6 +78,17 @@ struct virtio_net_config {
> >> __u16 max_virtqueue_pairs;
> >> /* Default maximum transmit unit advice */
> >> __u16 mtu;
> >> + /*
> >> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
> >> + * Any other value stands for unknown.
> >> + */
> >> + __u32 speed;
> >> + /*
> >> + * 0x00 - half duplex
> >> + * 0x01 - full duplex
> >> + * Any other value stands for unknown.
> >> + */
> >> + __u8 duplex;
> >> } __attribute__((packed));
> >>
> >> /*
> >> --
> >> 2.6.1
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Michael S. Tsirkin @ 2018-01-04 18:23 UTC (permalink / raw)
To: Jason Baron; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <20180104201934-mutt-send-email-mst@kernel.org>
On Thu, Jan 04, 2018 at 08:22:08PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 04, 2018 at 01:12:30PM -0500, Jason Baron wrote:
> >
> >
> > On 01/04/2018 12:05 PM, Michael S. Tsirkin wrote:
> > > On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote:
> > >> The ability to set speed and duplex for virtio_net is useful in various
> > >> scenarios as described here:
> > >>
> > >> 16032be virtio_net: add ethtool support for set and get of settings
> > >>
> > >> However, it would be nice to be able to set this from the hypervisor,
> > >> such that virtio_net doesn't require custom guest ethtool commands.
> > >>
> > >> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
> > >> the hypervisor to export a linkspeed and duplex setting. The user can
> > >> subsequently overwrite it later if desired via: 'ethtool -s'.
> > >>
> > >> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
> > >> is that device feature bits are to grow down from bit 63, since the
> > >> transports are starting from bit 24 and growing up.
> > >>
> > >> Signed-off-by: Jason Baron <jbaron@akamai.com>
> > >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > >> Cc: Jason Wang <jasowang@redhat.com>
> > >> Cc: virtio-dev@lists.oasis-open.org
> > >> ---
> > >> drivers/net/virtio_net.c | 19 ++++++++++++++++++-
> > >> include/uapi/linux/virtio_net.h | 13 +++++++++++++
> > >> 2 files changed, 31 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > >> index 6fb7b65..0b2d314 100644
> > >> --- a/drivers/net/virtio_net.c
> > >> +++ b/drivers/net/virtio_net.c
> > >> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
> > >>
> > >> vi->status = v;
> > >>
> > >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> > >
> > > BTW we can avoid this read for when link goes down.
> > > Not a big deal but still.
> >
> > So you are saying that we can just set vi->speed and vi->duplex to
> > 'unknown' when the link goes down and not check for the presence of
> > VIRTIO_NET_F_SPEED_DUPLEX?
> >
> > If so, that could over-write what the user may have configured in the
> > guest via 'ethtool -s' when the link goes down, so that would be a
> > change in behavior, but perhaps that is ok?
>
> No - what I am saying is that your patch overwrites the values
> set by user when link goes down.
>
> I suggest limiting this call to when
>
> if (vi->status & VIRTIO_NET_S_LINK_UP)
>
> and then the values are overwritten when link goes up
> which seems closer to what a user might expect.
>
> >
> > I think I would prefer to have the link down event still check for
> > VIRTIO_NET_F_SPEED_DUPLEX before changing speed/duplex. That way we
> > still have 2 modes for updating the fields:
> >
> > 1) completely guest controlled. Same as we have now and host does not
> > change any values and does not set VIRTIO_NET_F_SPEED_DUPLEX flag (hence
> > don't remove above check).
> >
> > 2) if speed or duplex or speed is set in the qemu command line, then set
> > the VIRTIO_NET_F_SPEED_DUPLEX and have host control the settings of
> > speed/duplex (with ability of guest to over-write if it wanted to).
I agree - I don't see a reason to touch the speed/duplex values when
VIRTIO_NET_F_SPEED_DUPLEX has not been negotiated.
> >
> >
> > >
> > >> + u32 speed;
> > >> + u8 duplex;
> > >> +
> > >> + speed = virtio_cread32(vi->vdev,
> > >> + offsetof(struct virtio_net_config,
> > >> + speed));
> > >> + if (ethtool_validate_speed(speed))
> > >> + vi->speed = speed;
> > >> + duplex = virtio_cread8(vi->vdev,
> > >> + offsetof(struct virtio_net_config,
> > >> + duplex));
> > >> + if (ethtool_validate_duplex(duplex))
> > >> + vi->duplex = duplex;
> > >> + }
> > >> +
> > >> if (vi->status & VIRTIO_NET_S_LINK_UP) {
> > >> netif_carrier_on(vi->dev);
> > >> netif_tx_wake_all_queues(vi->dev);
> > >
> > > OK so this handles the case when VIRTIO_NET_F_STATUS is set,
> > > but when it's clear we need to call this from virtnet_probe.
> > >
> > > I propose moving this chunk to a function and calling from two places.
> > >
> >
> > good point. will update.
> >
> > Thanks,
> >
> > -Jason
> >
> > >
> > >> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
> > >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> > >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> > >> VIRTIO_NET_F_CTRL_MAC_ADDR, \
> > >> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> > >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> > >> + VIRTIO_NET_F_SPEED_DUPLEX
> > >>
> > >> static unsigned int features[] = {
> > >> VIRTNET_FEATURES,
> > >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> > >> index fc353b5..5de6ed3 100644
> > >> --- a/include/uapi/linux/virtio_net.h
> > >> +++ b/include/uapi/linux/virtio_net.h
> > >> @@ -57,6 +57,8 @@
> > >> * Steering */
> > >> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
> > >>
> > >> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
> > >> +
> > >> #ifndef VIRTIO_NET_NO_LEGACY
> > >> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
> > >> #endif /* VIRTIO_NET_NO_LEGACY */
> > >> @@ -76,6 +78,17 @@ struct virtio_net_config {
> > >> __u16 max_virtqueue_pairs;
> > >> /* Default maximum transmit unit advice */
> > >> __u16 mtu;
> > >> + /*
> > >> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
> > >> + * Any other value stands for unknown.
> > >> + */
> > >> + __u32 speed;
> > >> + /*
> > >> + * 0x00 - half duplex
> > >> + * 0x01 - full duplex
> > >> + * Any other value stands for unknown.
> > >> + */
> > >> + __u8 duplex;
> > >> } __attribute__((packed));
> > >>
> > >> /*
> > >> --
> > >> 2.6.1
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Jason Baron via Virtualization @ 2018-01-04 19:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <20180104201934-mutt-send-email-mst@kernel.org>
On 01/04/2018 01:22 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 04, 2018 at 01:12:30PM -0500, Jason Baron wrote:
>>
>>
>> On 01/04/2018 12:05 PM, Michael S. Tsirkin wrote:
>>> On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote:
>>>> The ability to set speed and duplex for virtio_net is useful in various
>>>> scenarios as described here:
>>>>
>>>> 16032be virtio_net: add ethtool support for set and get of settings
>>>>
>>>> However, it would be nice to be able to set this from the hypervisor,
>>>> such that virtio_net doesn't require custom guest ethtool commands.
>>>>
>>>> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
>>>> the hypervisor to export a linkspeed and duplex setting. The user can
>>>> subsequently overwrite it later if desired via: 'ethtool -s'.
>>>>
>>>> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
>>>> is that device feature bits are to grow down from bit 63, since the
>>>> transports are starting from bit 24 and growing up.
>>>>
>>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> Cc: virtio-dev@lists.oasis-open.org
>>>> ---
>>>> drivers/net/virtio_net.c | 19 ++++++++++++++++++-
>>>> include/uapi/linux/virtio_net.h | 13 +++++++++++++
>>>> 2 files changed, 31 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 6fb7b65..0b2d314 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work)
>>>>
>>>> vi->status = v;
>>>>
>>>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
>>>
>>> BTW we can avoid this read for when link goes down.
>>> Not a big deal but still.
>>
>> So you are saying that we can just set vi->speed and vi->duplex to
>> 'unknown' when the link goes down and not check for the presence of
>> VIRTIO_NET_F_SPEED_DUPLEX?
>>
>> If so, that could over-write what the user may have configured in the
>> guest via 'ethtool -s' when the link goes down, so that would be a
>> change in behavior, but perhaps that is ok?
>
> No - what I am saying is that your patch overwrites the values
> set by user when link goes down.
>
> I suggest limiting this call to when
>
> if (vi->status & VIRTIO_NET_S_LINK_UP)
>
> and then the values are overwritten when link goes up
> which seems closer to what a user might expect.
ok, will update this for v4.
Thanks,
-Jason
>
>>
>> I think I would prefer to have the link down event still check for
>> VIRTIO_NET_F_SPEED_DUPLEX before changing speed/duplex. That way we
>> still have 2 modes for updating the fields:
>>
>> 1) completely guest controlled. Same as we have now and host does not
>> change any values and does not set VIRTIO_NET_F_SPEED_DUPLEX flag (hence
>> don't remove above check).
>>
>> 2) if speed or duplex or speed is set in the qemu command line, then set
>> the VIRTIO_NET_F_SPEED_DUPLEX and have host control the settings of
>> speed/duplex (with ability of guest to over-write if it wanted to).
>>
>>
>>>
>>>> + u32 speed;
>>>> + u8 duplex;
>>>> +
>>>> + speed = virtio_cread32(vi->vdev,
>>>> + offsetof(struct virtio_net_config,
>>>> + speed));
>>>> + if (ethtool_validate_speed(speed))
>>>> + vi->speed = speed;
>>>> + duplex = virtio_cread8(vi->vdev,
>>>> + offsetof(struct virtio_net_config,
>>>> + duplex));
>>>> + if (ethtool_validate_duplex(duplex))
>>>> + vi->duplex = duplex;
>>>> + }
>>>> +
>>>> if (vi->status & VIRTIO_NET_S_LINK_UP) {
>>>> netif_carrier_on(vi->dev);
>>>> netif_tx_wake_all_queues(vi->dev);
>>>
>>> OK so this handles the case when VIRTIO_NET_F_STATUS is set,
>>> but when it's clear we need to call this from virtnet_probe.
>>>
>>> I propose moving this chunk to a function and calling from two places.
>>>
>>
>> good point. will update.
>>
>> Thanks,
>>
>> -Jason
>>
>>>
>>>> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = {
>>>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>>>> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>>>> VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>> + VIRTIO_NET_F_SPEED_DUPLEX
>>>>
>>>> static unsigned int features[] = {
>>>> VIRTNET_FEATURES,
>>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>>> index fc353b5..5de6ed3 100644
>>>> --- a/include/uapi/linux/virtio_net.h
>>>> +++ b/include/uapi/linux/virtio_net.h
>>>> @@ -57,6 +57,8 @@
>>>> * Steering */
>>>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>>>>
>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
>>>> +
>>>> #ifndef VIRTIO_NET_NO_LEGACY
>>>> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
>>>> #endif /* VIRTIO_NET_NO_LEGACY */
>>>> @@ -76,6 +78,17 @@ struct virtio_net_config {
>>>> __u16 max_virtqueue_pairs;
>>>> /* Default maximum transmit unit advice */
>>>> __u16 mtu;
>>>> + /*
>>>> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
>>>> + * Any other value stands for unknown.
>>>> + */
>>>> + __u32 speed;
>>>> + /*
>>>> + * 0x00 - half duplex
>>>> + * 0x01 - full duplex
>>>> + * Any other value stands for unknown.
>>>> + */
>>>> + __u8 duplex;
>>>> } __attribute__((packed));
>>>>
>>>> /*
>>>> --
>>>> 2.6.1
^ permalink raw reply
* [PATCH v2] vhost: remove unused lock check flag in vhost_dev_cleanup()
From: 夷则(Caspar) @ 2018-01-05 9:39 UTC (permalink / raw)
To: Michael S . Tsirkin, Jason Wang, Stefan Hajnoczi
Cc: 夷则(Caspar), linux-kernel, kvm, virtualization
In-Reply-To: <57a32eb1fd105b2aacc96b7caae0d8336c010fe0.1514130393.git.jinli.zjl@alibaba-inc.com>
From: Caspar Zhang <jinli.zjl@alibaba-inc.com>
In commit ea5d404655ba ("vhost: fix release path lockdep checks"),
Michael added a flag to check whether we should hold a lock in
vhost_dev_cleanup(), however, in commit 47283bef7ed3 ("vhost: move
memory pointer to VQs"), RCU operations have been replaced by
mutex, we can remove the no-longer-used `locked' parameter now.
Signed-off-by: Caspar Zhang <jinli.zjl@alibaba-inc.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
v1->v2: fix author name with UTF-8 characters.
drivers/vhost/net.c | 2 +-
drivers/vhost/scsi.c | 2 +-
drivers/vhost/test.c | 2 +-
drivers/vhost/vhost.c | 5 ++---
drivers/vhost/vhost.h | 2 +-
drivers/vhost/vsock.c | 2 +-
6 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index c7bdeb655646..a354d8d731e3 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -996,7 +996,7 @@ static int vhost_net_release(struct inode *inode, struct file *f)
vhost_net_stop(n, &tx_sock, &rx_sock);
vhost_net_flush(n);
vhost_dev_stop(&n->dev);
- vhost_dev_cleanup(&n->dev, false);
+ vhost_dev_cleanup(&n->dev);
vhost_net_vq_reset(n);
if (tx_sock)
sockfd_put(tx_sock);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 71517b3c5558..797d08916553 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1420,7 +1420,7 @@ static int vhost_scsi_release(struct inode *inode, struct file *f)
mutex_unlock(&vs->dev.mutex);
vhost_scsi_clear_endpoint(vs, &t);
vhost_dev_stop(&vs->dev);
- vhost_dev_cleanup(&vs->dev, false);
+ vhost_dev_cleanup(&vs->dev);
/* Jobs can re-queue themselves in evt kick handler. Do extra flush. */
vhost_scsi_flush(vs);
kfree(vs->dev.vqs);
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index 3cc98c07dcd3..906b8f0f19f7 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -157,7 +157,7 @@ static int vhost_test_release(struct inode *inode, struct file *f)
vhost_test_stop(n, &private);
vhost_test_flush(n);
- vhost_dev_cleanup(&n->dev, false);
+ vhost_dev_cleanup(&n->dev);
/* We do an extra flush before freeing memory,
* since jobs can re-queue themselves. */
vhost_test_flush(n);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 33ac2b186b85..014675c3d569 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -544,7 +544,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_umem *umem)
{
int i;
- vhost_dev_cleanup(dev, true);
+ vhost_dev_cleanup(dev);
/* Restore memory to default empty mapping. */
INIT_LIST_HEAD(&umem->umem_list);
@@ -611,8 +611,7 @@ static void vhost_clear_msg(struct vhost_dev *dev)
spin_unlock(&dev->iotlb_lock);
}
-/* Caller should have device mutex if and only if locked is set */
-void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
+void vhost_dev_cleanup(struct vhost_dev *dev)
{
int i;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 79c6e7a60a5e..ff4d918e3e0a 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -181,7 +181,7 @@ bool vhost_dev_has_owner(struct vhost_dev *dev);
long vhost_dev_check_owner(struct vhost_dev *);
struct vhost_umem *vhost_dev_reset_owner_prepare(void);
void vhost_dev_reset_owner(struct vhost_dev *, struct vhost_umem *);
-void vhost_dev_cleanup(struct vhost_dev *, bool locked);
+void vhost_dev_cleanup(struct vhost_dev *);
void vhost_dev_stop(struct vhost_dev *);
long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user *argp);
long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp);
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 5a5e981bd8e4..0d14e2ff19f1 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -599,7 +599,7 @@ static int vhost_vsock_dev_release(struct inode *inode, struct file *file)
}
spin_unlock_bh(&vsock->send_pkt_list_lock);
- vhost_dev_cleanup(&vsock->dev, false);
+ vhost_dev_cleanup(&vsock->dev);
kfree(vsock->dev.vqs);
vhost_vsock_free(vsock);
return 0;
--
2.15.1
^ permalink raw reply related
* [PATCH v4 0/3] virtio_net: allow hypervisor to indicate linkspeed and duplex setting
From: Jason Baron via Virtualization @ 2018-01-05 22:44 UTC (permalink / raw)
To: mst; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
We have found it useful to be able to set the linkspeed and duplex
settings from the host-side for virtio_net. This obviates the need
for guest changes and settings for these fields, and does not require
custom ethtool commands for virtio_net.
The ability to set linkspeed and duplex is useful in various cases
as described here:
16032be virtio_net: add ethtool support for set and get of settings
Using 'ethtool -s' continues to over-write the linkspeed/duplex
settings with this patch.
The 1/3 patch is against net-next, while the 2-3/3 patch are the associated
qemu changes that would go in after as update-linux-headers.sh should
be run first. So the qemu patches are a demonstration of how I intend this
to work.
Thanks,
-Jason
linux changes:
changes from v3:
* break the speed/duplex read into a function and also call from virtnet_probe
when status bit is not negotiated
* only do speed/duplex read in virtnet_config_changed_work() on LINK_UP
changes from v2:
* move speed/duplex read into virtnet_config_changed_work() so link up changes
are detected
Jason Baron (1):
virtio_net: propagate linkspeed/duplex settings from the hypervisor
drivers/net/virtio_net.c | 23 ++++++++++++++++++++++-
include/uapi/linux/virtio_net.h | 13 +++++++++++++
2 files changed, 35 insertions(+), 1 deletion(-)
qemu changes:
Jason Baron (2):
qemu: virtio-net: use 64-bit values for feature flags
qemu: add linkspeed and duplex settings to virtio-net
hw/net/virtio-net.c | 87 ++++++++++++++++++++---------
include/hw/virtio/virtio-net.h | 5 +-
include/standard-headers/linux/virtio_net.h | 13 +++++
3 files changed, 77 insertions(+), 28 deletions(-)
--
2.6.1
^ permalink raw reply
* [PATCH v4 2/3] qemu: virtio-net: use 64-bit values for feature flags
From: Jason Baron via Virtualization @ 2018-01-05 22:44 UTC (permalink / raw)
To: mst; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <cover.1515190238.git.jbaron@akamai.com>
In prepartion for using some of the high order feature bits, make sure that
virtio-net uses 64-bit values everywhere.
Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: virtio-dev@lists.oasis-open.org
---
hw/net/virtio-net.c | 55 +++++++++++++++++++++---------------------
include/hw/virtio/virtio-net.h | 2 +-
2 files changed, 29 insertions(+), 28 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 38674b0..54823af 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -48,18 +48,18 @@
(offsetof(container, field) + sizeof(((container *)0)->field))
typedef struct VirtIOFeature {
- uint32_t flags;
+ uint64_t flags;
size_t end;
} VirtIOFeature;
static VirtIOFeature feature_sizes[] = {
- {.flags = 1 << VIRTIO_NET_F_MAC,
+ {.flags = 1ULL << VIRTIO_NET_F_MAC,
.end = endof(struct virtio_net_config, mac)},
- {.flags = 1 << VIRTIO_NET_F_STATUS,
+ {.flags = 1ULL << VIRTIO_NET_F_STATUS,
.end = endof(struct virtio_net_config, status)},
- {.flags = 1 << VIRTIO_NET_F_MQ,
+ {.flags = 1ULL << VIRTIO_NET_F_MQ,
.end = endof(struct virtio_net_config, max_virtqueue_pairs)},
- {.flags = 1 << VIRTIO_NET_F_MTU,
+ {.flags = 1ULL << VIRTIO_NET_F_MTU,
.end = endof(struct virtio_net_config, mtu)},
{}
};
@@ -1938,7 +1938,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
int i;
if (n->net_conf.mtu) {
- n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
+ n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
}
virtio_net_set_config_size(n, n->host_features);
@@ -2109,45 +2109,46 @@ static const VMStateDescription vmstate_virtio_net = {
};
static Property virtio_net_properties[] = {
- DEFINE_PROP_BIT("csum", VirtIONet, host_features, VIRTIO_NET_F_CSUM, true),
- DEFINE_PROP_BIT("guest_csum", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("csum", VirtIONet, host_features,
+ VIRTIO_NET_F_CSUM, true),
+ DEFINE_PROP_BIT64("guest_csum", VirtIONet, host_features,
VIRTIO_NET_F_GUEST_CSUM, true),
- DEFINE_PROP_BIT("gso", VirtIONet, host_features, VIRTIO_NET_F_GSO, true),
- DEFINE_PROP_BIT("guest_tso4", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("gso", VirtIONet, host_features, VIRTIO_NET_F_GSO, true),
+ DEFINE_PROP_BIT64("guest_tso4", VirtIONet, host_features,
VIRTIO_NET_F_GUEST_TSO4, true),
- DEFINE_PROP_BIT("guest_tso6", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("guest_tso6", VirtIONet, host_features,
VIRTIO_NET_F_GUEST_TSO6, true),
- DEFINE_PROP_BIT("guest_ecn", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("guest_ecn", VirtIONet, host_features,
VIRTIO_NET_F_GUEST_ECN, true),
- DEFINE_PROP_BIT("guest_ufo", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("guest_ufo", VirtIONet, host_features,
VIRTIO_NET_F_GUEST_UFO, true),
- DEFINE_PROP_BIT("guest_announce", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("guest_announce", VirtIONet, host_features,
VIRTIO_NET_F_GUEST_ANNOUNCE, true),
- DEFINE_PROP_BIT("host_tso4", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("host_tso4", VirtIONet, host_features,
VIRTIO_NET_F_HOST_TSO4, true),
- DEFINE_PROP_BIT("host_tso6", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("host_tso6", VirtIONet, host_features,
VIRTIO_NET_F_HOST_TSO6, true),
- DEFINE_PROP_BIT("host_ecn", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("host_ecn", VirtIONet, host_features,
VIRTIO_NET_F_HOST_ECN, true),
- DEFINE_PROP_BIT("host_ufo", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("host_ufo", VirtIONet, host_features,
VIRTIO_NET_F_HOST_UFO, true),
- DEFINE_PROP_BIT("mrg_rxbuf", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("mrg_rxbuf", VirtIONet, host_features,
VIRTIO_NET_F_MRG_RXBUF, true),
- DEFINE_PROP_BIT("status", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("status", VirtIONet, host_features,
VIRTIO_NET_F_STATUS, true),
- DEFINE_PROP_BIT("ctrl_vq", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("ctrl_vq", VirtIONet, host_features,
VIRTIO_NET_F_CTRL_VQ, true),
- DEFINE_PROP_BIT("ctrl_rx", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("ctrl_rx", VirtIONet, host_features,
VIRTIO_NET_F_CTRL_RX, true),
- DEFINE_PROP_BIT("ctrl_vlan", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("ctrl_vlan", VirtIONet, host_features,
VIRTIO_NET_F_CTRL_VLAN, true),
- DEFINE_PROP_BIT("ctrl_rx_extra", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("ctrl_rx_extra", VirtIONet, host_features,
VIRTIO_NET_F_CTRL_RX_EXTRA, true),
- DEFINE_PROP_BIT("ctrl_mac_addr", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("ctrl_mac_addr", VirtIONet, host_features,
VIRTIO_NET_F_CTRL_MAC_ADDR, true),
- DEFINE_PROP_BIT("ctrl_guest_offloads", VirtIONet, host_features,
+ DEFINE_PROP_BIT64("ctrl_guest_offloads", VirtIONet, host_features,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, true),
- DEFINE_PROP_BIT("mq", VirtIONet, host_features, VIRTIO_NET_F_MQ, false),
+ DEFINE_PROP_BIT64("mq", VirtIONet, host_features, VIRTIO_NET_F_MQ, false),
DEFINE_NIC_PROPERTIES(VirtIONet, nic_conf),
DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
TX_TIMER_INTERVAL),
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index b81b6a4..e7634c9 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -67,7 +67,7 @@ typedef struct VirtIONet {
uint32_t has_vnet_hdr;
size_t host_hdr_len;
size_t guest_hdr_len;
- uint32_t host_features;
+ uint64_t host_features;
uint8_t has_ufo;
uint32_t mergeable_rx_bufs;
uint8_t promisc;
--
2.6.1
^ permalink raw reply related
* [PATCH net-next v4 1/3] virtio_net: propagate linkspeed/duplex settings from the hypervisor
From: Jason Baron via Virtualization @ 2018-01-05 22:44 UTC (permalink / raw)
To: mst; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <cover.1515190238.git.jbaron@akamai.com>
The ability to set speed and duplex for virtio_net is useful in various
scenarios as described here:
16032be virtio_net: add ethtool support for set and get of settings
However, it would be nice to be able to set this from the hypervisor,
such that virtio_net doesn't require custom guest ethtool commands.
Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
the hypervisor to export a linkspeed and duplex setting. The user can
subsequently overwrite it later if desired via: 'ethtool -s'.
Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention
is that device feature bits are to grow down from bit 63, since the
transports are starting from bit 24 and growing up.
Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: virtio-dev@lists.oasis-open.org
---
drivers/net/virtio_net.c | 23 ++++++++++++++++++++++-
include/uapi/linux/virtio_net.h | 13 +++++++++++++
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6fb7b65..4f27508 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1894,6 +1894,24 @@ static void virtnet_init_settings(struct net_device *dev)
vi->duplex = DUPLEX_UNKNOWN;
}
+static void virtnet_update_settings(struct virtnet_info *vi)
+{
+ u32 speed;
+ u8 duplex;
+
+ if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX))
+ return;
+
+ speed = virtio_cread32(vi->vdev, offsetof(struct virtio_net_config,
+ speed));
+ if (ethtool_validate_speed(speed))
+ vi->speed = speed;
+ duplex = virtio_cread8(vi->vdev, offsetof(struct virtio_net_config,
+ duplex));
+ if (ethtool_validate_duplex(duplex))
+ vi->duplex = duplex;
+}
+
static const struct ethtool_ops virtnet_ethtool_ops = {
.get_drvinfo = virtnet_get_drvinfo,
.get_link = ethtool_op_get_link,
@@ -2147,6 +2165,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
vi->status = v;
if (vi->status & VIRTIO_NET_S_LINK_UP) {
+ virtnet_update_settings(vi);
netif_carrier_on(vi->dev);
netif_tx_wake_all_queues(vi->dev);
} else {
@@ -2695,6 +2714,7 @@ static int virtnet_probe(struct virtio_device *vdev)
schedule_work(&vi->config_work);
} else {
vi->status = VIRTIO_NET_S_LINK_UP;
+ virtnet_update_settings(vi);
netif_carrier_on(dev);
}
@@ -2796,7 +2816,8 @@ static struct virtio_device_id id_table[] = {
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
VIRTIO_NET_F_CTRL_MAC_ADDR, \
- VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
+ VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
+ VIRTIO_NET_F_SPEED_DUPLEX
static unsigned int features[] = {
VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index fc353b5..5de6ed3 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -57,6 +57,8 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
+
#ifndef VIRTIO_NET_NO_LEGACY
#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
#endif /* VIRTIO_NET_NO_LEGACY */
@@ -76,6 +78,17 @@ struct virtio_net_config {
__u16 max_virtqueue_pairs;
/* Default maximum transmit unit advice */
__u16 mtu;
+ /*
+ * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
+ * Any other value stands for unknown.
+ */
+ __u32 speed;
+ /*
+ * 0x00 - half duplex
+ * 0x01 - full duplex
+ * Any other value stands for unknown.
+ */
+ __u8 duplex;
} __attribute__((packed));
/*
--
2.6.1
^ permalink raw reply related
* [PATCH v4 3/3] qemu: add linkspeed and duplex settings to virtio-net
From: Jason Baron via Virtualization @ 2018-01-05 22:44 UTC (permalink / raw)
To: mst; +Cc: virtio-dev, netdev, qemu-devel, virtualization, davem
In-Reply-To: <cover.1515190238.git.jbaron@akamai.com>
Although linkspeed and duplex can be set in a linux guest via 'ethtool -s',
this requires custom ethtool commands for virtio-net by default.
Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
the hypervisor to export a linkspeed and duplex setting. The user can
subsequently overwrite it later if desired via: 'ethtool -s'.
Linkspeed and duplex settings can be set as:
'-device virtio-net,speed=10000,duplex=full'
where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: virtio-dev@lists.oasis-open.org
---
hw/net/virtio-net.c | 32 +++++++++++++++++++++++++++++
include/hw/virtio/virtio-net.h | 3 +++
include/standard-headers/linux/virtio_net.h | 13 ++++++++++++
3 files changed, 48 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 54823af..cd63659 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -40,6 +40,12 @@
#define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
#define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
+/* duplex and speed */
+#define DUPLEX_UNKNOWN 0xff
+#define DUPLEX_HALF 0x00
+#define DUPLEX_FULL 0x01
+#define SPEED_UNKNOWN -1
+
/*
* Calculate the number of bytes up to and including the given 'field' of
* 'container'.
@@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
.end = endof(struct virtio_net_config, max_virtqueue_pairs)},
{.flags = 1ULL << VIRTIO_NET_F_MTU,
.end = endof(struct virtio_net_config, mtu)},
+ {.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX,
+ .end = endof(struct virtio_net_config, duplex)},
{}
};
@@ -89,6 +97,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
memcpy(netcfg.mac, n->mac, ETH_ALEN);
+ virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
+ netcfg.duplex = n->net_conf.duplex;
memcpy(config, &netcfg, n->config_size);
}
@@ -1941,6 +1951,26 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
}
+ if (n->net_conf.duplex_str) {
+ if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
+ n->net_conf.duplex = DUPLEX_HALF;
+ } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
+ n->net_conf.duplex = DUPLEX_FULL;
+ } else {
+ error_setg(errp, "'duplex' must be 'half' or 'full'");
+ }
+ n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
+ } else {
+ n->net_conf.duplex = DUPLEX_UNKNOWN;
+ }
+
+ if (n->net_conf.speed < SPEED_UNKNOWN) {
+ error_setg(errp, "'speed' must be between -1 (SPEED_UNKOWN) and "
+ "INT_MAX");
+ } else if (n->net_conf.speed >= 0) {
+ n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
+ }
+
virtio_net_set_config_size(n, n->host_features);
virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
@@ -2161,6 +2191,8 @@ static Property virtio_net_properties[] = {
DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
true),
+ DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
+ DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index e7634c9..02484dc 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -38,6 +38,9 @@ typedef struct virtio_net_conf
uint16_t rx_queue_size;
uint16_t tx_queue_size;
uint16_t mtu;
+ int32_t speed;
+ char *duplex_str;
+ uint8_t duplex;
} virtio_net_conf;
/* Maximum packet size we can receive from tap device: header + 64k */
diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
index 30ff249..17c8531 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -57,6 +57,8 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
+
#ifndef VIRTIO_NET_NO_LEGACY
#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
#endif /* VIRTIO_NET_NO_LEGACY */
@@ -76,6 +78,17 @@ struct virtio_net_config {
uint16_t max_virtqueue_pairs;
/* Default maximum transmit unit advice */
uint16_t mtu;
+ /*
+ * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
+ * Any other value stands for unknown.
+ */
+ uint32_t speed;
+ /*
+ * 0x00 - half duplex
+ * 0x01 - full duplex
+ * Any other value stands for unknown.
+ */
+ uint8_t duplex;
} QEMU_PACKED;
/*
diff --git a/pixman b/pixman
new file mode 160000
index 0000000..87eea99
--- /dev/null
+++ b/pixman
@@ -0,0 +1 @@
+Subproject commit 87eea99e443b389c978cf37efc52788bf03a0ee0
--
2.6.1
^ permalink raw reply related
* Re: [PATCH] [PATCH] virtio: make VIRTIO a menuconfig to ease disabling it all
From: Randy Dunlap @ 2018-01-07 3:25 UTC (permalink / raw)
To: Vincent Legoll, mst, jasowang, virtualization, linux-kernel
In-Reply-To: <20180103094918.6106-2-vincent.legoll@gmail.com>
On 01/03/18 01:49, Vincent Legoll wrote:
> No need to get into the submenu to disable all VIRTIO-related
> config entries.
>
> This makes it easier to disable all VIRTIO config options
> without entering the submenu. It will also enable one
> to see that en/dis-abled state from the outside menu.
>
> This is only intended to change menuconfig UI, not change
> the config dependencies.
>
> v2: add "default y" to avoid breaking existing configs
>
> Signed-off-by: Vincent Legoll <vincent.legoll@gmail.com>
For a single patch (not 2 or more in a series), please just use one
email with the patch description etc. in it. No need for a cover letter.
> ---
> drivers/virtio/Kconfig | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index cff773f15b7e..290a1875e1d3 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -5,7 +5,11 @@ config VIRTIO
> bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG
> or CONFIG_S390_GUEST.
>
> -menu "Virtio drivers"
> +menuconfig VIRTIO_MENU
> + bool "Virtio drivers"
> + default y
The 2 lines above should be indented only with 1 tab. They should not line up
with the help text above (help text is indented more than other Kconfig lines).
After that little style thing is fixed, you can add:
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org> # works for me
and even though this will disable the drivers that are listed immediately
inside this if/endif block, there are several other drivers that select VIRTIO,
so it can be slightly tricky to figure out what is causing CONFIG_VIRTIO
to be enabled after having disabled CONFIG_VIRTIO_MENU.
Thanks.
> +
> +if VIRTIO_MENU
>
> config VIRTIO_PCI
> tristate "PCI driver for virtio devices"
> @@ -79,4 +83,4 @@ config VIRTIO_MMIO_CMDLINE_DEVICES
>
> If unsure, say 'N'.
>
> -endmenu
> +endif # VIRTIO_MENU
>
--
~Randy
^ permalink raw reply
* [PATCH] virtio: make VIRTIO a menuconfig to ease disabling it all
From: Vincent Legoll @ 2018-01-07 11:33 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, virtualization, linux-kernel
Cc: Vincent Legoll, Randy Dunlap
In-Reply-To: <459e2097-f20a-18f8-17d9-2d9ed011211d@infradead.org>
No need to get into the submenu to disable all VIRTIO-related
config entries.
This makes it easier to disable all VIRTIO config options
without entering the submenu. It will also enable one
to see that en/dis-abled state from the outside menu.
This is only intended to change menuconfig UI, not change
the config dependencies.
v2: Added "default y" to avoid breaking existing configs
v3: Fixed wrong indentation, added *-by from Randy
Signed-off-by: Vincent Legoll <vincent.legoll@gmail.com>
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org> # works for me
---
drivers/virtio/Kconfig | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index cff773f15b7e..35897649c24f 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -5,7 +5,11 @@ config VIRTIO
bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG
or CONFIG_S390_GUEST.
-menu "Virtio drivers"
+menuconfig VIRTIO_MENU
+ bool "Virtio drivers"
+ default y
+
+if VIRTIO_MENU
config VIRTIO_PCI
tristate "PCI driver for virtio devices"
@@ -79,4 +83,4 @@ config VIRTIO_MMIO_CMDLINE_DEVICES
If unsure, say 'N'.
-endmenu
+endif # VIRTIO_MENU
--
2.14.1
^ permalink raw reply related
* Re: [RFC PATCH] virtio: add space for device features show
From: Cornelia Huck @ 2018-01-08 12:50 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <20171226133539.GA15965@localhost.didichuxing.com>
On Tue, 26 Dec 2017 21:35:44 +0800
weiping zhang <zhangweiping@didichuxing.com> wrote:
> make features string more readable, add space every 8bit.
> example:
> 00101010 01110000 00000000 00001100
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/virtio/virtio.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 59e36ef..d7d2db1 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -50,9 +50,12 @@ static ssize_t features_show(struct device *_d,
>
> /* We actually represent this as a bitstring, as it could be
> * arbitrary length in future. */
> - for (i = 0; i < sizeof(dev->features)*8; i++)
> - len += sprintf(buf+len, "%c",
> + for (i = 0; i < sizeof(dev->features) * 8; i++) {
> + len += sprintf(buf + len, "%c",
> __virtio_test_bit(dev, i) ? '1' : '0');
> + if (i % 8 == 7)
> + len += sprintf(buf + len, " ");
> + }
> len += sprintf(buf+len, "\n");
> return len;
> }
This might break consumers of this attribute.
I think the better approach is to write a userspace tool that can do
additional helpful stuff (like decoding the feature bits).
^ permalink raw reply
* [PATCH] vhost: Remove the unused variable.
From: Tonghao Zhang @ 2018-01-09 1:46 UTC (permalink / raw)
To: jasowang, virtualization
The patch (7235acdb1) changed the way of the work
flushing in which the queued seq, done seq, and the
flushing are not used anymore. Then remove them now.
Fixes: 7235acdb1 ("vhost: simplify work flushing")
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
drivers/vhost/vhost.c | 1 -
drivers/vhost/vhost.h | 4 ----
2 files changed, 5 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 33ac2b186b85..9b04cad91d65 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -181,7 +181,6 @@ void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn)
{
clear_bit(VHOST_WORK_QUEUED, &work->flags);
work->fn = fn;
- init_waitqueue_head(&work->done);
}
EXPORT_SYMBOL_GPL(vhost_work_init);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 79c6e7a60a5e..749fe13e061c 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -20,10 +20,6 @@ typedef void (*vhost_work_fn_t)(struct vhost_work *work);
struct vhost_work {
struct llist_node node;
vhost_work_fn_t fn;
- wait_queue_head_t done;
- int flushing;
- unsigned queue_seq;
- unsigned done_seq;
unsigned long flags;
};
--
2.13.6
^ permalink raw reply related
* Re: [PATCH] vhost: Remove the unused variable.
From: Jason Wang @ 2018-01-09 3:02 UTC (permalink / raw)
To: Tonghao Zhang, virtualization
In-Reply-To: <1515462393-13354-1-git-send-email-xiangxia.m.yue@gmail.com>
On 2018年01月09日 09:46, Tonghao Zhang wrote:
> The patch (7235acdb1) changed the way of the work
> flushing in which the queued seq, done seq, and the
> flushing are not used anymore. Then remove them now.
>
> Fixes: 7235acdb1 ("vhost: simplify work flushing")
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> drivers/vhost/vhost.c | 1 -
> drivers/vhost/vhost.h | 4 ----
> 2 files changed, 5 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 33ac2b186b85..9b04cad91d65 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -181,7 +181,6 @@ void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn)
> {
> clear_bit(VHOST_WORK_QUEUED, &work->flags);
> work->fn = fn;
> - init_waitqueue_head(&work->done);
> }
> EXPORT_SYMBOL_GPL(vhost_work_init);
>
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 79c6e7a60a5e..749fe13e061c 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -20,10 +20,6 @@ typedef void (*vhost_work_fn_t)(struct vhost_work *work);
> struct vhost_work {
> struct llist_node node;
> vhost_work_fn_t fn;
> - wait_queue_head_t done;
> - int flushing;
> - unsigned queue_seq;
> - unsigned done_seq;
> unsigned long flags;
> };
>
Thanks for the patch, but please use get_maintainer.pl to cc correct
list and maintainers.
You can add my Acked-by when reposting.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH net-next] vhost_net: batch used ring update in rx
From: Jason Wang @ 2018-01-09 10:27 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel; +Cc: willemb
This patch tries to batched used ring update during RX. This is pretty
fit for the case when guest is much faster (e.g dpdk based
backend). In this case, used ring is almost empty:
- we may get serious cache line misses/contending on both used ring
and used idx.
- at most 1 packet could be dequeued at one time, batching in guest
does not make much effect.
Update used ring in a batch can help since guest won't access the used
ring until used idx was advanced for several descriptors and since we
advance used ring for every N packets, guest will only need to access
used idx for every N packet since it can cache the used idx. To have a
better interaction for both batch dequeuing and dpdk batching,
VHOST_RX_BATCH was used as the maximum number of descriptors that
could be batched.
Test were done between two machines with 2.40GHz Intel(R) Xeon(R) CPU
E5-2630 connected back to back through ixgbe. Traffic were generated
on one remote ixgbe through MoonGen and measure the RX pps through
testpmd in guest when do xdp_redirect_map from local ixgbe to
tap. RX pps were increased from 3.05 Mpps to 4.00 Mpps (about 31%
improvement).
One possible concern for this is the implications for TCP (especially
latency sensitive workload). Result[1] does not show obvious changes
for most of the netperf test (RR, TX, and RX). And we do get some
improvements for RX on some specific size.
Guest RX:
size/sessions/+thu%/+normalize%
64/ 1/ +2%/ +2%
64/ 2/ +2%/ -1%
64/ 4/ +1%/ +1%
64/ 8/ 0%/ 0%
256/ 1/ +6%/ -3%
256/ 2/ -3%/ +2%
256/ 4/ +11%/ +11%
256/ 8/ 0%/ 0%
512/ 1/ +4%/ 0%
512/ 2/ +2%/ +2%
512/ 4/ 0%/ -1%
512/ 8/ -8%/ -8%
1024/ 1/ -7%/ -17%
1024/ 2/ -8%/ -7%
1024/ 4/ +1%/ 0%
1024/ 8/ 0%/ 0%
2048/ 1/ +30%/ +14%
2048/ 2/ +46%/ +40%
2048/ 4/ 0%/ 0%
2048/ 8/ 0%/ 0%
4096/ 1/ +23%/ +22%
4096/ 2/ +26%/ +23%
4096/ 4/ 0%/ +1%
4096/ 8/ 0%/ 0%
16384/ 1/ -2%/ -3%
16384/ 2/ +1%/ -4%
16384/ 4/ -1%/ -3%
16384/ 8/ 0%/ -1%
65535/ 1/ +15%/ +7%
65535/ 2/ +4%/ +7%
65535/ 4/ 0%/ +1%
65535/ 8/ 0%/ 0%
TCP_RR:
size/sessions/+thu%/+normalize%
1/ 1/ 0%/ +1%
1/ 25/ +2%/ +1%
1/ 50/ +4%/ +1%
64/ 1/ 0%/ -4%
64/ 25/ +2%/ +1%
64/ 50/ 0%/ -1%
256/ 1/ 0%/ 0%
256/ 25/ 0%/ 0%
256/ 50/ +4%/ +2%
Guest TX:
size/sessions/+thu%/+normalize%
64/ 1/ +4%/ -2%
64/ 2/ -6%/ -5%
64/ 4/ +3%/ +6%
64/ 8/ 0%/ +3%
256/ 1/ +15%/ +16%
256/ 2/ +11%/ +12%
256/ 4/ +1%/ 0%
256/ 8/ +5%/ +5%
512/ 1/ -1%/ -6%
512/ 2/ 0%/ -8%
512/ 4/ -2%/ +4%
512/ 8/ +6%/ +9%
1024/ 1/ +3%/ +1%
1024/ 2/ +3%/ +9%
1024/ 4/ 0%/ +7%
1024/ 8/ 0%/ +7%
2048/ 1/ +8%/ +2%
2048/ 2/ +3%/ -1%
2048/ 4/ -1%/ +11%
2048/ 8/ +3%/ +9%
4096/ 1/ +8%/ +8%
4096/ 2/ 0%/ -7%
4096/ 4/ +4%/ +4%
4096/ 8/ +2%/ +5%
16384/ 1/ -3%/ +1%
16384/ 2/ -1%/ -12%
16384/ 4/ -1%/ +5%
16384/ 8/ 0%/ +1%
65535/ 1/ 0%/ -3%
65535/ 2/ +5%/ +16%
65535/ 4/ +1%/ +2%
65535/ 8/ +1%/ -1%
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index c7bdeb6..988af72 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -744,7 +744,7 @@ static void handle_rx(struct vhost_net *net)
};
size_t total_len = 0;
int err, mergeable;
- s16 headcount;
+ s16 headcount, nheads = 0;
size_t vhost_hlen, sock_hlen;
size_t vhost_len, sock_len;
struct socket *sock;
@@ -772,7 +772,7 @@ static void handle_rx(struct vhost_net *net)
while ((sock_len = vhost_net_rx_peek_head_len(net, sock->sk))) {
sock_len += sock_hlen;
vhost_len = sock_len + vhost_hlen;
- headcount = get_rx_bufs(vq, vq->heads, vhost_len,
+ headcount = get_rx_bufs(vq, vq->heads + nheads, vhost_len,
&in, vq_log, &log,
likely(mergeable) ? UIO_MAXIOV : 1);
/* On error, stop handling until the next kick. */
@@ -844,8 +844,12 @@ static void handle_rx(struct vhost_net *net)
vhost_discard_vq_desc(vq, headcount);
goto out;
}
- vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
- headcount);
+ nheads += headcount;
+ if (nheads > VHOST_RX_BATCH) {
+ vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
+ nheads);
+ nheads = 0;
+ }
if (unlikely(vq_log))
vhost_log_write(vq, vq_log, log, vhost_len);
total_len += vhost_len;
@@ -856,6 +860,9 @@ static void handle_rx(struct vhost_net *net)
}
vhost_net_enable_vq(net, vq);
out:
+ if (nheads)
+ vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
+ nheads);
mutex_unlock(&vq->mutex);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH v21 0/5] Virtio-balloon Enhancement
From: Wei Wang @ 2018-01-09 11:10 UTC (permalink / raw)
To: virtio-dev, linux-kernel, qemu-devel, virtualization, kvm,
linux-mm, mst, mhocko, akpm, mawilcox
Cc: aarcange, yang.zhang.wz, riel, quan.xu0, penguin-kernel,
liliang.opensource, willy, amit.shah, cornelia.huck, pbonzini,
nilal, mgorman
This patch series enhances the existing virtio-balloon with the following
new features:
1) fast ballooning: transfer ballooned pages between the guest and host in
chunks using sgs, instead of one array each time; and
2) free page block reporting: a new virtqueue to report guest free pages
to the host.
The second feature can be used to accelerate live migration of VMs. Here
are some details:
Live migration needs to transfer the VM's memory from the source machine
to the destination round by round. For the 1st round, all the VM's memory
is transferred. From the 2nd round, only the pieces of memory that were
written by the guest (after the 1st round) are transferred. One method
that is popularly used by the hypervisor to track which part of memory is
written is to write-protect all the guest memory.
The second feature enables the optimization of the 1st round memory
transfer - the hypervisor can skip the transfer of guest free pages in the
1st round. It is not concerned that the memory pages are used after they
are given to the hypervisor as a hint of the free pages, because they will
be tracked by the hypervisor and transferred in the next round if they are
used and written.
ChangeLog:
v20 (RESEND)->v21:
1) patch 1:
- xb_zero: fix the bug related to bitmap_clear;
- testing/radix-tree/Makefile: removed the "$CC.." line;
- added a test case for xbitmap_check_zero_bits;
2) patch 2: virtio-balloon
- changed the usage of the xb_ related APIs
v19->v20:
1) patch 1: xbitmap
- add __rcu to "void **slot";
- remove the exceptional path.
2) patch 3: xbitmap
- DeveloperNotes: add an item to comment that the current bit range
related APIs operating on extremely large ranges (e.g.
[0, ULONG_MAX)) will take too long time. This can be optimized in
the future.
- remove the exceptional path;
- remove xb_preload_and_set();
- reimplement xb_clear_bit_range to make its usage close to
bitmap_clear;
- rename xb_find_next_set_bit to xb_find_set, and re-implement it
in a style close to find_next_bit;
- rename xb_find_next_zero_bit to xb_find_clear, and re-implement
it in a stytle close to find_next_zero_bit;
- separate the implementation of xb_find_set and xb_find_clear for
the convenience of future updates.
3) patch 4: virtio-balloon
- xb_set_page: change the way to call xb_ related APIs
v18->v19:
1) patch 3:
- xb_clear_bit_range and xb_find_next_bit will deal with range [start,
end), where end is changed to be exclusive of the range.
- add overflow checks at the end of xb_clear_bit_range and
xb_find_next_bit
- add overflow related test cases
2) patch 4:
- change back to the previous add_one_sg methond, which is based on the
scatterlist struct
- tell_host_sgs: use "uint64_t len" to avoid overflow
- batch_balloon_page_sg: a simpler function to implement the batching of
sgs
3) patch 6: batch_free_page_sg: batch sgs using the previous scatterlist struct
4) patch 7: add a config field, poison_val, to tell the host about the poison
value
v17->v18:
1) patch 1-2: new to solve some tools related compilation issues
2) patch 3: revert to the original xbitmap implementation from Matthew
Wilcox with some minor changes (e.g. comments added to the exported
functions)
3) patch 4: summarize the changes we want to make to patch 3
4) patch 5: add the developer notes as a reminder for users to avoid
concurrent accesses to the ida bitmap
5) patch 6: a new vring API to allow users to directly pass in a physical
address to a vring desc
6) patch 7: ballooning time is reduced from ~490ms to ~440ms with the new
implementation
- use the new API from patch 6 to send balloon pages
- xb_preload with "GFP_NOWAIT | __GFP_NOWARN" flag;
- handle the case when xb_set_page() fails to avoid memory leak;
- put xb_set_page() under the balloon lock
7) patch 9: simper implementation
- start free page reporting by sending a new cmd id from the host
- guest acks the start or stop via adding a cmd id to the free page vq
- use vb->report_free_page, instead of vb->report_free_page_stop
- use WRITE_ONCE/READ_ONCE to access vb->report_free_page
- use the new API from patch 6 to send free pages to avoid the
unnecessary use of kaddr.
8) patch 10: new patch to solve the page posioning issue reported by
Michael S. Tsirkin
v16->v17:
1) patch 1: please check the commit log there;
2) patch 3: included Michael S. Tsirkin patch to fix the potential
deadlock issue;
3) patch 4: use BUG_ON if virtqueue_add_ returns error, which is
expected never to happen;
4) patch 4: add leak_balloon_sg_oom, which is used in the oom case when
VIRTIO_BALLOON_F_SG is in use;
5) patch 6: use config registers, instead of a vq, as the command channel
between the host and guest;
6) patch 6: add the command sequence id support.
v15->v16:
1) mm: stop reporting the free pfn range if the callback returns false;
2) mm: move some implementaion of walk_free_mem_block into a function to
make the code layout looks better;
3) xbitmap: added some optimizations suggested by Matthew, please refer to
the ChangLog in the xbitmap patch for details.
4) xbitmap: added a test suite
5) virtio-balloon: bail out with a warning when virtqueue_add_inbuf returns
an error
6) virtio-balloon: some small code re-arrangement, e.g. detachinf used buf
from the vq before adding a new buf
v14->v15:
1) mm: make the report callback return a bool value - returning 1 to stop
walking through the free page list.
2) virtio-balloon: batching sgs of balloon pages till the vq is full
3) virtio-balloon: create a new workqueue, rather than using the default
system_wq, to queue the free page reporting work item.
4) virtio-balloon: add a ctrl_vq to be a central control plane which will
handle all the future control related commands between the host and guest.
Add free page report as the first feature controlled under ctrl_vq, and
the free_page_vq is a data plane vq dedicated to the transmission of free
page blocks.
v13->v14:
1) xbitmap: move the code from lib/radix-tree.c to lib/xbitmap.c.
2) xbitmap: consolidate the implementation of xb_bit_set/clear/test into
one xb_bit_ops.
3) xbitmap: add documents for the exported APIs.
4) mm: rewrite the function to walk through free page blocks.
5) virtio-balloon: when reporting a free page blcok to the device, if the
vq is full (less likey to happen in practice), just skip reporting this
block, instead of busywaiting till an entry gets released.
6) virtio-balloon: fail the probe function if adding the signal buf in
init_vqs fails.
v12->v13:
1) mm: use a callback function to handle the the free page blocks from the
report function. This avoids exposing the zone internal to a kernel
module.
2) virtio-balloon: send balloon pages or a free page block using a single
sg each time. This has the benefits of simpler implementation with no new
APIs.
3) virtio-balloon: the free_page_vq is used to report free pages only (no
multiple usages interleaving)
4) virtio-balloon: Balloon pages and free page blocks are sent via input
sgs, and the completion signal to the host is sent via an output sg.
v11->v12:
1) xbitmap: use the xbitmap from Matthew Wilcox to record ballooned pages.
2) virtio-ring: enable the driver to build up a desc chain using vring
desc.
3) virtio-ring: Add locking to the existing START_USE() and END_USE()
macro to lock/unlock the vq when a vq operation starts/ends.
4) virtio-ring: add virtqueue_kick_sync() and virtqueue_kick_async()
5) virtio-balloon: describe chunks of ballooned pages and free pages
blocks directly using one or more chains of desc from the vq.
v10->v11:
1) virtio_balloon: use vring_desc to describe a chunk;
2) virtio_ring: support to add an indirect desc table to virtqueue;
3) virtio_balloon: use cmdq to report guest memory statistics.
v9->v10:
1) mm: put report_unused_page_block() under CONFIG_VIRTIO_BALLOON;
2) virtio-balloon: add virtballoon_validate();
3) virtio-balloon: msg format change;
4) virtio-balloon: move miscq handling to a task on system_freezable_wq;
5) virtio-balloon: code cleanup.
v8->v9:
1) Split the two new features, VIRTIO_BALLOON_F_BALLOON_CHUNKS and
VIRTIO_BALLOON_F_MISC_VQ, which were mixed together in the previous
implementation;
2) Simpler function to get the free page block.
v7->v8:
1) Use only one chunk format, instead of two.
2) re-write the virtio-balloon implementation patch.
3) commit changes
4) patch re-org
Matthew Wilcox (1):
xbitmap: Introduce xbitmap
Wei Wang (4):
virtio-balloon: VIRTIO_BALLOON_F_SG
mm: support reporting free page blocks
virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
virtio-balloon: don't report free pages when page poisoning is enabled
drivers/virtio/virtio_balloon.c | 443 ++++++++++++++++++++++++++----
include/linux/mm.h | 6 +
include/linux/xbitmap.h | 48 ++++
include/uapi/linux/virtio_balloon.h | 7 +
lib/Makefile | 2 +-
lib/radix-tree.c | 38 ++-
lib/xbitmap.c | 444 +++++++++++++++++++++++++++++++
mm/page_alloc.c | 91 +++++++
tools/include/linux/bitmap.h | 34 +++
tools/include/linux/kernel.h | 2 +
tools/testing/radix-tree/Makefile | 17 +-
tools/testing/radix-tree/linux/kernel.h | 2 -
tools/testing/radix-tree/linux/xbitmap.h | 1 +
tools/testing/radix-tree/main.c | 4 +
tools/testing/radix-tree/test.h | 1 +
15 files changed, 1080 insertions(+), 60 deletions(-)
create mode 100644 include/linux/xbitmap.h
create mode 100644 lib/xbitmap.c
create mode 100644 tools/testing/radix-tree/linux/xbitmap.h
--
2.7.4
^ permalink raw reply
* [PATCH v21 1/5] xbitmap: Introduce xbitmap
From: Wei Wang @ 2018-01-09 11:10 UTC (permalink / raw)
To: virtio-dev, linux-kernel, qemu-devel, virtualization, kvm,
linux-mm, mst, mhocko, akpm, mawilcox
Cc: aarcange, yang.zhang.wz, riel, quan.xu0, penguin-kernel,
liliang.opensource, willy, amit.shah, cornelia.huck, pbonzini,
nilal, mgorman
In-Reply-To: <1515496262-7533-1-git-send-email-wei.w.wang@intel.com>
From: Matthew Wilcox <mawilcox@microsoft.com>
The eXtensible Bitmap is a sparse bitmap representation which is
efficient for set bits which tend to cluster. It supports up to
'unsigned long' worth of bits.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
include/linux/xbitmap.h | 48 ++++
lib/Makefile | 2 +-
lib/radix-tree.c | 38 ++-
lib/xbitmap.c | 444 +++++++++++++++++++++++++++++++
tools/include/linux/bitmap.h | 34 +++
tools/include/linux/kernel.h | 2 +
tools/testing/radix-tree/Makefile | 17 +-
tools/testing/radix-tree/linux/kernel.h | 2 -
tools/testing/radix-tree/linux/xbitmap.h | 1 +
tools/testing/radix-tree/main.c | 4 +
tools/testing/radix-tree/test.h | 1 +
11 files changed, 583 insertions(+), 10 deletions(-)
create mode 100644 include/linux/xbitmap.h
create mode 100644 lib/xbitmap.c
create mode 100644 tools/testing/radix-tree/linux/xbitmap.h
diff --git a/include/linux/xbitmap.h b/include/linux/xbitmap.h
new file mode 100644
index 0000000..c008309
--- /dev/null
+++ b/include/linux/xbitmap.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * eXtensible Bitmaps
+ * Copyright (c) 2017 Microsoft Corporation
+ * Author: Matthew Wilcox <mawilcox@microsoft.com>
+ *
+ * eXtensible Bitmaps provide an unlimited-size sparse bitmap facility.
+ * All bits are initially zero.
+ *
+ * Locking is to be provided by the user. No xb_ function is safe to
+ * call concurrently with any other xb_ function.
+ */
+
+#include <linux/idr.h>
+
+struct xb {
+ struct radix_tree_root xbrt;
+};
+
+#define XB_INIT { \
+ .xbrt = RADIX_TREE_INIT(IDR_RT_MARKER | GFP_NOWAIT), \
+}
+#define DEFINE_XB(name) struct xb name = XB_INIT
+
+static inline void xb_init(struct xb *xb)
+{
+ INIT_RADIX_TREE(&xb->xbrt, IDR_RT_MARKER | GFP_NOWAIT);
+}
+
+int xb_set_bit(struct xb *xb, unsigned long bit);
+bool xb_test_bit(const struct xb *xb, unsigned long bit);
+void xb_clear_bit(struct xb *xb, unsigned long bit);
+void xb_zero(struct xb *xb, unsigned long min, unsigned long max);
+void xb_fill(struct xb *xb, unsigned long min, unsigned long max);
+bool xb_find_set(const struct xb *xb, unsigned long max, unsigned long *bit);
+bool xb_find_zero(const struct xb *xb, unsigned long max, unsigned long *bit);
+
+static inline bool xb_empty(const struct xb *xb)
+{
+ return radix_tree_empty(&xb->xbrt);
+}
+
+int __must_check xb_preload(gfp_t);
+
+static inline void xb_preload_end(void)
+{
+ preempt_enable();
+}
diff --git a/lib/Makefile b/lib/Makefile
index d11c48e..08a8183 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -19,7 +19,7 @@ KCOV_INSTRUMENT_dynamic_debug.o := n
lib-y := ctype.o string.o vsprintf.o cmdline.o \
rbtree.o radix-tree.o dump_stack.o timerqueue.o\
- idr.o int_sqrt.o extable.o \
+ idr.o xbitmap.o int_sqrt.o extable.o \
sha1.o chacha20.o irq_regs.o argv_split.o \
flex_proportions.o ratelimit.o show_mem.o \
is_single_threaded.o plist.o decompress.o kobject_uevent.o \
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index c8d5556..d2bd8fe 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -37,7 +37,7 @@
#include <linux/rcupdate.h>
#include <linux/slab.h>
#include <linux/string.h>
-
+#include <linux/xbitmap.h>
/* Number of nodes in fully populated tree of given height */
static unsigned long height_to_maxnodes[RADIX_TREE_MAX_PATH + 1] __read_mostly;
@@ -77,6 +77,11 @@ static struct kmem_cache *radix_tree_node_cachep;
RADIX_TREE_MAP_SHIFT))
#define IDA_PRELOAD_SIZE (IDA_MAX_PATH * 2 - 1)
+#define XB_INDEX_BITS (BITS_PER_LONG - ilog2(IDA_BITMAP_BITS))
+#define XB_MAX_PATH (DIV_ROUND_UP(XB_INDEX_BITS, \
+ RADIX_TREE_MAP_SHIFT))
+#define XB_PRELOAD_SIZE (XB_MAX_PATH * 2 - 1)
+
/*
* Per-cpu pool of preloaded nodes
*/
@@ -1781,7 +1786,7 @@ void __rcu **radix_tree_next_chunk(const struct radix_tree_root *root,
child = rcu_dereference_raw(node->slots[offset]);
}
- if (!child)
+ if (!child && !is_idr(root))
goto restart;
if (child == RADIX_TREE_RETRY)
break;
@@ -2135,6 +2140,35 @@ int ida_pre_get(struct ida *ida, gfp_t gfp)
}
EXPORT_SYMBOL(ida_pre_get);
+/**
+ * xb_preload - preload for xb_set_bit()
+ * @gfp_mask: allocation mask to use for preloading
+ *
+ * Preallocate memory to use for the next call to xb_set_bit(). On success,
+ * return zero, with preemption disabled. On error, return -ENOMEM with
+ * preemption not disabled.
+ */
+int xb_preload(gfp_t gfp)
+{
+ if (!this_cpu_read(ida_bitmap)) {
+ struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);
+
+ if (!bitmap)
+ return -ENOMEM;
+ /*
+ * The per-CPU variable is updated with preemption enabled.
+ * If the calling task is unlucky to be scheduled to another
+ * CPU which has no ida_bitmap allocation, it will be detected
+ * when setting a bit (i.e. xb_set_bit()).
+ */
+ bitmap = this_cpu_cmpxchg(ida_bitmap, NULL, bitmap);
+ kfree(bitmap);
+ }
+
+ return __radix_tree_preload(gfp, XB_PRELOAD_SIZE);
+}
+EXPORT_SYMBOL(xb_preload);
+
void __rcu **idr_get_free_cmn(struct radix_tree_root *root,
struct radix_tree_iter *iter, gfp_t gfp,
unsigned long max)
diff --git a/lib/xbitmap.c b/lib/xbitmap.c
new file mode 100644
index 0000000..62b2211
--- /dev/null
+++ b/lib/xbitmap.c
@@ -0,0 +1,444 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * XBitmap implementation
+ * Copyright (c) 2017 Microsoft Corporation
+ * Author: Matthew Wilcox <mawilcox@microsoft.com>
+ */
+
+#include <linux/bitmap.h>
+#include <linux/export.h>
+#include <linux/slab.h>
+#include <linux/xbitmap.h>
+
+/**
+ * xb_set_bit() - Set a bit in the XBitmap.
+ * @xb: The XBitmap.
+ * @bit: Index of the bit to set.
+ *
+ * This function is used to set a bit in the xbitmap.
+ *
+ * Return: 0 on success. -ENOMEM if memory could not be allocated.
+ */
+int xb_set_bit(struct xb *xb, unsigned long bit)
+{
+ unsigned long index = bit / IDA_BITMAP_BITS;
+ struct radix_tree_root *root = &xb->xbrt;
+ struct radix_tree_iter iter;
+ void __rcu **slot;
+ struct ida_bitmap *bitmap;
+
+ bit %= IDA_BITMAP_BITS;
+ radix_tree_iter_init(&iter, index);
+ slot = idr_get_free_cmn(root, &iter, GFP_NOWAIT | __GFP_NOWARN, index);
+ if (IS_ERR(slot)) {
+ if (slot == ERR_PTR(-ENOSPC))
+ return 0; /* Already set */
+ return -ENOMEM;
+ }
+ bitmap = rcu_dereference_raw(*slot);
+ if (!bitmap) {
+ bitmap = this_cpu_xchg(ida_bitmap, NULL);
+ if (!bitmap)
+ return -ENOMEM;
+ memset(bitmap, 0, sizeof(*bitmap));
+ radix_tree_iter_replace(root, &iter, slot, bitmap);
+ }
+
+ __set_bit(bit, bitmap->bitmap);
+ if (bitmap_full(bitmap->bitmap, IDA_BITMAP_BITS))
+ radix_tree_iter_tag_clear(root, &iter, IDR_FREE);
+ return 0;
+}
+EXPORT_SYMBOL(xb_set_bit);
+
+/**
+ * xb_clear_bit() - Clear a bit in the XBitmap.
+ * @xb: The XBitmap.
+ * @bit: Index of the bit to clear.
+ *
+ * This function is used to clear a bit in the xbitmap.
+ */
+void xb_clear_bit(struct xb *xb, unsigned long bit)
+{
+ unsigned long index = bit / IDA_BITMAP_BITS;
+ struct radix_tree_root *root = &xb->xbrt;
+ struct radix_tree_iter iter;
+ void __rcu **slot;
+ struct ida_bitmap *bitmap;
+
+ bit %= IDA_BITMAP_BITS;
+ slot = radix_tree_iter_lookup(root, &iter, index);
+ if (!slot)
+ return;
+ bitmap = radix_tree_deref_slot(slot);
+ if (!bitmap)
+ return;
+
+ radix_tree_iter_tag_set(root, &iter, IDR_FREE);
+ __clear_bit(bit, bitmap->bitmap);
+ if (bitmap_empty(bitmap->bitmap, IDA_BITMAP_BITS)) {
+ kfree(bitmap);
+ radix_tree_iter_delete(root, &iter, slot);
+ }
+}
+EXPORT_SYMBOL(xb_clear_bit);
+
+/**
+ * xb_zero() - Clear a range of bits in the XBitmap.
+ * @xb: The XBitmap.
+ * @min: The first bit to clear.
+ * @max: The last bit to clear.
+ *
+ * This function is used to clear a range of bits in the xbitmap.
+ */
+void xb_zero(struct xb *xb, unsigned long min, unsigned long max)
+{
+ struct radix_tree_root *root = &xb->xbrt;
+ struct radix_tree_iter iter;
+ void __rcu **slot;
+ struct ida_bitmap *bitmap;
+ unsigned long index = min / IDA_BITMAP_BITS;
+ unsigned long first = min % IDA_BITMAP_BITS;
+ unsigned long maxindex = max / IDA_BITMAP_BITS;
+
+ radix_tree_for_each_slot(slot, root, &iter, index) {
+ unsigned long nbits = IDA_BITMAP_BITS;
+
+ if (index > maxindex)
+ break;
+ bitmap = radix_tree_deref_slot(slot);
+ if (!bitmap)
+ continue;
+ radix_tree_iter_tag_set(root, &iter, IDR_FREE);
+
+ if (!first && iter.index < maxindex)
+ goto delete;
+ if (iter.index == maxindex)
+ nbits = max % IDA_BITMAP_BITS + 1;
+ bitmap_clear(bitmap->bitmap, first, nbits - first);
+ first = 0;
+ if (bitmap_empty(bitmap->bitmap, IDA_BITMAP_BITS))
+ goto delete;
+ continue;
+delete:
+ kfree(bitmap);
+ radix_tree_iter_delete(root, &iter, slot);
+ }
+}
+EXPORT_SYMBOL(xb_zero);
+
+/**
+ * xb_test_bit() - Test a bit in the xbitmap.
+ * @xb: The XBitmap.
+ * @bit: Index of the bit to test.
+ *
+ * This function is used to test a bit in the xbitmap.
+ *
+ * Return: %true if the bit is set.
+ */
+bool xb_test_bit(const struct xb *xb, unsigned long bit)
+{
+ unsigned long index = bit / IDA_BITMAP_BITS;
+ struct ida_bitmap *bitmap = radix_tree_lookup(&xb->xbrt, index);
+
+ bit %= IDA_BITMAP_BITS;
+
+ if (!bitmap)
+ return false;
+ return test_bit(bit, bitmap->bitmap);
+}
+EXPORT_SYMBOL(xb_test_bit);
+
+/**
+ * xb_find_set() - Find the next set bit in a range of bits.
+ * @xb: The XBitmap.
+ * @max: The maximum position to search.
+ * @bit: The first bit to examine, and on exit, the found bit.
+ *
+ * On entry, @bit points to the index of the first bit to search. On exit,
+ * if this function returns %true, @bit will be updated to the index of the
+ * first found bit. It will not be updated if this function returns %false.
+ *
+ * Return: %true if a set bit was found.
+ */
+bool xb_find_set(const struct xb *xb, unsigned long max, unsigned long *bit)
+{
+ struct radix_tree_iter iter;
+ void __rcu **slot;
+ struct ida_bitmap *bitmap;
+ unsigned long index = *bit / IDA_BITMAP_BITS;
+ unsigned int first = *bit % IDA_BITMAP_BITS;
+ unsigned long maxindex = max / IDA_BITMAP_BITS;
+
+ radix_tree_for_each_slot(slot, &xb->xbrt, &iter, index) {
+ if (iter.index > maxindex)
+ break;
+ bitmap = radix_tree_deref_slot(slot);
+ if (bitmap) {
+ unsigned int nbits = IDA_BITMAP_BITS;
+
+ if (iter.index == maxindex)
+ nbits = max % IDA_BITMAP_BITS + 1;
+ first = find_next_bit(bitmap->bitmap, nbits, first);
+ if (first != nbits) {
+ *bit = first + iter.index * IDA_BITMAP_BITS;
+ return true;
+ }
+ }
+ first = 0;
+ }
+
+ return false;
+}
+EXPORT_SYMBOL(xb_find_set);
+
+/**
+ * xb_find_zero() - Find the next zero bit in a range of bits
+ * @xb: The XBitmap.
+ * @max: The maximum index to search.
+ * @bit: Pointer to an index.
+ *
+ * On entry, @bit points to the index of the first bit to search. On exit,
+ * if this function returns %true, @bit will be updated to the index of the
+ * first found bit. It will not be updated if this function returns %false.
+ *
+ * Return: %true if a clear bit was found.
+ */
+bool xb_find_zero(const struct xb *xb, unsigned long max, unsigned long *bit)
+{
+ void __rcu **slot;
+ struct radix_tree_iter iter;
+ struct ida_bitmap *bitmap;
+ unsigned long index = *bit / IDA_BITMAP_BITS;
+ unsigned long first = *bit % IDA_BITMAP_BITS;
+ unsigned long maxindex = max / IDA_BITMAP_BITS;
+
+ radix_tree_for_each_tagged(slot, &xb->xbrt, &iter, index, IDR_FREE) {
+ unsigned int nbits = IDA_BITMAP_BITS;
+
+ if (iter.index > maxindex)
+ return false;
+ bitmap = radix_tree_deref_slot(slot);
+ if (!bitmap)
+ break;
+ if (iter.index == maxindex)
+ nbits = max % IDA_BITMAP_BITS + 1;
+ first = find_next_zero_bit(bitmap->bitmap, nbits, first);
+ if (first != nbits)
+ break;
+ first = 0;
+ }
+
+ *bit = first + iter.index * IDA_BITMAP_BITS;
+ return true;
+}
+EXPORT_SYMBOL(xb_find_zero);
+
+#ifndef __KERNEL__
+
+static DEFINE_XB(xb1);
+
+static void xbitmap_check_bit(unsigned long bit)
+{
+ unsigned long nbit = 0;
+
+ xb_preload(GFP_KERNEL);
+ assert(!xb_test_bit(&xb1, bit));
+ assert(xb_set_bit(&xb1, bit) == 0);
+ assert(xb_test_bit(&xb1, bit));
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == true);
+ assert(nbit == bit);
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == true);
+ assert(nbit == bit);
+ nbit++;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == false);
+ assert(nbit == bit + 1);
+ xb_clear_bit(&xb1, bit);
+ assert(xb_empty(&xb1));
+ xb_clear_bit(&xb1, bit);
+ assert(xb_empty(&xb1));
+ nbit = 0;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == false);
+ assert(nbit == 0);
+ xb_preload_end();
+}
+
+/*
+ * In the following tests, preload is called once when all the bits to set
+ * locate in the same ida bitmap. Otherwise, it is recommended to call
+ * preload for each xb_set_bit.
+ */
+static void xbitmap_check_bit_range(void)
+{
+ unsigned long nbit = 0;
+
+ /* Regular test1: node = NULL */
+ xb_preload(GFP_KERNEL);
+ xb_set_bit(&xb1, 700);
+ xb_preload_end();
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == true);
+ assert(nbit == 700);
+ nbit++;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == false);
+ assert(nbit == 701);
+ xb_zero(&xb1, 0, 1023);
+
+ /*
+ * Regular test2
+ * set bit 2000, 2001, 2040
+ * Next 1 in [0, 2048] --> 2000
+ * Next 1 in [2000, 2002] --> 2000
+ * Next 1 in [2002, 2040] --> 2040
+ * Next 1 in [2002, 2039] --> none
+ * Next 0 in [2000, 2048] --> 2002
+ * Next 0 in [2048, 2060] --> 2048
+ */
+ xb_preload(GFP_KERNEL);
+ assert(!xb_set_bit(&xb1, 2000));
+ assert(!xb_set_bit(&xb1, 2001));
+ assert(!xb_set_bit(&xb1, 2040));
+ nbit = 0;
+ assert(xb_find_set(&xb1, 2048, &nbit) == true);
+ assert(nbit == 2000);
+ assert(xb_find_set(&xb1, 2002, &nbit) == true);
+ assert(nbit == 2000);
+ nbit = 2002;
+ assert(xb_find_set(&xb1, 2040, &nbit) == true);
+ assert(nbit == 2040);
+ nbit = 2002;
+ assert(xb_find_set(&xb1, 2039, &nbit) == false);
+ assert(nbit == 2002);
+ nbit = 2000;
+ assert(xb_find_zero(&xb1, 2048, &nbit) == true);
+ assert(nbit == 2002);
+ nbit = 2048;
+ assert(xb_find_zero(&xb1, 2060, &nbit) == true);
+ assert(nbit == 2048);
+ xb_zero(&xb1, 0, 2048);
+ nbit = 0;
+ assert(xb_find_set(&xb1, 2048, &nbit) == false);
+ assert(nbit == 0);
+ xb_preload_end();
+
+ /*
+ * Overflow tests:
+ * Set bit 1 and ULONG_MAX - 4
+ * Next 1 in [0, ULONG_MAX] --> 1
+ * Next 1 in [1, ULONG_MAX] --> 1
+ * Next 1 in [2, ULONG_MAX] --> ULONG_MAX - 4
+ * Next 1 in [ULONG_MAX - 3, 2] --> none
+ * Next 0 in [ULONG_MAX - 4, ULONG_MAX] --> ULONG_MAX - 3
+ * Zero [ULONG_MAX - 4, ULONG_MAX]
+ * Next 1 in [ULONG_MAX - 10, ULONG_MAX] --> none
+ * Next 1 in [ULONG_MAX - 1, 2] --> none
+ * Zero [0, 1]
+ * Next 1 in [0, 2] --> none
+ */
+ xb_preload(GFP_KERNEL);
+ assert(!xb_set_bit(&xb1, 1));
+ xb_preload_end();
+ xb_preload(GFP_KERNEL);
+ assert(!xb_set_bit(&xb1, ULONG_MAX - 4));
+ nbit = 0;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == true);
+ assert(nbit == 1);
+ nbit = 1;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == true);
+ assert(nbit == 1);
+ nbit = 2;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == true);
+ assert(nbit == ULONG_MAX - 4);
+ nbit++;
+ assert(xb_find_set(&xb1, 2, &nbit) == false);
+ assert(nbit == ULONG_MAX - 3);
+ nbit--;
+ assert(xb_find_zero(&xb1, ULONG_MAX, &nbit) == true);
+ assert(nbit == ULONG_MAX - 3);
+ xb_zero(&xb1, ULONG_MAX - 4, ULONG_MAX);
+ nbit = ULONG_MAX - 10;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == false);
+ assert(nbit == ULONG_MAX - 10);
+ nbit = ULONG_MAX - 1;
+ assert(xb_find_set(&xb1, 2, &nbit) == false);
+ xb_zero(&xb1, 0, 1);
+ nbit = 0;
+ assert(xb_find_set(&xb1, 2, &nbit) == false);
+ assert(nbit == 0);
+ xb_preload_end();
+ assert(xb_empty(&xb1));
+}
+
+static void xbitmap_check_zero_bits(void)
+{
+ assert(xb_empty(&xb1));
+
+ /* Zero an empty xbitmap should work though no real work to do */
+ xb_zero(&xb1, 0, ULONG_MAX);
+ assert(xb_empty(&xb1));
+
+ xb_preload(GFP_KERNEL);
+ assert(xb_set_bit(&xb1, 0) == 0);
+ xb_preload_end();
+
+ /* Overflow test */
+ xb_zero(&xb1, ULONG_MAX - 10, ULONG_MAX);
+ assert(xb_test_bit(&xb1, 0));
+
+ xb_preload(GFP_KERNEL);
+ assert(xb_set_bit(&xb1, ULONG_MAX) == 0);
+ xb_preload_end();
+
+ xb_zero(&xb1, 0, ULONG_MAX);
+ assert(xb_empty(&xb1));
+}
+
+/* Check that setting an already-full bitmap works */
+static void xbitmap_check_set(unsigned long base)
+{
+ unsigned long i;
+
+ assert(xb_empty(&xb1));
+
+ for (i = 0; i < 64 * 1024; i++) {
+ xb_preload(GFP_KERNEL);
+ assert(xb_set_bit(&xb1, base + i) == 0);
+ xb_preload_end();
+ }
+ for (i = 0; i < 64 * 1024; i++)
+ assert(xb_set_bit(&xb1, base + i) == 0);
+
+ for (i = 0; i < 64 * 1024; i++)
+ xb_clear_bit(&xb1, base + i);
+
+ assert(xb_empty(&xb1));
+}
+
+static void xbitmap_checks(void)
+{
+ xb_init(&xb1);
+ xbitmap_check_bit(0);
+ xbitmap_check_bit(30);
+ xbitmap_check_bit(31);
+ xbitmap_check_bit(62);
+ xbitmap_check_bit(63);
+ xbitmap_check_bit(64);
+ xbitmap_check_bit(700);
+ xbitmap_check_bit(1023);
+ xbitmap_check_bit(1024);
+ xbitmap_check_bit(1025);
+ xbitmap_check_bit((1UL << 63) | (1UL << 24));
+ xbitmap_check_bit((1UL << 63) | (1UL << 24) | 70);
+
+ xbitmap_check_bit_range();
+ xbitmap_check_zero_bits();
+ xbitmap_check_set(0);
+ xbitmap_check_set(1024);
+ xbitmap_check_set(1024 * 64);
+}
+
+int __weak main(void)
+{
+ radix_tree_init();
+ xbitmap_checks();
+}
+#endif
diff --git a/tools/include/linux/bitmap.h b/tools/include/linux/bitmap.h
index ca16027..8d0bc1b 100644
--- a/tools/include/linux/bitmap.h
+++ b/tools/include/linux/bitmap.h
@@ -37,6 +37,40 @@ static inline void bitmap_zero(unsigned long *dst, int nbits)
}
}
+static inline void __bitmap_clear(unsigned long *map, unsigned int start,
+ int len)
+{
+ unsigned long *p = map + BIT_WORD(start);
+ const unsigned int size = start + len;
+ int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
+ unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
+
+ while (len - bits_to_clear >= 0) {
+ *p &= ~mask_to_clear;
+ len -= bits_to_clear;
+ bits_to_clear = BITS_PER_LONG;
+ mask_to_clear = ~0UL;
+ p++;
+ }
+ if (len) {
+ mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
+ *p &= ~mask_to_clear;
+ }
+}
+
+static inline __always_inline void bitmap_clear(unsigned long *map,
+ unsigned int start,
+ unsigned int nbits)
+{
+ if (__builtin_constant_p(nbits) && nbits == 1)
+ __clear_bit(start, map);
+ else if (__builtin_constant_p(start & 7) && IS_ALIGNED(start, 8) &&
+ __builtin_constant_p(nbits & 7) && IS_ALIGNED(nbits, 8))
+ memset((char *)map + start / 8, 0, nbits / 8);
+ else
+ __bitmap_clear(map, start, nbits);
+}
+
static inline void bitmap_fill(unsigned long *dst, unsigned int nbits)
{
unsigned int nlongs = BITS_TO_LONGS(nbits);
diff --git a/tools/include/linux/kernel.h b/tools/include/linux/kernel.h
index 0ad8844..3c992ae 100644
--- a/tools/include/linux/kernel.h
+++ b/tools/include/linux/kernel.h
@@ -13,6 +13,8 @@
#define UINT_MAX (~0U)
#endif
+#define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0)
+
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define PERF_ALIGN(x, a) __PERF_ALIGN_MASK(x, (typeof(x))(a)-1)
diff --git a/tools/testing/radix-tree/Makefile b/tools/testing/radix-tree/Makefile
index fa7ee36..788e526 100644
--- a/tools/testing/radix-tree/Makefile
+++ b/tools/testing/radix-tree/Makefile
@@ -1,12 +1,13 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS += -I. -I../../include -g -O2 -Wall -D_LGPL_SOURCE -fsanitize=address
-LDFLAGS += -fsanitize=address
-LDLIBS+= -lpthread -lurcu
-TARGETS = main idr-test multiorder
+LDFLAGS += -fsanitize=address $(LDLIBS)
+LDLIBS := -lpthread -lurcu
+TARGETS = main idr-test multiorder xbitmap
CORE_OFILES := radix-tree.o idr.o linux.o test.o find_bit.o
OFILES = main.o $(CORE_OFILES) regression1.o regression2.o regression3.o \
- tag_check.o multiorder.o idr-test.o iteration_check.o benchmark.o
+ tag_check.o multiorder.o idr-test.o iteration_check.o benchmark.o \
+ xbitmap.o
ifndef SHIFT
SHIFT=3
@@ -25,8 +26,10 @@ idr-test: idr-test.o $(CORE_OFILES)
multiorder: multiorder.o $(CORE_OFILES)
+xbitmap: xbitmap.o $(CORE_OFILES)
+
clean:
- $(RM) $(TARGETS) *.o radix-tree.c idr.c generated/map-shift.h
+ $(RM) $(TARGETS) *.o radix-tree.c idr.c xbitmap.c generated/map-shift.h
vpath %.c ../../lib
@@ -34,6 +37,7 @@ $(OFILES): Makefile *.h */*.h generated/map-shift.h \
../../include/linux/*.h \
../../include/asm/*.h \
../../../include/linux/radix-tree.h \
+ ../../../include/linux/xbitmap.h \
../../../include/linux/idr.h
radix-tree.c: ../../../lib/radix-tree.c
@@ -42,6 +46,9 @@ radix-tree.c: ../../../lib/radix-tree.c
idr.c: ../../../lib/idr.c
sed -e 's/^static //' -e 's/__always_inline //' -e 's/inline //' < $< > $@
+xbitmap.c: ../../../lib/xbitmap.c
+ sed -e 's/^static //' -e 's/__always_inline //' -e 's/inline //' < $< > $@
+
.PHONY: mapshift
mapshift:
diff --git a/tools/testing/radix-tree/linux/kernel.h b/tools/testing/radix-tree/linux/kernel.h
index c3bc3f3..426f32f 100644
--- a/tools/testing/radix-tree/linux/kernel.h
+++ b/tools/testing/radix-tree/linux/kernel.h
@@ -17,6 +17,4 @@
#define pr_debug printk
#define pr_cont printk
-#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
-
#endif /* _KERNEL_H */
diff --git a/tools/testing/radix-tree/linux/xbitmap.h b/tools/testing/radix-tree/linux/xbitmap.h
new file mode 100644
index 0000000..61de214
--- /dev/null
+++ b/tools/testing/radix-tree/linux/xbitmap.h
@@ -0,0 +1 @@
+#include "../../../../include/linux/xbitmap.h"
diff --git a/tools/testing/radix-tree/main.c b/tools/testing/radix-tree/main.c
index 257f3f8..d112363 100644
--- a/tools/testing/radix-tree/main.c
+++ b/tools/testing/radix-tree/main.c
@@ -326,6 +326,10 @@ static void single_thread_tests(bool long_run)
rcu_barrier();
printv(2, "after idr_checks: %d allocated, preempt %d\n",
nr_allocated, preempt_count);
+ xbitmap_checks();
+ rcu_barrier();
+ printv(2, "after xbitmap_checks: %d allocated, preempt %d\n",
+ nr_allocated, preempt_count);
big_gang_check(long_run);
rcu_barrier();
printv(2, "after big_gang_check: %d allocated, preempt %d\n",
diff --git a/tools/testing/radix-tree/test.h b/tools/testing/radix-tree/test.h
index d9c031d..8175d6b 100644
--- a/tools/testing/radix-tree/test.h
+++ b/tools/testing/radix-tree/test.h
@@ -38,6 +38,7 @@ void benchmark(void);
void idr_checks(void);
void ida_checks(void);
void ida_thread_tests(void);
+void xbitmap_checks(void);
struct item *
item_tag_set(struct radix_tree_root *root, unsigned long index, int tag);
--
2.7.4
^ permalink raw reply related
* [PATCH v21 2/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wei Wang @ 2018-01-09 11:10 UTC (permalink / raw)
To: virtio-dev, linux-kernel, qemu-devel, virtualization, kvm,
linux-mm, mst, mhocko, akpm, mawilcox
Cc: aarcange, yang.zhang.wz, riel, quan.xu0, penguin-kernel,
liliang.opensource, willy, amit.shah, cornelia.huck, pbonzini,
nilal, mgorman
In-Reply-To: <1515496262-7533-1-git-send-email-wei.w.wang@intel.com>
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 by one array each
time. 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 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 ~460ms resulting
in an improvement of ~89%.
TODO:
- optimize stage 1) by allocating/freeing a chunk of pages instead of a
single page each time.
- sort the internal balloon page queue.
- enable OOM to free inflated pages maintained in the local temporary
list.
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>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/virtio/virtio_balloon.c | 233 +++++++++++++++++++++++++++++++++---
include/uapi/linux/virtio_balloon.h | 1 +
2 files changed, 216 insertions(+), 18 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index a1fb52c..10876ea 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,15 +146,128 @@ static void set_page_pfns(struct virtio_balloon *vb,
page_to_balloon_pfn(page) + i);
}
+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));
+}
+
+static void add_one_sg(struct virtqueue *vq, unsigned long pfn, uint32_t len)
+{
+ struct scatterlist sg;
+ unsigned int unused;
+ int err;
+
+ sg_init_table(&sg, 1);
+ sg_set_page(&sg, pfn_to_page(pfn), len, 0);
+
+ /* Detach all the used buffers from the vq */
+ while (virtqueue_get_buf(vq, &unused))
+ ;
+
+ err = virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
+ /*
+ * This is expected to never fail: there is always at least 1 entry
+ * available on the vq, because when the vq is full the worker thread
+ * that adds the sg will be put into sleep until at least 1 entry is
+ * available to use.
+ */
+ BUG_ON(err);
+}
+
+static void batch_balloon_page_sg(struct virtio_balloon *vb,
+ struct virtqueue *vq,
+ unsigned long pfn,
+ uint32_t len)
+{
+ add_one_sg(vq, pfn, len);
+
+ /* Batch till the vq is full */
+ if (!vq->num_free)
+ kick_and_wait(vq, vb->acked);
+}
+
+/*
+ * 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 pfn_start, pfn_end;
+ uint32_t max_len = round_down(UINT_MAX, PAGE_SIZE);
+ uint64_t len;
+
+ pfn_start = page_xb_start;
+ while (pfn_start < page_xb_end) {
+ if (!xb_find_set(&vb->page_xb, page_xb_end, &pfn_start))
+ break;
+ pfn_end = pfn_start + 1;
+ if (!xb_find_zero(&vb->page_xb, page_xb_end, &pfn_end))
+ pfn_end = page_xb_end + 1;
+ len = (pfn_end - pfn_start) << PAGE_SHIFT;
+ while (len > max_len) {
+ batch_balloon_page_sg(vb, vq, pfn_start, max_len);
+ pfn_start += max_len >> PAGE_SHIFT;
+ len -= max_len;
+ }
+ batch_balloon_page_sg(vb, vq, pfn_start, (uint32_t)len);
+ pfn_start = 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))
+ kick_and_wait(vq, vb->acked);
+
+ xb_zero(&vb->page_xb, page_xb_start, page_xb_end);
+}
+
+static inline int 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);
+ int ret;
+
+ *pfn_min = min(pfn, *pfn_min);
+ *pfn_max = max(pfn, *pfn_max);
+
+ do {
+ if (xb_preload(GFP_NOWAIT | __GFP_NOWARN) < 0)
+ return -ENOMEM;
+
+ ret = xb_set_bit(&vb->page_xb, pfn);
+ xb_preload_end();
+ } while (unlikely(ret == -EAGAIN));
+
+ return ret;
+}
+
static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
{
unsigned num_allocated_pages;
unsigned num_pfns;
struct page *page;
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));
+ if (!use_sg)
+ num = min(num, ARRAY_SIZE(vb->pfns));
for (num_pfns = 0; num_pfns < num;
num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
@@ -173,8 +291,15 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
while ((page = balloon_page_pop(&pages))) {
balloon_page_enqueue(&vb->vb_dev_info, page);
+ if (use_sg) {
+ if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) {
+ __free_page(page);
+ continue;
+ }
+ } else {
+ set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
+ }
- 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))
@@ -184,8 +309,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;
@@ -211,9 +340,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 */
@@ -223,7 +355,14 @@ 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) {
+ if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) {
+ balloon_page_enqueue(&vb->vb_dev_info, page);
+ break;
+ }
+ } else {
+ set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
+ }
list_add(&page->lru, &pages);
vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
}
@@ -234,13 +373,55 @@ 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;
}
+/*
+ * The regular leak_balloon() with VIRTIO_BALLOON_F_SG needs memory allocation
+ * for xbitmap, which is not suitable for the oom case. This function does not
+ * use xbitmap to chunk pages, so it can be used by oom notifier to deflate
+ * pages when VIRTIO_BALLOON_F_SG is negotiated.
+ */
+static unsigned int leak_balloon_sg_oom(struct virtio_balloon *vb)
+{
+ unsigned int n;
+ struct page *page;
+ struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
+ struct virtqueue *vq = vb->deflate_vq;
+ LIST_HEAD(pages);
+
+ mutex_lock(&vb->balloon_lock);
+ for (n = 0; n < oom_pages; n++) {
+ page = balloon_page_dequeue(vb_dev_info);
+ if (!page)
+ break;
+
+ list_add(&page->lru, &pages);
+ vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
+ batch_balloon_page_sg(vb, vb->deflate_vq, page_to_pfn(page),
+ PAGE_SIZE);
+ release_pages_balloon(vb, &pages);
+ }
+
+ /*
+ * 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))
+ kick_and_wait(vq, vb->acked);
+ mutex_unlock(&vb->balloon_lock);
+
+ return n;
+}
+
static inline void update_stat(struct virtio_balloon *vb, int idx,
u16 tag, u64 val)
{
@@ -380,7 +561,10 @@ static int virtballoon_oom_notify(struct notifier_block *self,
return NOTIFY_OK;
freed = parm;
- num_freed_pages = leak_balloon(vb, oom_pages);
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG))
+ num_freed_pages = leak_balloon_sg_oom(vb);
+ else
+ num_freed_pages = leak_balloon(vb, oom_pages);
update_balloon_size(vb);
*freed += num_freed_pages;
@@ -477,6 +661,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;
/*
@@ -498,16 +683,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) {
+ add_one_sg(vb->inflate_vq, page_to_pfn(newpage), PAGE_SIZE);
+ kick_and_wait(vb->inflate_vq, vb->acked);
+ } 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) {
+ add_one_sg(vb->deflate_vq, page_to_pfn(page), PAGE_SIZE);
+ kick_and_wait(vb->deflate_vq, vb->acked);
+ } 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 */
@@ -566,6 +759,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);
@@ -682,6 +878,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 related
* [PATCH v21 3/5] mm: support reporting free page blocks
From: Wei Wang @ 2018-01-09 11:11 UTC (permalink / raw)
To: virtio-dev, linux-kernel, qemu-devel, virtualization, kvm,
linux-mm, mst, mhocko, akpm, mawilcox
Cc: aarcange, yang.zhang.wz, riel, quan.xu0, penguin-kernel,
liliang.opensource, willy, amit.shah, cornelia.huck, pbonzini,
nilal, mgorman
In-Reply-To: <1515496262-7533-1-git-send-email-wei.w.wang@intel.com>
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.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Michal Hocko <mhocko@kernel.org>
---
include/linux/mm.h | 6 ++++
mm/page_alloc.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ea818ff..b3077dd 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1938,6 +1938,12 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
unsigned long zone_start_pfn, unsigned long *zholes_size);
extern void free_initmem(void);
+extern void walk_free_mem_block(void *opaque,
+ int min_order,
+ bool (*report_pfn_range)(void *opaque,
+ unsigned long pfn,
+ unsigned long num));
+
/*
* 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 76c9688..705de22 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4899,6 +4899,97 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
show_swap_cache_info();
}
+/*
+ * Walk through a free page list and report the found pfn range via the
+ * callback.
+ *
+ * Return false if the callback requests to stop reporting. Otherwise,
+ * return true.
+ */
+static bool walk_free_page_list(void *opaque,
+ struct zone *zone,
+ int order,
+ enum migratetype mt,
+ bool (*report_pfn_range)(void *,
+ unsigned long,
+ unsigned long))
+{
+ struct page *page;
+ struct list_head *list;
+ unsigned long pfn, flags;
+ bool ret;
+
+ 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);
+ ret = report_pfn_range(opaque, pfn, 1 << order);
+ if (!ret)
+ break;
+ }
+ spin_unlock_irqrestore(&zone->lock, flags);
+
+ return ret;
+}
+
+/**
+ * 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_pfn_range: the callback to report the pfn range of the free pages
+ *
+ * If the callback returns false, stop iterating the list of free page blocks.
+ * Otherwise, continue to report.
+ *
+ * Please note that there are no locking guarantees for the callback and
+ * that the reported pfn range might be freed or disappear after the
+ * callback returns so the caller has to be very careful how it is used.
+ *
+ * The callback itself must not sleep or perform any operations which would
+ * require any memory allocations directly (not even GFP_NOWAIT/GFP_ATOMIC)
+ * or via any lock dependency. It is generally advisable to implement
+ * the callback as simple as possible and defer any heavy lifting to a
+ * different context.
+ *
+ * There is no guarantee that each free range will be reported only once
+ * during one walk_free_mem_block invocation.
+ *
+ * pfn_to_page on the given range is strongly discouraged and if there is
+ * an absolute need for that make sure to contact MM people to discuss
+ * potential problems.
+ *
+ * The function itself might sleep so it cannot be called from atomic
+ * contexts.
+ *
+ * In general low orders tend to be very volatile and so it makes more
+ * sense to query larger ones first for various optimizations which like
+ * ballooning etc... This will reduce the overhead as well.
+ */
+void walk_free_mem_block(void *opaque,
+ int min_order,
+ bool (*report_pfn_range)(void *opaque,
+ unsigned long pfn,
+ unsigned long num))
+{
+ struct zone *zone;
+ int order;
+ enum migratetype mt;
+ bool ret;
+
+ for_each_populated_zone(zone) {
+ for (order = MAX_ORDER - 1; order >= min_order; order--) {
+ for (mt = 0; mt < MIGRATE_TYPES; mt++) {
+ ret = walk_free_page_list(opaque, zone,
+ order, mt,
+ report_pfn_range);
+ if (!ret)
+ return;
+ }
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(walk_free_mem_block);
+
static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
{
zoneref->zone = zone;
--
2.7.4
^ permalink raw reply related
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