* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Samudrala, Sridhar @ 2017-12-19 19:42 UTC (permalink / raw)
To: Stephen Hemminger, David Miller
Cc: netdev, virtualization, alexander.duyck, mst
In-Reply-To: <20171219104159.30d9caaf@xeon-e3>
On 12/19/2017 10:41 AM, Stephen Hemminger wrote:
> On Tue, 19 Dec 2017 13:21:17 -0500 (EST)
> David Miller <davem@davemloft.net> wrote:
>
>> From: Stephen Hemminger <stephen@networkplumber.org>
>> Date: Tue, 19 Dec 2017 09:55:48 -0800
>>
>>> could be 10ms, just enough to let udev do its renaming
>> Please, move to some kind of notification or event based handling of
>> this problem.
>>
>> No delay is safe, what if userspace gets swapped out or whatever
>> else might make userspace stall unexpectedly?
>>
> The plan is to remove the delay and do the naming in the kernel.
> This was suggested by Lennart since udev is only doing naming policy
> because kernel names were not repeatable.
>
> This makes the VF show up as "ethN_vf" on Hyper-V which is user friendly.
>
> Patch is pending.
Do we really need to delay the setup until the name is changed?
Can't we call dev_set_mtu() and dev_open() until dev_change_name() is done?
Thanks
Sridhar
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Stephen Hemminger @ 2017-12-19 19:46 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: mst, netdev, alexander.duyck, virtualization, David Miller
In-Reply-To: <344f0afc-b4aa-9f1a-15b8-166ba97d270b@intel.com>
On Tue, 19 Dec 2017 11:42:33 -0800
"Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
> On 12/19/2017 10:41 AM, Stephen Hemminger wrote:
> > On Tue, 19 Dec 2017 13:21:17 -0500 (EST)
> > David Miller <davem@davemloft.net> wrote:
> >
> >> From: Stephen Hemminger <stephen@networkplumber.org>
> >> Date: Tue, 19 Dec 2017 09:55:48 -0800
> >>
> >>> could be 10ms, just enough to let udev do its renaming
> >> Please, move to some kind of notification or event based handling of
> >> this problem.
> >>
> >> No delay is safe, what if userspace gets swapped out or whatever
> >> else might make userspace stall unexpectedly?
> >>
> > The plan is to remove the delay and do the naming in the kernel.
> > This was suggested by Lennart since udev is only doing naming policy
> > because kernel names were not repeatable.
> >
> > This makes the VF show up as "ethN_vf" on Hyper-V which is user friendly.
> >
> > Patch is pending.
> Do we really need to delay the setup until the name is changed?
> Can't we call dev_set_mtu() and dev_open() until dev_change_name() is done?
>
> Thanks
> Sridhar
You can call dev_set_mtu, but when dev_open is done the device name
can not be changed by userspace.
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Samudrala, Sridhar @ 2017-12-19 22:37 UTC (permalink / raw)
To: Stephen Hemminger
Cc: mst, netdev, alexander.duyck, virtualization, David Miller
In-Reply-To: <20171219114627.76dac732@xeon-e3>
On 12/19/2017 11:46 AM, Stephen Hemminger wrote:
> On Tue, 19 Dec 2017 11:42:33 -0800
> "Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
>
>> On 12/19/2017 10:41 AM, Stephen Hemminger wrote:
>>> On Tue, 19 Dec 2017 13:21:17 -0500 (EST)
>>> David Miller <davem@davemloft.net> wrote:
>>>
>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>> Date: Tue, 19 Dec 2017 09:55:48 -0800
>>>>
>>>>> could be 10ms, just enough to let udev do its renaming
>>>> Please, move to some kind of notification or event based handling of
>>>> this problem.
>>>>
>>>> No delay is safe, what if userspace gets swapped out or whatever
>>>> else might make userspace stall unexpectedly?
>>>>
>>> The plan is to remove the delay and do the naming in the kernel.
>>> This was suggested by Lennart since udev is only doing naming policy
>>> because kernel names were not repeatable.
>>>
>>> This makes the VF show up as "ethN_vf" on Hyper-V which is user friendly.
>>>
>>> Patch is pending.
>> Do we really need to delay the setup until the name is changed?
>> Can't we call dev_set_mtu() and dev_open() until dev_change_name() is done?
>>
>> Thanks
>> Sridhar
> You can call dev_set_mtu, but when dev_open is done the device name
> can not be changed by userspace.
I did a quick test to remove the delay and also the dev_open() call and
i don't see
any issues with virtio taking over the VF datapath.
Only the netdev_info() messages may show old device name.
Any specific scenario where we need to explicitly call VF's dev_open()
in the VF setup process?
I tried i40evf driver loaded after virtio_net AND virtio_net loading
after i40evf.
Thanks
Sridhar
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Stephen Hemminger @ 2017-12-19 22:53 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: mst, netdev, alexander.duyck, virtualization, David Miller
In-Reply-To: <ddef3208-3cf5-faa2-7572-a561a6b35e2f@intel.com>
On Tue, 19 Dec 2017 14:37:50 -0800
"Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
> On 12/19/2017 11:46 AM, Stephen Hemminger wrote:
> > On Tue, 19 Dec 2017 11:42:33 -0800
> > "Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
> >
> >> On 12/19/2017 10:41 AM, Stephen Hemminger wrote:
> >>> On Tue, 19 Dec 2017 13:21:17 -0500 (EST)
> >>> David Miller <davem@davemloft.net> wrote:
> >>>
> >>>> From: Stephen Hemminger <stephen@networkplumber.org>
> >>>> Date: Tue, 19 Dec 2017 09:55:48 -0800
> >>>>
> >>>>> could be 10ms, just enough to let udev do its renaming
> >>>> Please, move to some kind of notification or event based handling of
> >>>> this problem.
> >>>>
> >>>> No delay is safe, what if userspace gets swapped out or whatever
> >>>> else might make userspace stall unexpectedly?
> >>>>
> >>> The plan is to remove the delay and do the naming in the kernel.
> >>> This was suggested by Lennart since udev is only doing naming policy
> >>> because kernel names were not repeatable.
> >>>
> >>> This makes the VF show up as "ethN_vf" on Hyper-V which is user friendly.
> >>>
> >>> Patch is pending.
> >> Do we really need to delay the setup until the name is changed?
> >> Can't we call dev_set_mtu() and dev_open() until dev_change_name() is done?
> >>
> >> Thanks
> >> Sridhar
> > You can call dev_set_mtu, but when dev_open is done the device name
> > can not be changed by userspace.
> I did a quick test to remove the delay and also the dev_open() call and
> i don't see
> any issues with virtio taking over the VF datapath.
> Only the netdev_info() messages may show old device name.
>
> Any specific scenario where we need to explicitly call VF's dev_open()
> in the VF setup process?
> I tried i40evf driver loaded after virtio_net AND virtio_net loading
> after i40evf.
>
> Thanks
> Sridhar
It happens with hotplug. It is possible on Hyper-V to hotplug SR-IOV on
and off while guest is running. If SR-IOV is disabled in host then the
VF device is removed (hotplug) and the inverse. If the master device is
up then the VF device should be brought up by the master device.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Samudrala, Sridhar @ 2017-12-20 0:26 UTC (permalink / raw)
To: Stephen Hemminger
Cc: mst, netdev, alexander.duyck, virtualization, David Miller
In-Reply-To: <20171219145345.3c261273@xeon-e3>
On 12/19/2017 2:53 PM, Stephen Hemminger wrote:
> On Tue, 19 Dec 2017 14:37:50 -0800
> "Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
>
>> On 12/19/2017 11:46 AM, Stephen Hemminger wrote:
>>> On Tue, 19 Dec 2017 11:42:33 -0800
>>> "Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
>>>
>>>> On 12/19/2017 10:41 AM, Stephen Hemminger wrote:
>>>>> On Tue, 19 Dec 2017 13:21:17 -0500 (EST)
>>>>> David Miller <davem@davemloft.net> wrote:
>>>>>
>>>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>>>> Date: Tue, 19 Dec 2017 09:55:48 -0800
>>>>>>
>>>>>>> could be 10ms, just enough to let udev do its renaming
>>>>>> Please, move to some kind of notification or event based handling of
>>>>>> this problem.
>>>>>>
>>>>>> No delay is safe, what if userspace gets swapped out or whatever
>>>>>> else might make userspace stall unexpectedly?
>>>>>>
>>>>> The plan is to remove the delay and do the naming in the kernel.
>>>>> This was suggested by Lennart since udev is only doing naming policy
>>>>> because kernel names were not repeatable.
>>>>>
>>>>> This makes the VF show up as "ethN_vf" on Hyper-V which is user friendly.
>>>>>
>>>>> Patch is pending.
>>>> Do we really need to delay the setup until the name is changed?
>>>> Can't we call dev_set_mtu() and dev_open() until dev_change_name() is done?
>>>>
>>>> Thanks
>>>> Sridhar
>>> You can call dev_set_mtu, but when dev_open is done the device name
>>> can not be changed by userspace.
>> I did a quick test to remove the delay and also the dev_open() call and
>> i don't see
>> any issues with virtio taking over the VF datapath.
>> Only the netdev_info() messages may show old device name.
>>
>> Any specific scenario where we need to explicitly call VF's dev_open()
>> in the VF setup process?
>> I tried i40evf driver loaded after virtio_net AND virtio_net loading
>> after i40evf.
>>
> It happens with hotplug. It is possible on Hyper-V to hotplug SR-IOV on
> and off while guest is running. If SR-IOV is disabled in host then the
> VF device is removed (hotplug) and the inverse. If the master device is
> up then the VF device should be brought up by the master device.
Even with KVM, we need to hot unplug SR-IOV device on the source host
and plug it
back on the destination host to enable live migration. It all works for
me even without
the dev_open() of the lower device from the VF setup routine. Will send
out a v2 version
of this patch without the delayed VF setup after some more testing next
week.
Thanks
Sridhar
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH v4 0/4] use put_device to cleanup resource
From: weiping zhang @ 2017-12-20 4:26 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
Hi,
The main change is split device_register into 2 sperate calls:
device_initalize() and device_add, and then the caller can use
put_device safety when fail to register_virtio_device.
v3->v4:
* split device_register into device_initialize and devicea_add that
the caller can always use put_device when fail to register virtio
device.
v2->v3:
* virtio: add new helper do get device's status then determine use
put_device or kfree.
v1->v2:
* virtio_pci: add comments in commit message for why using put_device
* virtio_vop: also use put_device int _vop_remove_device
weiping zhang (4):
virtio: split device_register into device_initialize and device_add
virtio_pci: don't kfree device on register failure
virtio_vop: don't kfree device on register failure
virtio_remoteproc: don't kfree device on register failure
drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++-------
drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
drivers/virtio/virtio.c | 18 +++++++++++++++---
drivers/virtio/virtio_pci_common.c | 8 ++++++--
4 files changed, 45 insertions(+), 14 deletions(-)
--
2.9.4
^ permalink raw reply
* [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
From: weiping zhang @ 2017-12-20 4:26 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513700444.git.zhangweiping@didichuxing.com>
In order to make caller do a simple cleanup, we split device_register
into device_initialize and device_add. device_initialize always sucess,
the caller can always use put_device when fail to register virtio_device
no matter fail at ida_simple_get or at device_add.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
Suggested-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/virtio/virtio.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index bf7ff39..3c9f211 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver *driver)
}
EXPORT_SYMBOL_GPL(unregister_virtio_driver);
+/**
+ * register_virtio_device - register virtio device
+ * @dev : virtio device interested
+ *
+ * If an error occurs, the caller must use put_device, instead of kfree, because
+ * device_initialize and device_add will increase @dev->dev's reference count.
+ *
+ * Returns: 0 on suceess, -error on failure
+ */
int register_virtio_device(struct virtio_device *dev)
{
int err;
dev->dev.bus = &virtio_bus;
+ device_initialize(&dev->dev);
/* Assign a unique device index and hence name. */
err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL);
@@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
INIT_LIST_HEAD(&dev->vqs);
- /* device_register() causes the bus infrastructure to look for a
- * matching driver. */
- err = device_register(&dev->dev);
+ /*
+ * device_add() causes the bus infrastructure to look for a matching
+ * driver.
+ */
+ err = device_add(&dev->dev);
if (err)
ida_simple_remove(&virtio_index_ida, dev->index);
out:
--
2.9.4
^ permalink raw reply related
* [PATCH v4 2/4] virtio_pci: don't kfree device on register failure
From: weiping zhang @ 2017-12-20 4:26 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513700444.git.zhangweiping@didichuxing.com>
As mentioned at drivers/base/core.c:
/*
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up the
* reference initialized in this function instead.
*/
so we don't free vp_dev until vp_dev->vdev.dev.release be called.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/virtio/virtio_pci_common.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 1c4797e..48d4d1c 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
static int virtio_pci_probe(struct pci_dev *pci_dev,
const struct pci_device_id *id)
{
- struct virtio_pci_device *vp_dev;
+ struct virtio_pci_device *vp_dev, *reg_dev = NULL;
int rc;
/* allocate our structure and fill it out */
@@ -551,6 +551,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
pci_set_master(pci_dev);
rc = register_virtio_device(&vp_dev->vdev);
+ reg_dev = vp_dev;
if (rc)
goto err_register;
@@ -564,7 +565,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
err_probe:
pci_disable_device(pci_dev);
err_enable_device:
- kfree(vp_dev);
+ if (reg_dev)
+ put_device(&vp_dev->vdev.dev);
+ else
+ kfree(vp_dev);
return rc;
}
--
2.9.4
^ permalink raw reply related
* [PATCH v4 3/4] virtio_vop: don't kfree device on register failure
From: weiping zhang @ 2017-12-20 4:27 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513700444.git.zhangweiping@didichuxing.com>
As mentioned at drivers/base/core.c:
/*
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up the
* reference initialized in this function instead.
*/
so we don't free vdev until vdev->vdev.dev.release be called.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..3633202 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data)
static void vop_virtio_release_dev(struct device *_d)
{
- /*
- * No need for a release method similar to virtio PCI.
- * Provide an empty one to avoid getting a warning from core.
- */
+ struct virtio_device *vdev =
+ container_of(_d, struct virtio_device, dev);
+ struct _vop_vdev *vop_vdev =
+ container_of(vdev, struct _vop_vdev, vdev);
+
+ kfree(vop_vdev);
}
/*
@@ -466,7 +468,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
unsigned int offset, struct vop_device *vpdev,
int dnode)
{
- struct _vop_vdev *vdev;
+ struct _vop_vdev *vdev, *reg_dev = NULL;
int ret;
u8 type = ioread8(&d->type);
@@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
vdev->c2h_vdev_db = ioread8(&vdev->dc->c2h_vdev_db);
ret = register_virtio_device(&vdev->vdev);
+ reg_dev = vdev;
if (ret) {
dev_err(_vop_dev(vdev),
"Failed to register vop device %u type %u\n",
@@ -512,7 +515,10 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
free_irq:
vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
kfree:
- kfree(vdev);
+ if (reg_dev)
+ put_device(&vdev->vdev.dev);
+ else
+ kfree(vdev);
return ret;
}
@@ -568,7 +574,7 @@ static int _vop_remove_device(struct mic_device_desc __iomem *d,
iowrite8(-1, &dc->h2c_vdev_db);
if (status & VIRTIO_CONFIG_S_DRIVER_OK)
wait_for_completion(&vdev->reset_done);
- kfree(vdev);
+ put_device(&vdev->vdev.dev);
iowrite8(1, &dc->guest_ack);
dev_dbg(&vpdev->dev, "%s %d guest_ack %d\n",
__func__, __LINE__, ioread8(&dc->guest_ack));
--
2.9.4
^ permalink raw reply related
* [PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure
From: weiping zhang @ 2017-12-20 4:27 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513700444.git.zhangweiping@didichuxing.com>
rproc_virtio_dev_release will be called iff virtio_device.dev's
refer count became to 0. Here we should check if we call device_register
or not, if called, put vdev.dev, and then rproc->dev's cleanup will be
done in rproc_virtio_dev_release, otherwise we do cleanup directly.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..1073ea3 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
{
struct rproc *rproc = rvdev->rproc;
struct device *dev = &rproc->dev;
- struct virtio_device *vdev = &rvdev->vdev;
+ struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL;
int ret;
vdev->id.device = id,
@@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
kref_get(&rvdev->refcount);
ret = register_virtio_device(vdev);
+ reg_dev = vdev;
if (ret) {
- put_device(&rproc->dev);
dev_err(dev, "failed to register vdev: %d\n", ret);
goto out;
}
dev_info(dev, "registered %s (type %d)\n", dev_name(&vdev->dev), id);
+ return 0;
+
out:
+ if (reg_dev)
+ put_device(&vdev->dev);
+ else {
+ kref_put(&rvdev->refcount, rproc_vdev_release);
+ put_device(&rproc->dev);
+ }
+
return ret;
}
--
2.9.4
^ permalink raw reply related
* [PATCH net-next] virtio_net: Add ethtool stats
From: Toshiaki Makita @ 2017-12-20 4:40 UTC (permalink / raw)
To: David S . Miller, Michael S . Tsirkin, Jason Wang; +Cc: netdev, virtualization
The main purpose of this patch is adding a way of checking per-queue stats.
It's useful to debug performance problems on multiqueue environment.
$ ethtool -S ens10
NIC statistics:
rx_packets: 4172939
tx_packets: 5855538
rx_bytes: 6317757408
tx_bytes: 8865151846
rx_dropped: 0
rx_length_errors: 0
rx_frame_errors: 0
tx_dropped: 0
tx_fifo_errors: 0
rx_queue_0_packets: 2090408
rx_queue_0_bytes: 3164825094
rx_queue_1_packets: 2082531
rx_queue_1_bytes: 3152932314
tx_queue_0_packets: 2770841
tx_queue_0_bytes: 4194955474
tx_queue_1_packets: 3084697
tx_queue_1_bytes: 4670196372
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 187 ++++++++++++++++++++++++++++++++++-------------
1 file changed, 136 insertions(+), 51 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6fb7b65..a0a7bf5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -65,14 +65,31 @@
VIRTIO_NET_F_GUEST_UFO
};
-struct virtnet_stats {
- struct u64_stats_sync tx_syncp;
- struct u64_stats_sync rx_syncp;
- u64 tx_bytes;
- u64 tx_packets;
-
- u64 rx_bytes;
- u64 rx_packets;
+struct virtnet_gstats {
+ char stat_string[ETH_GSTRING_LEN];
+ int stat_offset;
+};
+
+#define VIRTNET_NETDEV_STAT(m) offsetof(struct rtnl_link_stats64, m)
+
+static const struct virtnet_gstats virtnet_gstrings_stats[] = {
+ { "rx_packets", VIRTNET_NETDEV_STAT(rx_packets) },
+ { "tx_packets", VIRTNET_NETDEV_STAT(tx_packets) },
+ { "rx_bytes", VIRTNET_NETDEV_STAT(rx_bytes) },
+ { "tx_bytes", VIRTNET_NETDEV_STAT(tx_bytes) },
+ { "rx_dropped", VIRTNET_NETDEV_STAT(rx_dropped) },
+ { "rx_length_errors", VIRTNET_NETDEV_STAT(rx_length_errors) },
+ { "rx_frame_errors", VIRTNET_NETDEV_STAT(rx_frame_errors) },
+ { "tx_dropped", VIRTNET_NETDEV_STAT(tx_dropped) },
+ { "tx_fifo_errors", VIRTNET_NETDEV_STAT(tx_fifo_errors) },
+};
+
+# define VIRTNET_GSTATS_LEN ARRAY_SIZE(virtnet_gstrings_stats)
+
+struct virtnet_queue_stats {
+ struct u64_stats_sync syncp;
+ u64 bytes;
+ u64 packets;
};
/* Internal representation of a send virtqueue */
@@ -86,6 +103,8 @@ struct send_queue {
/* Name of the send queue: output.$index */
char name[40];
+ struct virtnet_queue_stats stats;
+
struct napi_struct napi;
};
@@ -98,6 +117,8 @@ struct receive_queue {
struct bpf_prog __rcu *xdp_prog;
+ struct virtnet_queue_stats stats;
+
/* Chain pages by the private ptr. */
struct page *pages;
@@ -149,9 +170,6 @@ struct virtnet_info {
/* Packet virtio header size */
u8 hdr_len;
- /* Active statistics */
- struct virtnet_stats __percpu *stats;
-
/* Work struct for refilling if we run low on memory. */
struct delayed_work refill;
@@ -1121,7 +1139,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget, bool *xdp_xmit)
struct virtnet_info *vi = rq->vq->vdev->priv;
unsigned int len, received = 0, bytes = 0;
void *buf;
- struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
if (!vi->big_packets || vi->mergeable_rx_bufs) {
void *ctx;
@@ -1144,10 +1161,10 @@ static int virtnet_receive(struct receive_queue *rq, int budget, bool *xdp_xmit)
schedule_delayed_work(&vi->refill, 0);
}
- u64_stats_update_begin(&stats->rx_syncp);
- stats->rx_bytes += bytes;
- stats->rx_packets += received;
- u64_stats_update_end(&stats->rx_syncp);
+ u64_stats_update_begin(&rq->stats.syncp);
+ rq->stats.bytes += bytes;
+ rq->stats.packets += received;
+ u64_stats_update_end(&rq->stats.syncp);
return received;
}
@@ -1156,8 +1173,6 @@ static void free_old_xmit_skbs(struct send_queue *sq)
{
struct sk_buff *skb;
unsigned int len;
- struct virtnet_info *vi = sq->vq->vdev->priv;
- struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
unsigned int packets = 0;
unsigned int bytes = 0;
@@ -1176,10 +1191,10 @@ static void free_old_xmit_skbs(struct send_queue *sq)
if (!packets)
return;
- u64_stats_update_begin(&stats->tx_syncp);
- stats->tx_bytes += bytes;
- stats->tx_packets += packets;
- u64_stats_update_end(&stats->tx_syncp);
+ u64_stats_update_begin(&sq->stats.syncp);
+ sq->stats.bytes += bytes;
+ sq->stats.packets += packets;
+ u64_stats_update_end(&sq->stats.syncp);
}
static void virtnet_poll_cleantx(struct receive_queue *rq)
@@ -1463,24 +1478,24 @@ static void virtnet_stats(struct net_device *dev,
struct rtnl_link_stats64 *tot)
{
struct virtnet_info *vi = netdev_priv(dev);
- int cpu;
unsigned int start;
+ int i;
- for_each_possible_cpu(cpu) {
- struct virtnet_stats *stats = per_cpu_ptr(vi->stats, cpu);
+ for (i = 0; i < vi->max_queue_pairs; i++) {
u64 tpackets, tbytes, rpackets, rbytes;
+ struct receive_queue *rq = &vi->rq[i];
+ struct send_queue *sq = &vi->sq[i];
do {
- start = u64_stats_fetch_begin_irq(&stats->tx_syncp);
- tpackets = stats->tx_packets;
- tbytes = stats->tx_bytes;
- } while (u64_stats_fetch_retry_irq(&stats->tx_syncp, start));
-
+ start = u64_stats_fetch_begin_irq(&sq->stats.syncp);
+ tpackets = sq->stats.packets;
+ tbytes = sq->stats.bytes;
+ } while (u64_stats_fetch_retry_irq(&sq->stats.syncp, start));
do {
- start = u64_stats_fetch_begin_irq(&stats->rx_syncp);
- rpackets = stats->rx_packets;
- rbytes = stats->rx_bytes;
- } while (u64_stats_fetch_retry_irq(&stats->rx_syncp, start));
+ start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
+ rpackets = rq->stats.packets;
+ rbytes = rq->stats.bytes;
+ } while (u64_stats_fetch_retry_irq(&rq->stats.syncp, start));
tot->rx_packets += rpackets;
tot->tx_packets += tpackets;
@@ -1817,6 +1832,84 @@ static int virtnet_set_channels(struct net_device *dev,
return err;
}
+static void virtnet_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ char *p = (char *)data;
+ unsigned int i;
+
+ switch (stringset) {
+ case ETH_SS_STATS:
+ for (i = 0; i < VIRTNET_GSTATS_LEN; i++) {
+ memcpy(p, virtnet_gstrings_stats[i].stat_string,
+ ETH_GSTRING_LEN);
+ p += ETH_GSTRING_LEN;
+ }
+ for (i = 0; i < vi->curr_queue_pairs; i++) {
+ sprintf(p, "rx_queue_%u_packets", i);
+ p += ETH_GSTRING_LEN;
+ sprintf(p, "rx_queue_%u_bytes", i);
+ p += ETH_GSTRING_LEN;
+ }
+ for (i = 0; i < vi->curr_queue_pairs; i++) {
+ sprintf(p, "tx_queue_%u_packets", i);
+ p += ETH_GSTRING_LEN;
+ sprintf(p, "tx_queue_%u_bytes", i);
+ p += ETH_GSTRING_LEN;
+ }
+ break;
+ }
+}
+
+static int virtnet_get_sset_count(struct net_device *dev, int sset)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+
+ switch (sset) {
+ case ETH_SS_STATS:
+ return VIRTNET_GSTATS_LEN + vi->curr_queue_pairs * 4;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static void virtnet_get_ethtool_stats(struct net_device *dev,
+ struct ethtool_stats *stats, u64 *data)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ struct rtnl_link_stats64 storage;
+ unsigned int idx = 0, start, i;
+ const u8 *stats_base;
+
+ stats_base = (u8 *)dev_get_stats(dev, &storage);
+ for (i = 0; i < VIRTNET_GSTATS_LEN; i++) {
+ data[idx++] = *(u64 *)(stats_base +
+ virtnet_gstrings_stats[i].stat_offset);
+ }
+
+ for (i = 0; i < vi->curr_queue_pairs; i++) {
+ struct receive_queue *rq = &vi->rq[i];
+
+ do {
+ start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
+ data[idx] = rq->stats.packets;
+ data[idx + 1] = rq->stats.bytes;
+ } while (u64_stats_fetch_retry_irq(&rq->stats.syncp, start));
+ idx += 2;
+ }
+
+ for (i = 0; i < vi->curr_queue_pairs; i++) {
+ struct send_queue *sq = &vi->sq[i];
+
+ do {
+ start = u64_stats_fetch_begin_irq(&sq->stats.syncp);
+ data[idx] = sq->stats.packets;
+ data[idx + 1] = sq->stats.bytes;
+ } while (u64_stats_fetch_retry_irq(&sq->stats.syncp, start));
+ idx += 2;
+ }
+}
+
static void virtnet_get_channels(struct net_device *dev,
struct ethtool_channels *channels)
{
@@ -1898,6 +1991,9 @@ static void virtnet_init_settings(struct net_device *dev)
.get_drvinfo = virtnet_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_ringparam = virtnet_get_ringparam,
+ .get_strings = virtnet_get_strings,
+ .get_sset_count = virtnet_get_sset_count,
+ .get_ethtool_stats = virtnet_get_ethtool_stats,
.set_channels = virtnet_set_channels,
.get_channels = virtnet_get_channels,
.get_ts_info = ethtool_op_get_ts_info,
@@ -2389,6 +2485,9 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
sg_init_table(vi->rq[i].sg, ARRAY_SIZE(vi->rq[i].sg));
ewma_pkt_len_init(&vi->rq[i].mrg_avg_pkt_len);
sg_init_table(vi->sq[i].sg, ARRAY_SIZE(vi->sq[i].sg));
+
+ u64_stats_init(&vi->rq[i].stats.syncp);
+ u64_stats_init(&vi->sq[i].stats.syncp);
}
return 0;
@@ -2513,7 +2612,7 @@ static int virtnet_validate(struct virtio_device *vdev)
static int virtnet_probe(struct virtio_device *vdev)
{
- int i, err;
+ int i, err = -ENOMEM;
struct net_device *dev;
struct virtnet_info *vi;
u16 max_queue_pairs;
@@ -2590,17 +2689,6 @@ static int virtnet_probe(struct virtio_device *vdev)
vi->dev = dev;
vi->vdev = vdev;
vdev->priv = vi;
- vi->stats = alloc_percpu(struct virtnet_stats);
- err = -ENOMEM;
- if (vi->stats == NULL)
- goto free;
-
- for_each_possible_cpu(i) {
- struct virtnet_stats *virtnet_stats;
- virtnet_stats = per_cpu_ptr(vi->stats, i);
- u64_stats_init(&virtnet_stats->tx_syncp);
- u64_stats_init(&virtnet_stats->rx_syncp);
- }
INIT_WORK(&vi->config_work, virtnet_config_changed_work);
@@ -2637,7 +2725,7 @@ static int virtnet_probe(struct virtio_device *vdev)
*/
dev_err(&vdev->dev, "device MTU appears to have changed "
"it is now %d < %d", mtu, dev->min_mtu);
- goto free_stats;
+ goto free;
}
dev->mtu = mtu;
@@ -2661,7 +2749,7 @@ static int virtnet_probe(struct virtio_device *vdev)
/* Allocate/initialize the rx/tx queues, and invoke find_vqs */
err = init_vqs(vi);
if (err)
- goto free_stats;
+ goto free;
#ifdef CONFIG_SYSFS
if (vi->mergeable_rx_bufs)
@@ -2715,8 +2803,6 @@ static int virtnet_probe(struct virtio_device *vdev)
cancel_delayed_work_sync(&vi->refill);
free_receive_page_frags(vi);
virtnet_del_vqs(vi);
-free_stats:
- free_percpu(vi->stats);
free:
free_netdev(dev);
return err;
@@ -2749,7 +2835,6 @@ static void virtnet_remove(struct virtio_device *vdev)
remove_vq_common(vi);
- free_percpu(vi->stats);
free_netdev(vi->dev);
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next] virtio_net: Add ethtool stats
From: Jason Wang @ 2017-12-20 8:13 UTC (permalink / raw)
To: Toshiaki Makita, David S . Miller, Michael S . Tsirkin
Cc: netdev, virtualization
In-Reply-To: <1513744837-2672-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On 2017年12月20日 12:40, Toshiaki Makita wrote:
> The main purpose of this patch is adding a way of checking per-queue stats.
> It's useful to debug performance problems on multiqueue environment.
>
> $ ethtool -S ens10
> NIC statistics:
> rx_packets: 4172939
> tx_packets: 5855538
> rx_bytes: 6317757408
> tx_bytes: 8865151846
> rx_dropped: 0
> rx_length_errors: 0
> rx_frame_errors: 0
> tx_dropped: 0
> tx_fifo_errors: 0
> rx_queue_0_packets: 2090408
> rx_queue_0_bytes: 3164825094
> rx_queue_1_packets: 2082531
> rx_queue_1_bytes: 3152932314
> tx_queue_0_packets: 2770841
> tx_queue_0_bytes: 4194955474
> tx_queue_1_packets: 3084697
> tx_queue_1_bytes: 4670196372
>
> Signed-off-by: Toshiaki Makita<makita.toshiaki@lab.ntt.co.jp>
Thanks for the patch. This is not the first patch that wants to do this.
Maybe you can go through the previous comments (E.g there's one in
https://patchwork.ozlabs.org/patch/244413/).
For me, I'd expect to add some XDP counters like other device did.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v20 0/7] Virtio-balloon Enhancement
From: Wei Wang @ 2017-12-20 10:34 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
nilal, riel, cornelia.huck, mhocko, quan.xu0, linux-kernel,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <201712192305.AAE21882.MtQHJOFFSFVOLO@I-love.SAKURA.ne.jp>
On 12/19/2017 10:05 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> ChangeLog:
>> 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.
> Removing exceptional path made this patch easier to read.
> But what I meant is
>
> Can you eliminate exception path and fold all xbitmap patches into one, and
> post only one xbitmap patch without virtio-balloon changes?
>
> .
>
> I still think we don't need xb_preload()/xb_preload_end().
Why would you think preload is not needed?
The bitmap is allocated via preload "bitmap =
this_cpu_cmpxchg(ida_bitmap, NULL, bitmap);", this allocated bitmap
would be used in xb_set_bit().
> I think xb_find_set() has a bug in !node path.
I think we can probably remove the "!node" path for now. It would be
good to get the fundamental part in first, and leave optimization to
come as separate patches with corresponding test cases in the future.
Best,
Wei
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Vitaly Kuznetsov @ 2017-12-20 10:51 UTC (permalink / raw)
To: David Miller
Cc: Stephen Hemminger, mst, sridhar.samudrala, alexander.duyck,
virtualization, netdev
In-Reply-To: <20171219.132017.1652469595167726292.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: "Samudrala, Sridhar" <sridhar.samudrala@intel.com>
> Date: Tue, 19 Dec 2017 09:41:39 -0800
>
>> This is based on netvsc implementation and here is the commit that
>> added this delay. Not sure if this needs to be 100ms.
>>
>> commit 6123c66854c174e4982f98195100c1d990f9e5e6
>> Author: stephen hemminger <stephen@networkplumber.org>
>> Date: Wed Aug 9 17:46:03 2017 -0700
>>
>> netvsc: delay setup of VF device
>>
>> When VF device is discovered, delay bring it automatically up in
>> order to allow userspace to some simple changes (like renaming).
>
> This is kind of bogus, I should have called this out when the patch
> was posted.
>
> Any delay is wrong, there needs to be tight synchronization if a
> userspace operation must occur before proceeding. If something
> happens and userspace is delayed, this whole thing doesn't work.
Hyper-V's netvsc does exactly the same and when this was first discussed
I suggested to allow renaming of IFF_UP interfaces:
https://patchwork.kernel.org/patch/9890299/
but as far as I understand it was decided that it's too risky and not
worth it. Maybe we need to reconsider this, at least for slave
interfaces (as scripts are not supposed to operate on them).
--
Vitaly
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v20 0/7] Virtio-balloon Enhancement
From: Matthew Wilcox @ 2017-12-20 12:25 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, mst, Tetsuo Handa, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, nilal, riel, cornelia.huck, mhocko, quan.xu0,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <5A3A3CBC.4030202@intel.com>
On Wed, Dec 20, 2017 at 06:34:36PM +0800, Wei Wang wrote:
> On 12/19/2017 10:05 PM, Tetsuo Handa wrote:
> > I think xb_find_set() has a bug in !node path.
>
> I think we can probably remove the "!node" path for now. It would be good to
> get the fundamental part in first, and leave optimization to come as
> separate patches with corresponding test cases in the future.
You can't remove the !node path. You'll see !node when the highest set
bit is less than 1024. So do something like this:
unsigned long bit;
xb_preload(GFP_KERNEL);
xb_set_bit(xb, 700);
xb_preload_end();
bit = xb_find_set(xb, ULONG_MAX, 0);
assert(bit == 700);
^ permalink raw reply
* Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
From: Cornelia Huck @ 2017-12-20 15:53 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <95ad7cb52d9e65b27f09a23c7f330128388502fa.1513700444.git.zhangweiping@didichuxing.com>
On Wed, 20 Dec 2017 12:26:25 +0800
weiping zhang <zwp10758@gmail.com> wrote:
[you used a different mail address in your From: than in your s-o-b:;
same for the other patches]
> In order to make caller do a simple cleanup, we split device_register
> into device_initialize and device_add. device_initialize always sucess,
s/success/succeeds/
> the caller can always use put_device when fail to register virtio_device
"so the caller can always use put_device when register_virtio_device
failed,"
> no matter fail at ida_simple_get or at device_add.
"no matter whether it failed..."
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> ---
> drivers/virtio/virtio.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index bf7ff39..3c9f211 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver *driver)
> }
> EXPORT_SYMBOL_GPL(unregister_virtio_driver);
>
> +/**
> + * register_virtio_device - register virtio device
> + * @dev : virtio device interested
"virtio device to be registered"
> + *
> + * If an error occurs, the caller must use put_device, instead of kfree, because
> + * device_initialize and device_add will increase @dev->dev's reference count.
That's not correct: It's not because of device_add increasing the
reference count (it releases it again on failure), but because another
code path may have obtained a reference.
What about:
"On error, the caller must call put_device on &@dev->dev (and not
kfree), as another code path may have obtained a reference to @dev."
> + *
> + * Returns: 0 on suceess, -error on failure
> + */
> int register_virtio_device(struct virtio_device *dev)
> {
> int err;
>
> dev->dev.bus = &virtio_bus;
> + device_initialize(&dev->dev);
>
> /* Assign a unique device index and hence name. */
> err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL);
> @@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
>
> INIT_LIST_HEAD(&dev->vqs);
>
> - /* device_register() causes the bus infrastructure to look for a
> - * matching driver. */
> - err = device_register(&dev->dev);
> + /*
> + * device_add() causes the bus infrastructure to look for a matching
> + * driver.
FWIW, I would just have done s/device_register/device_add/ in the
comment, but this is ok as well.
> + */
> + err = device_add(&dev->dev);
> if (err)
> ida_simple_remove(&virtio_index_ida, dev->index);
> out:
Your code change is fine.
^ permalink raw reply
* Re: [PATCH v4 2/4] virtio_pci: don't kfree device on register failure
From: Cornelia Huck @ 2017-12-20 15:55 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <dbd2c99ac7063a7905205c845c89b561537e1581.1513700444.git.zhangweiping@didichuxing.com>
On Wed, 20 Dec 2017 12:26:43 +0800
weiping zhang <zwp10758@gmail.com> wrote:
> As mentioned at drivers/base/core.c:
> /*
> * NOTE: _Never_ directly free @dev after calling this function, even
> * if it returned an error! Always use put_device() to give up the
> * reference initialized in this function instead.
> */
> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/virtio/virtio_pci_common.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..48d4d1c 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
> static int virtio_pci_probe(struct pci_dev *pci_dev,
> const struct pci_device_id *id)
> {
> - struct virtio_pci_device *vp_dev;
> + struct virtio_pci_device *vp_dev, *reg_dev = NULL;
Not sure if I would have used a pointer for this purpose, but as that
is what Michael had proposed...
> int rc;
>
> /* allocate our structure and fill it out */
> @@ -551,6 +551,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> pci_set_master(pci_dev);
>
> rc = register_virtio_device(&vp_dev->vdev);
> + reg_dev = vp_dev;
> if (rc)
> goto err_register;
>
> @@ -564,7 +565,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> err_probe:
> pci_disable_device(pci_dev);
> err_enable_device:
> - kfree(vp_dev);
> + if (reg_dev)
> + put_device(&vp_dev->vdev.dev);
> + else
> + kfree(vp_dev);
> return rc;
> }
>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply
* Re: [PATCH v4 3/4] virtio_vop: don't kfree device on register failure
From: Cornelia Huck @ 2017-12-20 15:57 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <3602a1fa10a90d3d52ad7fa26f29e36b0127d970.1513700444.git.zhangweiping@didichuxing.com>
On Wed, 20 Dec 2017 12:27:04 +0800
weiping zhang <zwp10758@gmail.com> wrote:
> As mentioned at drivers/base/core.c:
> /*
> * NOTE: _Never_ directly free @dev after calling this function, even
> * if it returned an error! Always use put_device() to give up the
> * reference initialized in this function instead.
> */
> so we don't free vdev until vdev->vdev.dev.release be called.
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
> index a341938..3633202 100644
> --- a/drivers/misc/mic/vop/vop_main.c
> +++ b/drivers/misc/mic/vop/vop_main.c
> @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data)
>
> static void vop_virtio_release_dev(struct device *_d)
> {
> - /*
> - * No need for a release method similar to virtio PCI.
> - * Provide an empty one to avoid getting a warning from core.
> - */
> + struct virtio_device *vdev =
> + container_of(_d, struct virtio_device, dev);
> + struct _vop_vdev *vop_vdev =
> + container_of(vdev, struct _vop_vdev, vdev);
> +
> + kfree(vop_vdev);
> }
>
> /*
> @@ -466,7 +468,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
> unsigned int offset, struct vop_device *vpdev,
> int dnode)
> {
> - struct _vop_vdev *vdev;
> + struct _vop_vdev *vdev, *reg_dev = NULL;
Similarly, not a fan of that pointer variable, but it's fine.
> int ret;
> u8 type = ioread8(&d->type);
>
> @@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
> vdev->c2h_vdev_db = ioread8(&vdev->dc->c2h_vdev_db);
>
> ret = register_virtio_device(&vdev->vdev);
> + reg_dev = vdev;
> if (ret) {
> dev_err(_vop_dev(vdev),
> "Failed to register vop device %u type %u\n",
> @@ -512,7 +515,10 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
> free_irq:
> vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
> kfree:
> - kfree(vdev);
> + if (reg_dev)
> + put_device(&vdev->vdev.dev);
> + else
> + kfree(vdev);
> return ret;
> }
>
> @@ -568,7 +574,7 @@ static int _vop_remove_device(struct mic_device_desc __iomem *d,
> iowrite8(-1, &dc->h2c_vdev_db);
> if (status & VIRTIO_CONFIG_S_DRIVER_OK)
> wait_for_completion(&vdev->reset_done);
> - kfree(vdev);
> + put_device(&vdev->vdev.dev);
> iowrite8(1, &dc->guest_ack);
> dev_dbg(&vpdev->dev, "%s %d guest_ack %d\n",
> __func__, __LINE__, ioread8(&dc->guest_ack));
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply
* Re: [PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure
From: Cornelia Huck @ 2017-12-20 16:12 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <443858dfab89720b846aa9c656679ff6ad20d0af.1513700444.git.zhangweiping@didichuxing.com>
On Wed, 20 Dec 2017 12:27:33 +0800
weiping zhang <zwp10758@gmail.com> wrote:
> rproc_virtio_dev_release will be called iff virtio_device.dev's
> refer count became to 0. Here we should check if we call device_register
"reference count drops to 0"
s/call/called/
> or not, if called, put vdev.dev, and then rproc->dev's cleanup will be
> done in rproc_virtio_dev_release, otherwise we do cleanup directly.
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 2946348..1073ea3 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> {
> struct rproc *rproc = rvdev->rproc;
> struct device *dev = &rproc->dev;
> - struct virtio_device *vdev = &rvdev->vdev;
> + struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL;
> int ret;
>
> vdev->id.device = id,
> @@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> kref_get(&rvdev->refcount);
>
> ret = register_virtio_device(vdev);
> + reg_dev = vdev;
> if (ret) {
> - put_device(&rproc->dev);
> dev_err(dev, "failed to register vdev: %d\n", ret);
> goto out;
> }
>
> dev_info(dev, "registered %s (type %d)\n", dev_name(&vdev->dev), id);
>
> + return 0;
> +
> out:
> + if (reg_dev)
> + put_device(&vdev->dev);
> + else {
> + kref_put(&rvdev->refcount, rproc_vdev_release);
> + put_device(&rproc->dev);
> + }
> +
> return ret;
> }
>
I think in this case using the marker makes a straightforward cleanup
way too complicated. There's a single way we can get to the out label,
and that's when register_virtio_device() failed. Switching
put_device(&rproc->dev) to put_device(@vdev->dev) (what your first
patch did) seems like the way to go.
(It also may be good to cc: the maintainers for this driver.)
^ permalink raw reply
* RE: [PATCH v20 0/7] Virtio-balloon Enhancement
From: Wang, Wei W @ 2017-12-20 16:13 UTC (permalink / raw)
To: Matthew Wilcox
Cc: yang.zhang.wz@gmail.com, kvm@vger.kernel.org, mst@redhat.com,
Tetsuo Handa, liliang.opensource@gmail.com, qemu-devel@nongnu.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
mawilcox@microsoft.com, nilal@redhat.com, riel@redhat.com,
cornelia.huck@de.ibm.com, mhocko@kernel.org, quan.xu0@gmail.com,
linux-kernel@vger.kernel.org, amit.sh
In-Reply-To: <20171220122547.GA1654@bombadil.infradead.org>
On Wednesday, December 20, 2017 8:26 PM, Matthew Wilcox wrote:
> On Wed, Dec 20, 2017 at 06:34:36PM +0800, Wei Wang wrote:
> > On 12/19/2017 10:05 PM, Tetsuo Handa wrote:
> > > I think xb_find_set() has a bug in !node path.
> >
> > I think we can probably remove the "!node" path for now. It would be
> > good to get the fundamental part in first, and leave optimization to
> > come as separate patches with corresponding test cases in the future.
>
> You can't remove the !node path. You'll see !node when the highest set bit
> is less than 1024. So do something like this:
>
> unsigned long bit;
> xb_preload(GFP_KERNEL);
> xb_set_bit(xb, 700);
> xb_preload_end();
> bit = xb_find_set(xb, ULONG_MAX, 0);
> assert(bit == 700);
This above test will result in "!node with bitmap !=NULL", and it goes to the regular "if (bitmap)" path, which finds 700.
A better test would be
...
xb_set_bit(xb, 700);
assert(xb_find_set(xb, ULONG_MAX, 800) == ULONG_MAX);
...
The first try with the "if (bitmap)" path doesn't find a set bit, and the remaining tries will always result in "!node && !bitmap", which implies no set bit anymore and no need to try in this case.
So, I think we can change it to
If (!node && !bitmap)
return size;
Best,
Wei
^ permalink raw reply
* Re: [PATCH v20 0/7] Virtio-balloon Enhancement
From: Matthew Wilcox @ 2017-12-20 17:10 UTC (permalink / raw)
To: Wang, Wei W
Cc: yang.zhang.wz@gmail.com, kvm@vger.kernel.org, mst@redhat.com,
Tetsuo Handa, liliang.opensource@gmail.com, qemu-devel@nongnu.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
mawilcox@microsoft.com, nilal@redhat.com, riel@redhat.com,
cornelia.huck@de.ibm.com, mhocko@kernel.org, quan.xu0@gmail.com,
linux-kernel@vger.kernel.org, amit.sh
In-Reply-To: <286AC319A985734F985F78AFA26841F73938CC3E@shsmsx102.ccr.corp.intel.com>
On Wed, Dec 20, 2017 at 04:13:16PM +0000, Wang, Wei W wrote:
> On Wednesday, December 20, 2017 8:26 PM, Matthew Wilcox wrote:
> > unsigned long bit;
> > xb_preload(GFP_KERNEL);
> > xb_set_bit(xb, 700);
> > xb_preload_end();
> > bit = xb_find_set(xb, ULONG_MAX, 0);
> > assert(bit == 700);
>
> This above test will result in "!node with bitmap !=NULL", and it goes to the regular "if (bitmap)" path, which finds 700.
>
> A better test would be
> ...
> xb_set_bit(xb, 700);
> assert(xb_find_set(xb, ULONG_MAX, 800) == ULONG_MAX);
> ...
I decided to write a test case to show you what I meant, then I discovered
the test suite didn't build, then the test I wrote took forever to run, so
I rewrote xb_find_set() using the radix tree iterators. So I have no idea
what bugs may be in your implementation, but at least this function passes
the current test suite. Of course, there may be gaps in the test suite.
And since I changed the API to not have the ambiguous return value, I
also changed the test suite, and maybe I introduced a bug.
diff --git a/include/linux/xbitmap.h b/include/linux/xbitmap.h
index ede1029b8a27..96e7e3560a0e 100644
--- a/include/linux/xbitmap.h
+++ b/include/linux/xbitmap.h
@@ -37,8 +37,7 @@ bool xb_test_bit(const struct xb *xb, unsigned long bit);
void xb_clear_bit(struct xb *xb, unsigned long bit);
void xb_clear_bit_range(struct xb *xb, unsigned long start,
unsigned long nbits);
-unsigned long xb_find_set(struct xb *xb, unsigned long size,
- unsigned long offset);
+bool xb_find_set(struct xb *xb, unsigned long max, unsigned long *bit);
unsigned long xb_find_zero(struct xb *xb, unsigned long size,
unsigned long offset);
diff --git a/lib/xbitmap.c b/lib/xbitmap.c
index 0bd3027b082d..58c26c8dd595 100644
--- a/lib/xbitmap.c
+++ b/lib/xbitmap.c
@@ -150,48 +150,39 @@ EXPORT_SYMBOL(xb_test_bit);
/**
* xb_find_set - find the next set bit in a range of bits
* @xb: the xbitmap to search from
- * @offset: the offset in the range to start searching
- * @size: the size of the range
+ * @max: the maximum position to search to
+ * @bit: the first bit to examine, and on exit, the found bit
*
- * Returns: the found bit or, @size if no set bit is found.
+ * Returns: %true if a set bit was found. @bit will be updated.
*/
-unsigned long xb_find_set(struct xb *xb, unsigned long size,
- unsigned long offset)
+bool xb_find_set(struct xb *xb, unsigned long max, unsigned long *bit)
{
- struct radix_tree_root *root = &xb->xbrt;
- struct radix_tree_node *node;
+ struct radix_tree_iter iter;
void __rcu **slot;
struct ida_bitmap *bitmap;
- unsigned long index = offset / IDA_BITMAP_BITS;
- unsigned long index_end = size / IDA_BITMAP_BITS;
- unsigned long bit = offset % IDA_BITMAP_BITS;
-
- if (unlikely(offset >= size))
- return size;
-
- while (index <= index_end) {
- unsigned long ret;
- unsigned int nbits = size - index * IDA_BITMAP_BITS;
-
- bitmap = __radix_tree_lookup(root, index, &node, &slot);
- if (!node) {
- index = (index | RADIX_TREE_MAP_MASK) + 1;
- continue;
- }
-
+ unsigned long index = *bit / IDA_BITMAP_BITS;
+ unsigned int first = *bit % IDA_BITMAP_BITS;
+ unsigned long index_end = max / IDA_BITMAP_BITS;
+
+ radix_tree_for_each_slot(slot, &xb->xbrt, &iter, index) {
+ if (iter.index > index_end)
+ break;
+ bitmap = radix_tree_deref_slot(slot);
if (bitmap) {
- if (nbits > IDA_BITMAP_BITS)
- nbits = IDA_BITMAP_BITS;
-
- ret = find_next_bit(bitmap->bitmap, nbits, bit);
- if (ret != nbits)
- return ret + index * IDA_BITMAP_BITS;
+ unsigned int nbits = IDA_BITMAP_BITS;
+ if (iter.index == index_end)
+ 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;
+ }
}
- bit = 0;
- index++;
+ first = 0;
}
- return size;
+ return false;
}
EXPORT_SYMBOL(xb_find_set);
@@ -246,19 +237,30 @@ static DEFINE_XB(xb1);
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_clear_bit(&xb1, bit) == 0);
+ 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));
- assert(xb_clear_bit(&xb1, bit) == 0);
+ xb_clear_bit(&xb1, bit);
assert(xb_empty(&xb1));
xb_preload_end();
}
static void xbitmap_check_bit_range(void)
{
+ unsigned long nbit;
+
/*
* Regular tests
* set bit 2000, 2001, 2040
@@ -273,14 +275,23 @@ static void xbitmap_check_bit_range(void)
assert(!xb_set_bit(&xb1, 2000));
assert(!xb_set_bit(&xb1, 2001));
assert(!xb_set_bit(&xb1, 2040));
- assert(xb_find_set(&xb1, 2048, 0) == 2000);
- assert(xb_find_set(&xb1, 2002, 2000) == 2000);
- assert(xb_find_set(&xb1, 2041, 2002) == 2040);
- assert(xb_find_set(&xb1, 2040, 2002) == 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, 2041, &nbit) == true);
+ assert(nbit == 2040);
+ nbit = 2002;
+ assert(xb_find_set(&xb1, 2040, &nbit) == true);
+ assert(nbit == 2040);
assert(xb_find_zero(&xb1, 2048, 2000) == 2002);
assert(xb_find_zero(&xb1, 2060, 2048) == 2048);
xb_clear_bit_range(&xb1, 0, 2048);
- assert(xb_find_set(&xb1, 2048, 0) == 2048);
+ nbit = 0;
+ assert(xb_find_set(&xb1, 2048, &nbit) == false);
+ assert(nbit == 0);
xb_preload_end();
/*
@@ -295,15 +306,22 @@ static void xbitmap_check_bit_range(void)
xb_preload_end();
xb_preload(GFP_KERNEL);
assert(!xb_set_bit(&xb1, ULONG_MAX - 4));
- assert(xb_find_set(&xb1, ULONG_MAX, ULONG_MAX - 4) == ULONG_MAX - 4);
- assert(xb_find_set(&xb1, ULONG_MAX + 4, ULONG_MAX - 3) ==
- ULONG_MAX + 4);
+ nbit = ULONG_MAX - 4;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == true);
+ assert(nbit == ULONG_MAX - 4);
+ nbit++;
+ assert(xb_find_set(&xb1, ULONG_MAX + 4, &nbit) == false);
+ assert(nbit == ULONG_MAX - 3);
assert(xb_find_zero(&xb1, ULONG_MAX + 4, ULONG_MAX - 4) ==
ULONG_MAX + 4);
xb_clear_bit_range(&xb1, ULONG_MAX - 4, 4);
- assert(xb_find_set(&xb1, ULONG_MAX, ULONG_MAX - 10) == ULONG_MAX);
+ nbit = ULONG_MAX - 10;
+ assert(xb_find_set(&xb1, ULONG_MAX, &nbit) == false);
+ assert(nbit == ULONG_MAX - 10);
xb_clear_bit_range(&xb1, 0, 2);
- assert(xb_find_set(&xb1, 2, 0) == 2);
+ nbit = 0;
+ assert(xb_find_set(&xb1, 2, &nbit) == false);
+ assert(nbit == 0);
xb_preload_end();
}
@@ -313,6 +331,10 @@ void xbitmap_checks(void)
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);
diff --git a/tools/testing/radix-tree/Makefile b/tools/testing/radix-tree/Makefile
index 34ece7883629..adf36e34dd77 100644
--- a/tools/testing/radix-tree/Makefile
+++ b/tools/testing/radix-tree/Makefile
@@ -1,9 +1,9 @@
# 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 \
diff --git a/tools/testing/radix-tree/linux/kernel.h b/tools/testing/radix-tree/linux/kernel.h
index c3bc3f364f68..426f32f28547 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 */
^ permalink raw reply related
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Jakub Kicinski @ 2017-12-20 22:33 UTC (permalink / raw)
To: Sridhar Samudrala; +Cc: netdev, virtualization, alexander.duyck, mst
In-Reply-To: <1513644036-45230-1-git-send-email-sridhar.samudrala@intel.com>
On Mon, 18 Dec 2017 16:40:36 -0800, Sridhar Samudrala wrote:
> +static int virtio_netdev_event(struct notifier_block *this,
> + unsigned long event, void *ptr)
> +{
> + struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> +
> + /* Skip our own events */
> + if (event_dev->netdev_ops == &virtnet_netdev)
> + return NOTIFY_DONE;
I wonder how does this work WRT loop prevention. What if I have two
virtio devices with the same MAC, what is preventing them from claiming
each other? Is it only the check above (not sure what is "own" in
comment referring to)?
I'm worried the check above will not stop virtio from enslaving hyperv
interfaces and vice versa, potentially leading to a loop, no? There is
also the fact that it would be preferable to share the code between
paravirt drivers, to avoid duplicated bugs.
My suggestion during the previous discussion was to create a paravirt
bond device, which will explicitly tell the OS which interfaces to
bond, regardless of which driver they're using. Could be some form of
ACPI/FW driver too, I don't know enough about x86 FW to suggest
something fully fleshed :(
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Michael S. Tsirkin @ 2017-12-21 0:14 UTC (permalink / raw)
To: Sridhar Samudrala; +Cc: netdev, alexander.duyck, virtualization
In-Reply-To: <1513644036-45230-1-git-send-email-sridhar.samudrala@intel.com>
On Mon, Dec 18, 2017 at 04:40:36PM -0800, Sridhar Samudrala wrote:
> This patch enables virtio to switch over to a VF datapath when a VF netdev
> is present with the same MAC address.
I prefer saying "a passthrough device" here. Does not have to be a VF at
all.
> It allows live migration of a VM
> with a direct attached VF without the need to setup a bond/team between a
> VF and virtio net device in the guest.
>
> The hypervisor needs to unplug the VF device from the guest on the source
> host and reset the MAC filter of the VF to initiate failover of datapath to
> virtio before starting the migration. After the migration is completed, the
> destination hypervisor sets the MAC filter on the VF and plugs it back to
> the guest to switch over to VF datapath.
>
> It is entirely based on netvsc implementation and it should be possible to
> make this code generic and move it to a common location that can be shared
> by netvsc and virtio.
>
> Also, i think we should make this a negotiated feature that is off by
> default via a new feature bit.
So please include this. A copy needs to go to virtio TC
to reserve the bit. Enabling this by default risks breaking
too many configurations.
>
> This patch is based on the discussion initiated by Jesse on this thread.
> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> drivers/net/virtio_net.c | 341 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 339 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 559b215c0169..a34c717bb15b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -31,6 +31,8 @@
> #include <linux/average.h>
> #include <linux/filter.h>
> #include <net/route.h>
> +#include <linux/netdevice.h>
> +#include <linux/netpoll.h>
>
> static int napi_weight = NAPI_POLL_WEIGHT;
> module_param(napi_weight, int, 0444);
> @@ -56,6 +58,8 @@ module_param(napi_tx, bool, 0644);
> */
> DECLARE_EWMA(pkt_len, 0, 64)
>
> +#define VF_TAKEOVER_INT (HZ / 10)
> +
> #define VIRTNET_DRIVER_VERSION "1.0.0"
>
> static const unsigned long guest_offloads[] = {
> @@ -117,6 +121,15 @@ struct receive_queue {
> char name[40];
> };
>
> +struct virtnet_vf_pcpu_stats {
> + u64 rx_packets;
> + u64 rx_bytes;
> + u64 tx_packets;
> + u64 tx_bytes;
> + struct u64_stats_sync syncp;
> + u32 tx_dropped;
> +};
> +
> struct virtnet_info {
> struct virtio_device *vdev;
> struct virtqueue *cvq;
> @@ -179,6 +192,11 @@ struct virtnet_info {
> u32 speed;
>
> unsigned long guest_offloads;
> +
> + /* State to manage the associated VF interface. */
> + struct net_device __rcu *vf_netdev;
> + struct virtnet_vf_pcpu_stats __percpu *vf_stats;
> + struct delayed_work vf_takeover;
> };
>
> struct padded_vnet_hdr {
> @@ -1300,16 +1318,51 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
> }
>
> +/* Send skb on the slave VF device. */
> +static int virtnet_vf_xmit(struct net_device *dev, struct net_device *vf_netdev,
> + struct sk_buff *skb)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> + unsigned int len = skb->len;
> + int rc;
> +
> + skb->dev = vf_netdev;
> + skb->queue_mapping = qdisc_skb_cb(skb)->slave_dev_queue_mapping;
> +
> + rc = dev_queue_xmit(skb);
> + if (likely(rc == NET_XMIT_SUCCESS || rc == NET_XMIT_CN)) {
> + struct virtnet_vf_pcpu_stats *pcpu_stats
> + = this_cpu_ptr(vi->vf_stats);
> +
> + u64_stats_update_begin(&pcpu_stats->syncp);
> + pcpu_stats->tx_packets++;
> + pcpu_stats->tx_bytes += len;
> + u64_stats_update_end(&pcpu_stats->syncp);
> + } else {
> + this_cpu_inc(vi->vf_stats->tx_dropped);
> + }
> +
> + return rc;
> +}
> +
> static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> int qnum = skb_get_queue_mapping(skb);
> struct send_queue *sq = &vi->sq[qnum];
> + struct net_device *vf_netdev;
> int err;
> struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
> bool kick = !skb->xmit_more;
> bool use_napi = sq->napi.weight;
>
> + /* if VF is present and up then redirect packets
> + * called with rcu_read_lock_bh
> + */
> + vf_netdev = rcu_dereference_bh(vi->vf_netdev);
> + if (vf_netdev && netif_running(vf_netdev) && !netpoll_tx_running(dev))
> + return virtnet_vf_xmit(dev, vf_netdev, skb);
> +
> /* Free up any pending old buffers before queueing new ones. */
> free_old_xmit_skbs(sq);
>
> @@ -1456,10 +1509,41 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
> return ret;
> }
>
> +static void virtnet_get_vf_stats(struct net_device *dev,
> + struct virtnet_vf_pcpu_stats *tot)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> + int i;
> +
> + memset(tot, 0, sizeof(*tot));
> +
> + for_each_possible_cpu(i) {
> + const struct virtnet_vf_pcpu_stats *stats
> + = per_cpu_ptr(vi->vf_stats, i);
> + u64 rx_packets, rx_bytes, tx_packets, tx_bytes;
> + unsigned int start;
> +
> + do {
> + start = u64_stats_fetch_begin_irq(&stats->syncp);
> + rx_packets = stats->rx_packets;
> + tx_packets = stats->tx_packets;
> + rx_bytes = stats->rx_bytes;
> + tx_bytes = stats->tx_bytes;
> + } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
> +
> + tot->rx_packets += rx_packets;
> + tot->tx_packets += tx_packets;
> + tot->rx_bytes += rx_bytes;
> + tot->tx_bytes += tx_bytes;
> + tot->tx_dropped += stats->tx_dropped;
> + }
> +}
> +
> static void virtnet_stats(struct net_device *dev,
> struct rtnl_link_stats64 *tot)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> + struct virtnet_vf_pcpu_stats vf_stats;
> int cpu;
> unsigned int start;
>
> @@ -1490,6 +1574,13 @@ static void virtnet_stats(struct net_device *dev,
> tot->rx_dropped = dev->stats.rx_dropped;
> tot->rx_length_errors = dev->stats.rx_length_errors;
> tot->rx_frame_errors = dev->stats.rx_frame_errors;
> +
> + virtnet_get_vf_stats(dev, &vf_stats);
> + tot->rx_packets += vf_stats.rx_packets;
> + tot->tx_packets += vf_stats.tx_packets;
> + tot->rx_bytes += vf_stats.rx_bytes;
> + tot->tx_bytes += vf_stats.tx_bytes;
> + tot->tx_dropped += vf_stats.tx_dropped;
> }
>
> #ifdef CONFIG_NET_POLL_CONTROLLER
> @@ -2508,6 +2599,47 @@ static int virtnet_validate(struct virtio_device *vdev)
> return 0;
> }
>
> +static void __virtnet_vf_setup(struct net_device *ndev,
> + struct net_device *vf_netdev)
> +{
> + int ret;
> +
> + /* Align MTU of VF with master */
> + ret = dev_set_mtu(vf_netdev, ndev->mtu);
> + if (ret)
> + netdev_warn(vf_netdev,
> + "unable to change mtu to %u\n", ndev->mtu);
> +
> + if (netif_running(ndev)) {
> + ret = dev_open(vf_netdev);
> + if (ret)
> + netdev_warn(vf_netdev,
> + "unable to open: %d\n", ret);
> + }
> +}
> +
> +/* Setup VF as slave of the virtio device.
> + * Runs in workqueue to avoid recursion in netlink callbacks.
> + */
> +static void virtnet_vf_setup(struct work_struct *w)
> +{
> + struct virtnet_info *vi
> + = container_of(w, struct virtnet_info, vf_takeover.work);
> + struct net_device *ndev = vi->dev;
> + struct net_device *vf_netdev;
> +
> + if (!rtnl_trylock()) {
> + schedule_delayed_work(&vi->vf_takeover, 0);
> + return;
> + }
> +
> + vf_netdev = rtnl_dereference(vi->vf_netdev);
> + if (vf_netdev)
> + __virtnet_vf_setup(ndev, vf_netdev);
> +
> + rtnl_unlock();
> +}
> +
> static int virtnet_probe(struct virtio_device *vdev)
> {
> int i, err;
> @@ -2600,6 +2732,11 @@ static int virtnet_probe(struct virtio_device *vdev)
> }
>
> INIT_WORK(&vi->config_work, virtnet_config_changed_work);
> + INIT_DELAYED_WORK(&vi->vf_takeover, virtnet_vf_setup);
> +
> + vi->vf_stats = netdev_alloc_pcpu_stats(struct virtnet_vf_pcpu_stats);
> + if (!vi->vf_stats)
> + goto free_stats;
>
> /* If we can receive ANY GSO packets, we must allocate large ones. */
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> @@ -2634,7 +2771,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> */
> dev_err(&vdev->dev, "device MTU appears to have changed "
> "it is now %d < %d", mtu, dev->min_mtu);
> - goto free_stats;
> + goto free_vf_stats;
> }
>
> dev->mtu = mtu;
> @@ -2658,7 +2795,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
> err = init_vqs(vi);
> if (err)
> - goto free_stats;
> + goto free_vf_stats;
>
> #ifdef CONFIG_SYSFS
> if (vi->mergeable_rx_bufs)
> @@ -2712,6 +2849,8 @@ static int virtnet_probe(struct virtio_device *vdev)
> cancel_delayed_work_sync(&vi->refill);
> free_receive_page_frags(vi);
> virtnet_del_vqs(vi);
> +free_vf_stats:
> + free_percpu(vi->vf_stats);
> free_stats:
> free_percpu(vi->stats);
> free:
> @@ -2733,19 +2872,178 @@ static void remove_vq_common(struct virtnet_info *vi)
> virtnet_del_vqs(vi);
> }
>
> +static struct net_device *get_virtio_bymac(const u8 *mac)
> +{
> + struct net_device *dev;
> +
> + ASSERT_RTNL();
> +
> + for_each_netdev(&init_net, dev) {
> + if (dev->netdev_ops != &virtnet_netdev)
> + continue; /* not a virtio_net device */
> +
> + if (ether_addr_equal(mac, dev->perm_addr))
> + return dev;
> + }
> +
> + return NULL;
> +}
> +
> +static struct net_device *get_virtio_byref(struct net_device *vf_netdev)
> +{
> + struct net_device *dev;
> +
> + ASSERT_RTNL();
> +
> + for_each_netdev(&init_net, dev) {
> + struct virtnet_info *vi;
> +
> + if (dev->netdev_ops != &virtnet_netdev)
> + continue; /* not a virtio_net device */
> +
> + vi = netdev_priv(dev);
> + if (rtnl_dereference(vi->vf_netdev) == vf_netdev)
> + return dev; /* a match */
> + }
> +
> + return NULL;
> +}
> +
> +/* Called when VF is injecting data into network stack.
> + * Change the associated network device from VF to virtio.
> + * note: already called with rcu_read_lock
> + */
> +static rx_handler_result_t virtnet_vf_handle_frame(struct sk_buff **pskb)
> +{
> + struct sk_buff *skb = *pskb;
> + struct net_device *ndev = rcu_dereference(skb->dev->rx_handler_data);
> + struct virtnet_info *vi = netdev_priv(ndev);
> + struct virtnet_vf_pcpu_stats *pcpu_stats =
> + this_cpu_ptr(vi->vf_stats);
> +
> + skb->dev = ndev;
> +
> + u64_stats_update_begin(&pcpu_stats->syncp);
> + pcpu_stats->rx_packets++;
> + pcpu_stats->rx_bytes += skb->len;
> + u64_stats_update_end(&pcpu_stats->syncp);
> +
> + return RX_HANDLER_ANOTHER;
> +}
> +
> +static int virtnet_vf_join(struct net_device *vf_netdev,
> + struct net_device *ndev)
> +{
> + struct virtnet_info *vi = netdev_priv(ndev);
> + int ret;
> +
> + ret = netdev_rx_handler_register(vf_netdev,
> + virtnet_vf_handle_frame, ndev);
> + if (ret != 0) {
> + netdev_err(vf_netdev,
> + "can not register virtio VF receive handler (err = %d)\n",
> + ret);
> + goto rx_handler_failed;
> + }
> +
> + ret = netdev_upper_dev_link(vf_netdev, ndev, NULL);
> + if (ret != 0) {
> + netdev_err(vf_netdev,
> + "can not set master device %s (err = %d)\n",
> + ndev->name, ret);
> + goto upper_link_failed;
> + }
> +
> + /* set slave flag before open to prevent IPv6 addrconf */
> + vf_netdev->flags |= IFF_SLAVE;
> +
> + schedule_delayed_work(&vi->vf_takeover, VF_TAKEOVER_INT);
> +
> + call_netdevice_notifiers(NETDEV_JOIN, vf_netdev);
> +
> + netdev_info(vf_netdev, "joined to %s\n", ndev->name);
> + return 0;
> +
> +upper_link_failed:
> + netdev_rx_handler_unregister(vf_netdev);
> +rx_handler_failed:
> + return ret;
> +}
> +
> +static int virtnet_register_vf(struct net_device *vf_netdev)
> +{
> + struct net_device *ndev;
> + struct virtnet_info *vi;
> +
> + if (vf_netdev->addr_len != ETH_ALEN)
> + return NOTIFY_DONE;
> +
> + /* We will use the MAC address to locate the virtio_net interface to
> + * associate with the VF interface. If we don't find a matching
> + * virtio interface, move on.
> + */
> + ndev = get_virtio_bymac(vf_netdev->perm_addr);
> + if (!ndev)
> + return NOTIFY_DONE;
> +
> + vi = netdev_priv(ndev);
> + if (rtnl_dereference(vi->vf_netdev))
> + return NOTIFY_DONE;
> +
> + if (virtnet_vf_join(vf_netdev, ndev) != 0)
> + return NOTIFY_DONE;
> +
> + netdev_info(ndev, "VF registering %s\n", vf_netdev->name);
> +
> + dev_hold(vf_netdev);
> + rcu_assign_pointer(vi->vf_netdev, vf_netdev);
> +
> + return NOTIFY_OK;
> +}
> +
> +static int virtnet_unregister_vf(struct net_device *vf_netdev)
> +{
> + struct net_device *ndev;
> + struct virtnet_info *vi;
> +
> + ndev = get_virtio_byref(vf_netdev);
> + if (!ndev)
> + return NOTIFY_DONE;
> +
> + vi = netdev_priv(ndev);
> + cancel_delayed_work_sync(&vi->vf_takeover);
> +
> + netdev_info(ndev, "VF unregistering %s\n", vf_netdev->name);
> +
> + netdev_rx_handler_unregister(vf_netdev);
> + netdev_upper_dev_unlink(vf_netdev, ndev);
> + RCU_INIT_POINTER(vi->vf_netdev, NULL);
> + dev_put(vf_netdev);
> +
> + return NOTIFY_OK;
> +}
> +
> static void virtnet_remove(struct virtio_device *vdev)
> {
> struct virtnet_info *vi = vdev->priv;
> + struct net_device *vf_netdev;
>
> virtnet_cpu_notif_remove(vi);
>
> /* Make sure no work handler is accessing the device. */
> flush_work(&vi->config_work);
>
> + rtnl_lock();
> + vf_netdev = rtnl_dereference(vi->vf_netdev);
> + if (vf_netdev)
> + virtnet_unregister_vf(vf_netdev);
> + rtnl_unlock();
> +
> unregister_netdev(vi->dev);
>
> remove_vq_common(vi);
>
> + free_percpu(vi->vf_stats);
> free_percpu(vi->stats);
> free_netdev(vi->dev);
> }
> @@ -2823,6 +3121,42 @@ static struct virtio_driver virtio_net_driver = {
> #endif
> };
>
> +static int virtio_netdev_event(struct notifier_block *this,
> + unsigned long event, void *ptr)
> +{
> + struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> +
> + /* Skip our own events */
> + if (event_dev->netdev_ops == &virtnet_netdev)
> + return NOTIFY_DONE;
> +
> + /* Avoid non-Ethernet type devices */
> + if (event_dev->type != ARPHRD_ETHER)
> + return NOTIFY_DONE;
> +
> + /* Avoid Vlan dev with same MAC registering as VF */
> + if (is_vlan_dev(event_dev))
> + return NOTIFY_DONE;
> +
> + /* Avoid Bonding master dev with same MAC registering as VF */
> + if ((event_dev->priv_flags & IFF_BONDING) &&
> + (event_dev->flags & IFF_MASTER))
> + return NOTIFY_DONE;
> +
> + switch (event) {
> + case NETDEV_REGISTER:
> + return virtnet_register_vf(event_dev);
> + case NETDEV_UNREGISTER:
> + return virtnet_unregister_vf(event_dev);
> + default:
> + return NOTIFY_DONE;
> + }
> +}
> +
> +static struct notifier_block virtio_netdev_notifier = {
> + .notifier_call = virtio_netdev_event,
> +};
> +
> static __init int virtio_net_driver_init(void)
> {
> int ret;
> @@ -2841,6 +3175,8 @@ static __init int virtio_net_driver_init(void)
> ret = register_virtio_driver(&virtio_net_driver);
> if (ret)
> goto err_virtio;
> +
> + register_netdevice_notifier(&virtio_netdev_notifier);
> return 0;
> err_virtio:
> cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
> @@ -2853,6 +3189,7 @@ module_init(virtio_net_driver_init);
>
> static __exit void virtio_net_driver_exit(void)
> {
> + unregister_netdevice_notifier(&virtio_netdev_notifier);
> unregister_virtio_driver(&virtio_net_driver);
> cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
> cpuhp_remove_multi_state(virtionet_online);
> --
> 2.14.3
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Michael S. Tsirkin @ 2017-12-21 0:15 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Sridhar Samudrala, virtualization, alexander.duyck, netdev
In-Reply-To: <20171220143140.0a3dc7f1@cakuba.netronome.com>
On Wed, Dec 20, 2017 at 02:33:34PM -0800, Jakub Kicinski wrote:
> On Mon, 18 Dec 2017 16:40:36 -0800, Sridhar Samudrala wrote:
> > +static int virtio_netdev_event(struct notifier_block *this,
> > + unsigned long event, void *ptr)
> > +{
> > + struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> > +
> > + /* Skip our own events */
> > + if (event_dev->netdev_ops == &virtnet_netdev)
> > + return NOTIFY_DONE;
>
> I wonder how does this work WRT loop prevention. What if I have two
> virtio devices with the same MAC, what is preventing them from claiming
> each other? Is it only the check above (not sure what is "own" in
> comment referring to)?
I expect we will add a feature bit (VIRTIO_NET_F_MASTER) and it will
be host's responsibility not to add more than 1 such device.
> I'm worried the check above will not stop virtio from enslaving hyperv
> interfaces and vice versa, potentially leading to a loop, no? There is
> also the fact that it would be preferable to share the code between
> paravirt drivers, to avoid duplicated bugs.
>
> My suggestion during the previous discussion was to create a paravirt
> bond device, which will explicitly tell the OS which interfaces to
> bond, regardless of which driver they're using. Could be some form of
> ACPI/FW driver too, I don't know enough about x86 FW to suggest
> something fully fleshed :(
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Jakub Kicinski @ 2017-12-21 0:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Sridhar Samudrala, virtualization, alexander.duyck, netdev
In-Reply-To: <20171221021450-mutt-send-email-mst@kernel.org>
On Thu, 21 Dec 2017 02:15:31 +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 20, 2017 at 02:33:34PM -0800, Jakub Kicinski wrote:
> > On Mon, 18 Dec 2017 16:40:36 -0800, Sridhar Samudrala wrote:
> > > +static int virtio_netdev_event(struct notifier_block *this,
> > > + unsigned long event, void *ptr)
> > > +{
> > > + struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> > > +
> > > + /* Skip our own events */
> > > + if (event_dev->netdev_ops == &virtnet_netdev)
> > > + return NOTIFY_DONE;
> >
> > I wonder how does this work WRT loop prevention. What if I have two
> > virtio devices with the same MAC, what is preventing them from claiming
> > each other? Is it only the check above (not sure what is "own" in
> > comment referring to)?
>
> I expect we will add a feature bit (VIRTIO_NET_F_MASTER) and it will
> be host's responsibility not to add more than 1 such device.
Do you mean a virtio_net feature bit? That won't stop the loop with
a hyperv device. Unless we want every paravirt device to have such
bit and enforce there can be only one magic bond enslavement active in
the system.
Making the bonding information separate from the slaves seems so much
cleaner... No limitation on device types, or how many bonds user
chooses to create. No MAC matching...
> > I'm worried the check above will not stop virtio from enslaving hyperv
> > interfaces and vice versa, potentially leading to a loop, no? There is
> > also the fact that it would be preferable to share the code between
> > paravirt drivers, to avoid duplicated bugs.
> >
> > My suggestion during the previous discussion was to create a paravirt
> > bond device, which will explicitly tell the OS which interfaces to
> > bond, regardless of which driver they're using. Could be some form of
> > ACPI/FW driver too, I don't know enough about x86 FW to suggest
> > something fully fleshed :(
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox