* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Koichiro Den @ 2017-08-21 12:58 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: netdev, virtualization
In-Reply-To: <5352c98a-fa48-fcf9-c062-9986a317a1b0@redhat.com>
On Mon, 2017-08-21 at 20:33 +0800, Jason Wang wrote:
>
> On 2017年08月19日 14:38, Koichiro Den wrote:
> > Facing the possible unbounded delay relying on freeing on xmit path,
> > we also better to invoke and clear the upper layer zerocopy callback
> > beforehand to keep them from waiting for unbounded duration in vain.
> > For instance, this removes the possible deadlock in the case that the
> > upper layer is a zerocopy-enabled vhost-net.
> > This does not apply if napi_tx is enabled since it will be called in
> > reasonale time.
> >
> > Signed-off-by: Koichiro Den <den@klaipeden.com>
> > ---
> > drivers/net/virtio_net.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4302f313d9a7..f7deaa5b7b50 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> >
> > /* Don't wait up for transmitted skbs to be freed. */
> > if (!use_napi) {
> > + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> > + struct ubuf_info *uarg;
> > + uarg = skb_shinfo(skb)->destructor_arg;
> > + if (uarg->callback)
> > + uarg->callback(uarg, true);
> > + skb_shinfo(skb)->destructor_arg = NULL;
> > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> > + }
> > skb_orphan(skb);
> > nf_reset(skb);
> > }
>
>
> Interesting, deadlock could be treated as a a radical case of the
> discussion here https://patchwork.kernel.org/patch/3787671/.
Thanks for letting me know this one. I am going to read it.
>
> git grep tells more similar skb_orphan() cases. Do we need to change
> them all (or part)?
Maybe, even though it seems less likely that we may meet it than the one I
described as an example, such as virtio-net sandwiched between vhost-net.
>
> Actually, we may meet similar issues at many other places (e.g netem).
> Need to consider a complete solution for this. Figuring out all places
> that could delay a packet is a method.
Okay I will do it and post the result and a suggestion if possible. Thanks.
>
> Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/2] vhost-net: revert vhost_exceeds_maxpend logic to its original
From: Jason Wang @ 2017-08-21 12:40 UTC (permalink / raw)
To: Koichiro Den, mst; +Cc: netdev, kvm, virtualization
In-Reply-To: <0cbc33d6-eb1e-cea4-4cdf-a8a88932da62@redhat.com>
On 2017年08月21日 11:06, Jason Wang wrote:
>
>
> On 2017年08月19日 14:41, Koichiro Den wrote:
>> To depend on vq.num and the usage of VHOST_MAX_PEND is not succinct
>> and in some case unexpected, so revert its logic part only.
>
> Hi:
>
> Could you explain a little bit more on the case that is was not
> sufficent?
>
> Thanks
Just have another thought.
I wonder whether or not just use ulimit(memlock) is better here. It
looks more flexible.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Koichiro Den @ 2017-08-21 12:40 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <CAF=yD-LYLsQNt_5yYZ+PUwv04ZwaJUXn-YakH=tuh0TV+BG7iA@mail.gmail.com>
On Sun, 2017-08-20 at 16:49 -0400, Willem de Bruijn wrote:
> On Sat, Aug 19, 2017 at 2:38 AM, Koichiro Den <den@klaipeden.com> wrote:
> > Facing the possible unbounded delay relying on freeing on xmit path,
> > we also better to invoke and clear the upper layer zerocopy callback
> > beforehand to keep them from waiting for unbounded duration in vain.
>
> Good point.
>
> > For instance, this removes the possible deadlock in the case that the
> > upper layer is a zerocopy-enabled vhost-net.
> > This does not apply if napi_tx is enabled since it will be called in
> > reasonale time.
>
> Indeed. Btw, I am gathering data to eventually make napi the default
> mode. But that is taking some time.
I'm looking forward to it. Btw I'm just guessing somehow delayed napi disabling
seems to relieve the interrupt costs much, though it might need to be stateful
so sounds a bit offensive. That's to say, not-yet-used ones are not necessarily
going to be used in the near future, so we have to limit the delay with some
sort of counter. Just guessing as a virtio novice, so pls take it with a grain
of salt ;)
>
> >
> > Signed-off-by: Koichiro Den <den@klaipeden.com>
> > ---
> > drivers/net/virtio_net.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4302f313d9a7..f7deaa5b7b50 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> >
> > /* Don't wait up for transmitted skbs to be freed. */
> > if (!use_napi) {
> > + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> > + struct ubuf_info *uarg;
> > + uarg = skb_shinfo(skb)->destructor_arg;
> > + if (uarg->callback)
> > + uarg->callback(uarg, true);
> > + skb_shinfo(skb)->destructor_arg = NULL;
> > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> > + }
>
> Instead of open coding, this can use skb_zcopy_clear.
I didn't notice it, seems necessary and sufficient. Please let me repost.
Thank you.
>
> > skb_orphan(skb);
> > nf_reset(skb);
> > }
> > --
> > 2.9.4
> >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Jason Wang @ 2017-08-21 12:33 UTC (permalink / raw)
To: Koichiro Den, mst; +Cc: netdev, virtualization
In-Reply-To: <20170819063854.27010-1-den@klaipeden.com>
On 2017年08月19日 14:38, Koichiro Den wrote:
> Facing the possible unbounded delay relying on freeing on xmit path,
> we also better to invoke and clear the upper layer zerocopy callback
> beforehand to keep them from waiting for unbounded duration in vain.
> For instance, this removes the possible deadlock in the case that the
> upper layer is a zerocopy-enabled vhost-net.
> This does not apply if napi_tx is enabled since it will be called in
> reasonale time.
>
> Signed-off-by: Koichiro Den <den@klaipeden.com>
> ---
> drivers/net/virtio_net.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4302f313d9a7..f7deaa5b7b50 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>
> /* Don't wait up for transmitted skbs to be freed. */
> if (!use_napi) {
> + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> + struct ubuf_info *uarg;
> + uarg = skb_shinfo(skb)->destructor_arg;
> + if (uarg->callback)
> + uarg->callback(uarg, true);
> + skb_shinfo(skb)->destructor_arg = NULL;
> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> + }
> skb_orphan(skb);
> nf_reset(skb);
> }
Interesting, deadlock could be treated as a a radical case of the
discussion here https://patchwork.kernel.org/patch/3787671/.
git grep tells more similar skb_orphan() cases. Do we need to change
them all (or part)?
Actually, we may meet similar issues at many other places (e.g netem).
Need to consider a complete solution for this. Figuring out all places
that could delay a packet is a method.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC 2/3] virtio-iommu: device probing and operations
From: Jean-Philippe Brucker @ 2017-08-21 12:00 UTC (permalink / raw)
To: Tian, Kevin, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: cdall@linaro.org, lorenzo.pieralisi@arm.com, mst@redhat.com,
marc.zyngier@arm.com, joro@8bytes.org, will.deacon@arm.com,
robin.murphy@arm.com
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D190D7BAB1@SHSMSX101.ccr.corp.intel.com>
On 21/08/17 08:59, Tian, Kevin wrote:
>> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
>> Sent: Monday, April 24, 2017 11:06 PM
>>>>>> 1. Attach device
>>>>>> ----------------
>>>>>>
>>>>>> struct virtio_iommu_req_attach {
>>>>>> le32 address_space;
>>>>>> le32 device;
>>>>>> le32 flags/reserved;
>>>>>> };
>>>>>>
>>>>>> Attach a device to an address space. 'address_space' is an identifier
>>>>>> unique to the guest. If the address space doesn't exist in the IOMMU
>>>>>
>>>>> Based on your description this address space ID is per operation right?
>>>>> MAP/UNMAP and page-table sharing should have different ID spaces...
>>>>
>>>> I think it's simpler if we keep a single IOASID space per virtio-iommu
>>>> device, because the maximum number of address spaces (described by
>>>> ioasid_bits) might be a restriction of the pIOMMU. For page-table
>> sharing
>>>> you still need to define which devices will share a page directory using
>>>> ATTACH requests, though that interface is not set in stone.
>>>
>>> got you. yes VM is supposed to consume less IOASIDs than physically
>>> available. It doesn’t hurt to have one IOASID space for both IOVA
>>> map/unmap usages (one IOASID per device) and SVM usages (multiple
>>> IOASIDs per device). The former is digested by software and the latter
>>> will be bound to hardware.
>>>
>>
>> Hmm, I'm using address space indexed by IOASID for "classic" IOMMU, and
>> then contexts indexed by PASID when talking about SVM. So in my mind an
>> address space can have multiple sub-address-spaces (contexts). Number of
>> IOASIDs is a limitation of the pIOMMU, and number of PASIDs is a
>> limitation of the device. Therefore attaching devices to address spaces
>> would update the number of available contexts in that address space. The
>> terminology is not ideal, and I'd be happy to change it for something more
>> clear.
>>
>
> (sorry to pick up this old thread, as the .tex one is not good for review
> and this thread provides necessary background for IOASID).
>
> Hi, Jean,
>
> I'd like to hear more clarification regarding the relationship between
> IOASID and PASID. When reading back above explanation, it looks
> confusing to me now (though I might get the meaning months ago :/).
> At least Intel VT-d only understands PASID (or you can think IOASID
> =PASID). There is no such layered address space concept. Then for
> map/unmap type request, do you intend to steal some PASIDs for
> that purpose on such architecture (since IOASID is a mandatory field
> in map/unmap request)?
IOASID is a logical ID, it isn't used by hardware. The address space
concept in virtio-iommu allows to group endpoints together, so that they
have the same address space. I thought it was pretty much the same as
"domains" in VT-d? In any case, it is the same as domains in Linux. An
IOASID provides a handle for communication between virtio-iommu device and
driver, but unlike PASID, the IOASID number doesn't mean anything outside
of virtio-iommu.
I haven't introduced PASIDs in public virtio-iommu documents yet, but the
way I intend it, PASID != IOASID. We will still have a logical address
space identified by IOASID, that can contain multiple contexts identified
by PASID. At the moment, after the ATTACH request, an address space
contains a single anonymous context (NO PASID) that can be managed with
MAP/UNMAP requests. With virtio-iommu v0.4, structures look like the
following. The NO PASID context is implicit.
address space context
endpoint ----. .- mapping
endpoint ----+---- IOASID -------- NO PASID ----+- mapping
endpoint ----' '- mapping
I'd like to add a flag to ATTACH that says "don't create a default
anonymous context, I'll handle contexts myself". Then a new "ADD_TABLE"
request to handle contexts. When creating a context, the guest decides if
it wants to manage it via MAP/UNMAP requests (and a new "context" field),
or instead manage mappings itself by allocating a page directory and use
INVALIDATE requests.
address space context
endpoint ----. .- mapping
endpoint ----+---- IOASID ----+--- NO PASID ----+- mapping
endpoint ----' | '- mapping
+--- PASID 0 ---- pgd
| ...
'--- PASID N ---- pgd
In this example the guest chose to still have an anonymous context that
uses MAP/UNMAP, along with a few PASID contexts with their own page tables.
Thanks,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: make napi_tx param easier to grasp
From: Koichiro Den @ 2017-08-21 11:49 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <CAF=yD-JA_JPyUbDd=SR9Epvzw6-kbUcrfvwRWODwmeV+yHEMHw@mail.gmail.com>
On Sun, 2017-08-20 at 16:30 -0400, Willem de Bruijn wrote:
> On Sat, Aug 19, 2017 at 2:37 AM, Koichiro Den <den@klaipeden.com> wrote:
> > The module param napi_tx needs not to be writable for now since we do
> > not have any means of activating/deactivating it online,
>
> A virtio_net device inherits its napi tx mode from the global napi_tx flag
> on device up. It is possible to change the parameter and bring a device
> down/up to change the device mode.
>
> > @@ -1179,13 +1172,19 @@ static int virtnet_open(struct net_device *dev)
> > struct virtnet_info *vi = netdev_priv(dev);
> > int i;
> >
> > + /* Tx napi touches cachelines on the cpu handling tx interrupts.
> > Only
> > + * enable the feature if this is likely affine with the transmit
> > path.
> > + */
> > + if (!vi->affinity_hint_set)
> > + napi_tx = false;
> > +
>
> This disables napi globally if a specific device lacks affinity.
Now I see this is not appropriate since it just represents whether or not TX
napi is available, not that whether or not it is being turned on or off on a
particular device. Thank you.
To be honest I hoped to make it possible to see which mode it is currently
running on with ease, but I guess it's not nice to accomplish it with net sysfs
nor ethtool or whatever because it seems not much generic matter.
Thanks.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] drm: virtio: constify drm_fb_helper_funcs
From: Arvind Yadav @ 2017-08-21 11:07 UTC (permalink / raw)
To: airlied, kraxel, daniel.vetter; +Cc: linux-kernel, dri-devel, virtualization
drm_fb_helper_funcs are not supposed to change at runtime.
All functions working with drm_fb_helper_funcs provided
by <drm/drm_fb_helper.h> work with const drm_fb_helper_funcs.
So mark the non-const structs as const.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/gpu/drm/virtio/virtgpu_fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
index 33df067..61f33ec 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fb.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
@@ -309,7 +309,7 @@ static int virtio_gpu_fbdev_destroy(struct drm_device *dev,
return 0;
}
-static struct drm_fb_helper_funcs virtio_gpu_fb_helper_funcs = {
+static const struct drm_fb_helper_funcs virtio_gpu_fb_helper_funcs = {
.fb_probe = virtio_gpufb_create,
};
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next 4/4] mlx4: sizeof style usage
From: Tariq Toukan @ 2017-08-21 10:53 UTC (permalink / raw)
To: Stephen Hemminger, Tariq Toukan, Stephen Hemminger,
mlindner@marvell.com, mst@redhat.com, jasowang@redhat.com
Cc: linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <DM2PR21MB0074E2B2C697B1300B77EFEACC860@DM2PR21MB0074.namprd21.prod.outlook.com>
On 20/08/2017 9:00 PM, Stephen Hemminger wrote:
> Yes, good catch.
>
OK, I will include a fix for this in my next series.
^ permalink raw reply
* RE: [RFC 2/3] virtio-iommu: device probing and operations
From: Tian, Kevin @ 2017-08-21 7:59 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: cdall@linaro.org, lorenzo.pieralisi@arm.com, mst@redhat.com,
marc.zyngier@arm.com, joro@8bytes.org, will.deacon@arm.com,
robin.murphy@arm.com
In-Reply-To: <a4df3472-449f-297f-2a18-b3a21affadee@arm.com>
> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
> Sent: Monday, April 24, 2017 11:06 PM
> >>>> 1. Attach device
> >>>> ----------------
> >>>>
> >>>> struct virtio_iommu_req_attach {
> >>>> le32 address_space;
> >>>> le32 device;
> >>>> le32 flags/reserved;
> >>>> };
> >>>>
> >>>> Attach a device to an address space. 'address_space' is an identifier
> >>>> unique to the guest. If the address space doesn't exist in the IOMMU
> >>>
> >>> Based on your description this address space ID is per operation right?
> >>> MAP/UNMAP and page-table sharing should have different ID spaces...
> >>
> >> I think it's simpler if we keep a single IOASID space per virtio-iommu
> >> device, because the maximum number of address spaces (described by
> >> ioasid_bits) might be a restriction of the pIOMMU. For page-table
> sharing
> >> you still need to define which devices will share a page directory using
> >> ATTACH requests, though that interface is not set in stone.
> >
> > got you. yes VM is supposed to consume less IOASIDs than physically
> > available. It doesn’t hurt to have one IOASID space for both IOVA
> > map/unmap usages (one IOASID per device) and SVM usages (multiple
> > IOASIDs per device). The former is digested by software and the latter
> > will be bound to hardware.
> >
>
> Hmm, I'm using address space indexed by IOASID for "classic" IOMMU, and
> then contexts indexed by PASID when talking about SVM. So in my mind an
> address space can have multiple sub-address-spaces (contexts). Number of
> IOASIDs is a limitation of the pIOMMU, and number of PASIDs is a
> limitation of the device. Therefore attaching devices to address spaces
> would update the number of available contexts in that address space. The
> terminology is not ideal, and I'd be happy to change it for something more
> clear.
>
(sorry to pick up this old thread, as the .tex one is not good for review
and this thread provides necessary background for IOASID).
Hi, Jean,
I'd like to hear more clarification regarding the relationship between
IOASID and PASID. When reading back above explanation, it looks
confusing to me now (though I might get the meaning months ago :/).
At least Intel VT-d only understands PASID (or you can think IOASID
=PASID). There is no such layered address space concept. Then for
map/unmap type request, do you intend to steal some PASIDs for
that purpose on such architecture (since IOASID is a mandatory field
in map/unmap request)?
Thanks
Kevin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v14 4/5] mm: support reporting free page blocks
From: Michal Hocko @ 2017-08-21 6:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170818201946-mutt-send-email-mst@kernel.org>
On Fri 18-08-17 20:23:05, Michael S. Tsirkin wrote:
> On Thu, Aug 17, 2017 at 11:26:55AM +0800, Wei Wang wrote:
[...]
> > +void walk_free_mem_block(void *opaque1,
> > + unsigned int min_order,
> > + void (*visit)(void *opaque2,
>
> You can just avoid opaque2 completely I think, then opaque1 can
> be renamed opaque.
>
> > + unsigned long pfn,
> > + unsigned long nr_pages))
> > +{
> > + struct zone *zone;
> > + struct page *page;
> > + struct list_head *list;
> > + unsigned int order;
> > + enum migratetype mt;
> > + unsigned long pfn, flags;
> > +
> > + for_each_populated_zone(zone) {
> > + for (order = MAX_ORDER - 1;
> > + order < MAX_ORDER && order >= min_order; order--) {
> > + for (mt = 0; mt < MIGRATE_TYPES; mt++) {
> > + spin_lock_irqsave(&zone->lock, flags);
> > + list = &zone->free_area[order].free_list[mt];
> > + list_for_each_entry(page, list, lru) {
> > + pfn = page_to_pfn(page);
> > + visit(opaque1, pfn, 1 << order);
>
> My only concern here is inability of callback to
> 1. break out of list
> 2. remove page from the list
As I've said before this has to be a read only API. You cannot simply
fiddle with the page allocator internals under its feet.
> So I would make the callback bool, and I would use
> list_for_each_entry_safe.
If a bool would tell to break out of the loop then I agree. This sounds
useful.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v14 4/5] mm: support reporting free page blocks
From: Michal Hocko @ 2017-08-21 6:14 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, mawilcox, linux-kernel, willy, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mgorman
In-Reply-To: <599A79DF.2000707@intel.com>
On Mon 21-08-17 14:12:47, Wei Wang wrote:
> On 08/18/2017 09:46 PM, Michal Hocko wrote:
[...]
> >>+/**
> >>+ * walk_free_mem_block - Walk through the free page blocks in the system
> >>+ * @opaque1: the context passed from the caller
> >>+ * @min_order: the minimum order of free lists to check
> >>+ * @visit: the callback function given by the caller
> >The original suggestion for using visit was motivated by a visit design
> >pattern but I can see how this can be confusing. Maybe a more explicit
> >name wold be better. What about report_free_range.
>
>
> I'm afraid that name would be too long to fit in nicely.
> How about simply naming it "report"?
I do not have a strong opinion on this. I wouldn't be afraid of using
slightly longer name here for the clarity sake, though.
> >>+ *
> >>+ * The function is used to walk through the free page blocks in the system,
> >>+ * and each free page block is reported to the caller via the @visit callback.
> >>+ * Please note:
> >>+ * 1) The function is used to report hints of free pages, so the caller should
> >>+ * not use those reported pages after the callback returns.
> >>+ * 2) The callback is invoked with the zone->lock being held, so it should not
> >>+ * block and should finish as soon as possible.
> >I think that the explicit note about zone->lock is not really need. This
> >can change in future and I would even bet that somebody might rely on
> >the lock being held for some purpose and silently get broken with the
> >change. Instead I would much rather see something like the following:
> >"
> >Please note that there are no locking guarantees for the callback
>
> Just a little confused with this one:
>
> The callback is invoked within zone->lock, why would we claim it "no
> locking guarantees for the callback"?
Because we definitely do not want anybody to rely on that fact and
(ab)use it. This might change in future and it would be better to be
clear about that.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v14 4/5] mm: support reporting free page blocks
From: Wei Wang @ 2017-08-21 6:12 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, mawilcox, linux-kernel, willy, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mgorman
In-Reply-To: <20170818134650.GC18499@dhcp22.suse.cz>
On 08/18/2017 09:46 PM, Michal Hocko wrote:
> On Thu 17-08-17 11:26:55, Wei Wang wrote:
>> This patch adds support to walk through the free page blocks in the
>> system and report them via a callback function. Some page blocks may
>> leave the free list after zone->lock is released, so it is the caller's
>> responsibility to either detect or prevent the use of such pages.
> This could see more details to be honest. Especially the usecase you are
> going to use this for. This will help us to understand the motivation
> in future when the current user might be gone a new ones largely diverge
> into a different usage. This wouldn't be the first time I have seen
> something like that.
OK, I will more details here about how it's used to accelerate live
migration.
>> 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>
>> ---
>> include/linux/mm.h | 6 ++++++
>> mm/page_alloc.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 50 insertions(+)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 46b9ac5..cd29b9f 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1835,6 +1835,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 *opaque1,
>> + unsigned int min_order,
>> + void (*visit)(void *opaque2,
>> + unsigned long pfn,
>> + unsigned long nr_pages));
>> +
>> /*
>> * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
>> * into the buddy system. The freed pages will be poisoned with pattern
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 6d00f74..a721a35 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -4762,6 +4762,50 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
>> show_swap_cache_info();
>> }
>>
>> +/**
>> + * walk_free_mem_block - Walk through the free page blocks in the system
>> + * @opaque1: the context passed from the caller
>> + * @min_order: the minimum order of free lists to check
>> + * @visit: the callback function given by the caller
> The original suggestion for using visit was motivated by a visit design
> pattern but I can see how this can be confusing. Maybe a more explicit
> name wold be better. What about report_free_range.
I'm afraid that name would be too long to fit in nicely.
How about simply naming it "report"?
>
>> + *
>> + * The function is used to walk through the free page blocks in the system,
>> + * and each free page block is reported to the caller via the @visit callback.
>> + * Please note:
>> + * 1) The function is used to report hints of free pages, so the caller should
>> + * not use those reported pages after the callback returns.
>> + * 2) The callback is invoked with the zone->lock being held, so it should not
>> + * block and should finish as soon as possible.
> I think that the explicit note about zone->lock is not really need. This
> can change in future and I would even bet that somebody might rely on
> the lock being held for some purpose and silently get broken with the
> change. Instead I would much rather see something like the following:
> "
> Please note that there are no locking guarantees for the callback
Just a little confused with this one:
The callback is invoked within zone->lock, why would we claim it "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 for various optimizations which like
> ballooning etc... This will reduce the overhead as well.
> "
I think it looks quite comprehensive. Thanks.
Best,
Wei
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v14 5/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Wei Wang @ 2017-08-21 5:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <20170818202337-mutt-send-email-mst@kernel.org>
On 08/19/2017 02:10 AM, Michael S. Tsirkin wrote:
> On Fri, Aug 18, 2017 at 04:36:06PM +0800, Wei Wang wrote:
>> On 08/18/2017 10:28 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 17, 2017 at 11:26:56AM +0800, Wei Wang wrote:
>>>> Add a new vq to report hints of guest free pages to the host.
>>>>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>>> Signed-off-by: Liang Li <liang.z.li@intel.com>
>>>> ---
>>>> drivers/virtio/virtio_balloon.c | 167 +++++++++++++++++++++++++++++++-----
>>>> include/uapi/linux/virtio_balloon.h | 1 +
>>>> 2 files changed, 147 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>>>> index 72041b4..e6755bc 100644
>>>> --- a/drivers/virtio/virtio_balloon.c
>>>> +++ b/drivers/virtio/virtio_balloon.c
>>>> @@ -54,11 +54,12 @@ static struct vfsmount *balloon_mnt;
>>>> struct virtio_balloon {
>>>> struct virtio_device *vdev;
>>>> - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
>>>> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
>>>> /* The balloon servicing is delegated to a freezable workqueue. */
>>>> struct work_struct update_balloon_stats_work;
>>>> struct work_struct update_balloon_size_work;
>>>> + struct work_struct report_free_page_work;
>>>> /* Prevent updating balloon when it is being canceled. */
>>>> spinlock_t stop_update_lock;
>>>> @@ -90,6 +91,13 @@ struct virtio_balloon {
>>>> /* Memory statistics */
>>>> struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
>>>> + /*
>>>> + * Used by the device and driver to signal each other.
>>>> + * device->driver: start the free page report.
>>>> + * driver->device: end the free page report.
>>>> + */
>>>> + __virtio32 report_free_page_signal;
>>>> +
>>>> /* To register callback in oom notifier call chain */
>>>> struct notifier_block nb;
>>>> };
>>>> @@ -174,6 +182,17 @@ static void send_balloon_page_sg(struct virtio_balloon *vb,
>>>> } while (unlikely(ret == -ENOSPC));
>>>> }
>>>> +static void send_free_page_sg(struct virtqueue *vq, void *addr, uint32_t size)
>>>> +{
>>>> + unsigned int len;
>>>> +
>>>> + add_one_sg(vq, addr, size);
>>>> + virtqueue_kick(vq);
>>>> + /* Release entries if there are */
>>>> + while (virtqueue_get_buf(vq, &len))
>>>> + ;
>>>> +}
>>>> +
>>>> /*
>>>> * 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.
>>>> @@ -511,42 +530,143 @@ static void update_balloon_size_func(struct work_struct *work)
>>>> queue_work(system_freezable_wq, work);
>>>> }
>>>> +static void virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
>>>> + unsigned long nr_pages)
>>>> +{
>>>> + struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
>>>> + void *addr = (void *)pfn_to_kaddr(pfn);
>>>> + uint32_t len = nr_pages << PAGE_SHIFT;
>>>> +
>>>> + send_free_page_sg(vb->free_page_vq, addr, len);
>>>> +}
>>>> +
>>>> +static void report_free_page_completion(struct virtio_balloon *vb)
>>>> +{
>>>> + struct virtqueue *vq = vb->free_page_vq;
>>>> + struct scatterlist sg;
>>>> + unsigned int len;
>>>> + int ret;
>>>> +
>>>> + sg_init_one(&sg, &vb->report_free_page_signal, sizeof(__virtio32));
>>>> +retry:
>>>> + ret = virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
>>>> + virtqueue_kick(vq);
>>>> + if (unlikely(ret == -ENOSPC)) {
>>>> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
>>>> + goto retry;
>>>> + }
>>>> +}
>>> So the annoying thing here is that once this starts going,
>>> it will keep sending free pages from the list even if
>>> host is no longer interested. There should be a way
>>> for host to tell guest "stop" or "start from the beginning".
>> This can be achieved via two output signal buf here:
>> signal_buf_start: filled with VIRTIO_BALLOON_F_FREE_PAGE_REPORT_START
>> signal_buf_end: filled with VIRTIO_BALLOON_F_FREE_PAGE_REPORT_END
>>
>> The device holds both, and can put one of them to the vq and notify.
> Do you mean device writes start and end in the buf? then it's an inbuf
> not an outbuf.
>
Not really. I meant that the driver fills two signal buffer,_START and _STOP
and send them as outbuf to the device. Then the device holds two read-only
signal buffer:
When request to start: add the _START elem to the vq
When request to stop: add the _STOP elem to the vq
>>
>>> It's the result of using same vq for guest to host and
>>> host to guest communication, and I think it's not a great idea.
>>> I'd reuse stats vq for host to guest requests maybe.
>>>
>>
>> As we discussed before, we can't have a vq interleave the report of stats
>> and free pages.
>> The vq will be locked when one command is in use. So, when live migration
>> starts, the
>> periodically reported stats will be delayed.
>
>
>
>
>
>> Would this be OK? Or would you
>> like to have
>> one host to guest vq, and multiple host to guest vqs? That is,
>>
>> - host to guest:
>> CMD_VQ
>>
>> - guest to host:
>> STATS_REPORT_VQ
>> FREE_PAGE_VQ
>>
>>
>> Best,
>> Wei
>>
> Point is stats report vq is also host to guest.
> So I think it can be combined with CMD VQ.
> If it's too hard a separate vq isn't too bad though.
>
IMHO, this kind of categorization - using stat_vq for
host-to-guest stats request,
guest-to-host stats report,
host-to-guest free page request,
seems a little tricky and unclear. I would think it is better to have
unrelated
features decoupled. For example, both stats report and free page report
are optional features. With the above implementation, using the free page
feature will depend on the stats report feature (in fact we can implement
it to have the free page feature work independently)
That being said, if you don't mind the above, we can go with that option
in the next version.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v14 5/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Wei Wang @ 2017-08-21 5:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <20170818211119-mutt-send-email-mst@kernel.org>
On 08/19/2017 02:26 AM, Michael S. Tsirkin wrote:
> On Fri, Aug 18, 2017 at 04:41:41PM +0800, Wei Wang wrote:
>> On 08/18/2017 10:13 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 17, 2017 at 11:26:56AM +0800, Wei Wang wrote:
>>>> Add a new vq to report hints of guest free pages to the host.
>>> Please add some text here explaining the report_free_page_signal
>>> thing.
>>>
>>>
>>> I also really think we need some kind of ID in the
>>> buffer to do a handshake. whenever id changes you
>>> add another outbuf.
>> Please let me introduce the current design first:
>> 1) device put the signal buf to the vq and notify the driver (we need
>> a buffer because currently the device can't notify when the vq is empty);
>>
>> 2) the driver starts the report of free page blocks via inbuf;
>>
>> 3) the driver adds an the signal buf via outbuf to tell the device all are
>> reported.
>>
>>
>> Could you please elaborate more on the usage of ID?
> While driver is free to maintain at most one buffer in flight
> the design must work with pipelined requests as that
> is important for performance.
How would the pipeline be designed?
Currently, once the report starts,
- the driver work: add_inbuf(free_pages) & kick;
- the device work:
record the pages into a free page bitmap;
virtqueue_push(elem);
virtio_notify();
For the driver, as long as the vq has available entries, it keeps doing
its work;
For the device, as long as there are free pages in the vq, it also keeps
doing its work.
>
> So host might be able to request the reporting twice.
> How does it know what is the report in response to?
The request to start is sent when live migration starts, where would be
the second chance to send the request to start?
>
> If we put an id in request and in response, then that fixes it.
>
>
> So there's a vq used for requesting free page reports.
> driver does add_inbuf( &device->id).
>
> Then when it starts reporting it does
>
>
> add_outbuf(&device->id)
>
> followed by pages.
>
>
> Also if device->id changes it knows it should restart
> reporting from beginning.
>
>
>
>
>
>
>>>> +retry:
>>>> + ret = virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
>>>> + virtqueue_kick(vq);
>>>> + if (unlikely(ret == -ENOSPC)) {
>>> what if there's another error?
>> Another error is -EIO, how about disabling the free page report feature?
>> (I also saw it isn't handled in many other virtio devices e.g. virtio-net)
>>
>>>> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
>>>> + goto retry;
>>>> + }
>>> what is this trickery doing? needs more comments or
>>> a simplification.
>> Just this:
>> if the vq is full, blocking wait till an entry gets released, then retry.
>> This is the
>> final one, which puts the signal buf to the vq to signify the end of the
>> report and
>> the mm lock is not held here, so it is fine to block.
>>
> But why do you kick here on failure? I would understand it if you
> did not kick when adding pages, as it is I don't understand.
>
>
> Also pls rewrite this with a for or while loop for clarity.
OK, I will rewrite this part.
Best,
Wei
^ permalink raw reply
* Re: [PATCH 1/2] vhost: remove the possible fruitless search on iotlb prefetch
From: Jason Wang @ 2017-08-21 3:09 UTC (permalink / raw)
To: Koichiro Den, mst; +Cc: netdev, kvm, virtualization
In-Reply-To: <20170819064114.27219-1-den@klaipeden.com>
On 2017年08月19日 14:41, Koichiro Den wrote:
> Signed-off-by: Koichiro Den <den@klaipeden.com>
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index e4613a3c362d..93e909afc1c3 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1184,7 +1184,7 @@ static int iotlb_access_ok(struct vhost_virtqueue *vq,
> while (len > s) {
> node = vhost_umem_interval_tree_iter_first(&umem->umem_tree,
> addr,
> - addr + len - 1);
> + addr + len - s - 1);
> if (node == NULL || node->start > addr) {
> vhost_iotlb_miss(vq, addr, access);
> return false;
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/2] vhost-net: revert vhost_exceeds_maxpend logic to its original
From: Jason Wang @ 2017-08-21 3:06 UTC (permalink / raw)
To: Koichiro Den, mst; +Cc: netdev, kvm, virtualization
In-Reply-To: <20170819064129.27272-1-den@klaipeden.com>
On 2017年08月19日 14:41, Koichiro Den wrote:
> To depend on vq.num and the usage of VHOST_MAX_PEND is not succinct
> and in some case unexpected, so revert its logic part only.
Hi:
Could you explain a little bit more on the case that is was not sufficent?
Thanks
>
> Signed-off-by: Koichiro Den <den@klaipeden.com>
> ---
> drivers/vhost/net.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 06d044862e58..99cf99b308a7 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -433,11 +433,15 @@ static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
>
> static bool vhost_exceeds_maxpend(struct vhost_net *net)
> {
> + int num_pends;
> struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
> struct vhost_virtqueue *vq = &nvq->vq;
>
> - return (nvq->upend_idx + vq->num - VHOST_MAX_PEND) % UIO_MAXIOV
> - == nvq->done_idx;
> + num_pends = likely(nvq->upend_idx >= nvq->done_idx) ?
> + (nvq->upend_idx - nvq->done_idx) :
> + (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx);
> +
> + return num_pends > VHOST_MAX_PEND;
> }
>
> /* Expects to be always run from workqueue - which acts as
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-20 20:49 UTC (permalink / raw)
To: Koichiro Den; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <20170819063854.27010-1-den@klaipeden.com>
On Sat, Aug 19, 2017 at 2:38 AM, Koichiro Den <den@klaipeden.com> wrote:
> Facing the possible unbounded delay relying on freeing on xmit path,
> we also better to invoke and clear the upper layer zerocopy callback
> beforehand to keep them from waiting for unbounded duration in vain.
Good point.
> For instance, this removes the possible deadlock in the case that the
> upper layer is a zerocopy-enabled vhost-net.
> This does not apply if napi_tx is enabled since it will be called in
> reasonale time.
Indeed. Btw, I am gathering data to eventually make napi the default
mode. But that is taking some time.
>
> Signed-off-by: Koichiro Den <den@klaipeden.com>
> ---
> drivers/net/virtio_net.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4302f313d9a7..f7deaa5b7b50 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>
> /* Don't wait up for transmitted skbs to be freed. */
> if (!use_napi) {
> + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> + struct ubuf_info *uarg;
> + uarg = skb_shinfo(skb)->destructor_arg;
> + if (uarg->callback)
> + uarg->callback(uarg, true);
> + skb_shinfo(skb)->destructor_arg = NULL;
> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> + }
Instead of open coding, this can use skb_zcopy_clear.
> skb_orphan(skb);
> nf_reset(skb);
> }
> --
> 2.9.4
>
>
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: make napi_tx param easier to grasp
From: Willem de Bruijn @ 2017-08-20 20:30 UTC (permalink / raw)
To: Koichiro Den; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <20170819063707.26822-1-den@klaipeden.com>
On Sat, Aug 19, 2017 at 2:37 AM, Koichiro Den <den@klaipeden.com> wrote:
> The module param napi_tx needs not to be writable for now since we do
> not have any means of activating/deactivating it online,
A virtio_net device inherits its napi tx mode from the global napi_tx flag
on device up. It is possible to change the parameter and bring a device
down/up to change the device mode.
> @@ -1179,13 +1172,19 @@ static int virtnet_open(struct net_device *dev)
> struct virtnet_info *vi = netdev_priv(dev);
> int i;
>
> + /* Tx napi touches cachelines on the cpu handling tx interrupts. Only
> + * enable the feature if this is likely affine with the transmit path.
> + */
> + if (!vi->affinity_hint_set)
> + napi_tx = false;
> +
This disables napi globally if a specific device lacks affinity.
^ permalink raw reply
* RE: [PATCH net-next 4/4] mlx4: sizeof style usage
From: Stephen Hemminger via Virtualization @ 2017-08-20 18:00 UTC (permalink / raw)
To: Tariq Toukan, Stephen Hemminger, mlindner@marvell.com,
mst@redhat.com, jasowang@redhat.com
Cc: linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <d38cac6d-77a7-e411-73b0-78adad9cbd5f@mellanox.com>
Yes, good catch.
-----Original Message-----
From: Tariq Toukan [mailto:tariqt@mellanox.com]
Sent: Sunday, August 20, 2017 3:27 AM
To: Stephen Hemminger <stephen@networkplumber.org>; mlindner@marvell.com; mst@redhat.com; jasowang@redhat.com
Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; virtualization@lists.linux-foundation.org; Stephen Hemminger <sthemmin@microsoft.com>
Subject: Re: [PATCH net-next 4/4] mlx4: sizeof style usage
[You don't often get email from TARIQT@MELLANOX.COM. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]
Thanks Stephen.
Sorry for the late reply, I was on vacation.
I know this is already accepted, but still I have one comment.
On 15/08/2017 8:29 PM, Stephen Hemminger wrote:
> The kernel coding style is to treat sizeof as a function
> (ie. with parenthesis) not as an operator.
>
> Also use kcalloc and kmalloc_array
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> @@ -726,7 +726,7 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
> }
> memcpy(&priv->mfunc.master.comm_arm_bit_vector,
> eqe->event.comm_channel_arm.bit_vec,
> - sizeof eqe->event.comm_channel_arm.bit_vec);
> + sizeof(eqe)->event.comm_channel_arm.bit_vec);
I think the brackets here are misplaced.
Shouldn't they be as follows?
sizeof(eqe->event.comm_channel_arm.bit_vec));
> queue_work(priv->mfunc.master.comm_wq,
> &priv->mfunc.master.comm_work);
> break;
Thanks,
Tariq
^ permalink raw reply
* Re: [PATCH net-next 4/4] mlx4: sizeof style usage
From: Tariq Toukan @ 2017-08-20 10:27 UTC (permalink / raw)
To: Stephen Hemminger, mlindner, mst, jasowang
Cc: linux-rdma, netdev, Stephen Hemminger, virtualization
In-Reply-To: <20170815172919.26153-5-sthemmin@microsoft.com>
Thanks Stephen.
Sorry for the late reply, I was on vacation.
I know this is already accepted, but still I have one comment.
On 15/08/2017 8:29 PM, Stephen Hemminger wrote:
> The kernel coding style is to treat sizeof as a function
> (ie. with parenthesis) not as an operator.
>
> Also use kcalloc and kmalloc_array
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> @@ -726,7 +726,7 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
> }
> memcpy(&priv->mfunc.master.comm_arm_bit_vector,
> eqe->event.comm_channel_arm.bit_vec,
> - sizeof eqe->event.comm_channel_arm.bit_vec);
> + sizeof(eqe)->event.comm_channel_arm.bit_vec);
I think the brackets here are misplaced.
Shouldn't they be as follows?
sizeof(eqe->event.comm_channel_arm.bit_vec));
> queue_work(priv->mfunc.master.comm_wq,
> &priv->mfunc.master.comm_work);
> break;
Thanks,
Tariq
^ permalink raw reply
* (unknown),
From: Solen win2 @ 2017-08-20 2:58 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 4 bytes --]
all
[-- Attachment #1.2: Type: text/html, Size: 79 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v14 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: kbuild test robot @ 2017-08-19 21:37 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, cornelia.huck, mhocko, linux-kernel, kbuild-all,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1502940416-42944-4-git-send-email-wei.w.wang@intel.com>
[-- Attachment #1: Type: text/plain, Size: 2808 bytes --]
Hi Wei,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.13-rc5 next-20170817]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Wei-Wang/lib-xbitmap-Introduce-xbitmap/20170820-035516
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/virtio/virtio_balloon.c: In function 'tell_host_sgs':
>> drivers/virtio/virtio_balloon.c:203:3: error: implicit declaration of function 'pfn_to_kaddr' [-Werror=implicit-function-declaration]
sg_addr = (void *)pfn_to_kaddr(sg_pfn_start);
^
cc1: some warnings being treated as errors
vim +/pfn_to_kaddr +203 drivers/virtio/virtio_balloon.c
176
177 /*
178 * Send balloon pages in sgs to host. The balloon pages are recorded in the
179 * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
180 * The page xbitmap is searched for continuous "1" bits, which correspond
181 * to continuous pages, to chunk into sgs.
182 *
183 * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
184 * need to be searched.
185 */
186 static void tell_host_sgs(struct virtio_balloon *vb,
187 struct virtqueue *vq,
188 unsigned long page_xb_start,
189 unsigned long page_xb_end)
190 {
191 unsigned long sg_pfn_start, sg_pfn_end;
192 void *sg_addr;
193 uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
194
195 sg_pfn_start = page_xb_start;
196 while (sg_pfn_start < page_xb_end) {
197 sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
198 page_xb_end, 1);
199 if (sg_pfn_start == page_xb_end + 1)
200 break;
201 sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
202 page_xb_end, 0);
> 203 sg_addr = (void *)pfn_to_kaddr(sg_pfn_start);
204 sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
205 while (sg_len > sg_max_len) {
206 send_balloon_page_sg(vb, vq, sg_addr, sg_max_len);
207 sg_addr += sg_max_len;
208 sg_len -= sg_max_len;
209 }
210 send_balloon_page_sg(vb, vq, sg_addr, sg_len);
211 xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
212 sg_pfn_start = sg_pfn_end + 1;
213 }
214 }
215
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50926 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v14 1/5] lib/xbitmap: Introduce xbitmap
From: kbuild test robot @ 2017-08-19 20:30 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, cornelia.huck, mhocko, linux-kernel, kbuild-all,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1502940416-42944-2-git-send-email-wei.w.wang@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]
Hi Matthew,
[auto build test WARNING on linus/master]
[also build test WARNING on v4.13-rc5 next-20170817]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Wei-Wang/lib-xbitmap-Introduce-xbitmap/20170820-035516
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
lib/xbitmap.c: In function 'xb_test_bit':
>> lib/xbitmap.c:153:26: warning: passing argument 1 of 'xb_bit_ops' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
return (bool)xb_bit_ops(xb, bit, XB_TEST);
^~
lib/xbitmap.c:23:12: note: expected 'struct xb *' but argument is of type 'const struct xb *'
static int xb_bit_ops(struct xb *xb, unsigned long bit, enum xb_ops ops)
^~~~~~~~~~
vim +153 lib/xbitmap.c
142
143 /**
144 * xb_test_bit - test a bit in the xbitmap
145 * @xb: the xbitmap tree used to record the bit
146 * @bit: index of the bit to set
147 *
148 * This function is used to test a bit in the xbitmap.
149 * Returns: 1 if the bit is set, or 0 otherwise.
150 */
151 bool xb_test_bit(const struct xb *xb, unsigned long bit)
152 {
> 153 return (bool)xb_bit_ops(xb, bit, XB_TEST);
154 }
155 EXPORT_SYMBOL(xb_test_bit);
156
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6665 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 2/2] vhost-net: revert vhost_exceeds_maxpend logic to its original
From: Koichiro Den @ 2017-08-19 6:41 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, kvm, virtualization
To depend on vq.num and the usage of VHOST_MAX_PEND is not succinct
and in some case unexpected, so revert its logic part only.
Signed-off-by: Koichiro Den <den@klaipeden.com>
---
drivers/vhost/net.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 06d044862e58..99cf99b308a7 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -433,11 +433,15 @@ static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
static bool vhost_exceeds_maxpend(struct vhost_net *net)
{
+ int num_pends;
struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
struct vhost_virtqueue *vq = &nvq->vq;
- return (nvq->upend_idx + vq->num - VHOST_MAX_PEND) % UIO_MAXIOV
- == nvq->done_idx;
+ num_pends = likely(nvq->upend_idx >= nvq->done_idx) ?
+ (nvq->upend_idx - nvq->done_idx) :
+ (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx);
+
+ return num_pends > VHOST_MAX_PEND;
}
/* Expects to be always run from workqueue - which acts as
--
2.9.4
^ permalink raw reply related
* [PATCH 1/2] vhost: remove the possible fruitless search on iotlb prefetch
From: Koichiro Den @ 2017-08-19 6:41 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, kvm, virtualization
Signed-off-by: Koichiro Den <den@klaipeden.com>
---
drivers/vhost/vhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e4613a3c362d..93e909afc1c3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1184,7 +1184,7 @@ static int iotlb_access_ok(struct vhost_virtqueue *vq,
while (len > s) {
node = vhost_umem_interval_tree_iter_first(&umem->umem_tree,
addr,
- addr + len - 1);
+ addr + len - s - 1);
if (node == NULL || node->start > addr) {
vhost_iotlb_miss(vq, addr, access);
return false;
--
2.9.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