* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-03-12 2:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Andrea Arcangeli, kvm, netdev, linux-kernel, virtualization,
linux-mm, Jerome Glisse
In-Reply-To: <20190311084525-mutt-send-email-mst@kernel.org>
On 2019/3/11 下午8:48, Michael S. Tsirkin wrote:
> On Mon, Mar 11, 2019 at 03:40:31PM +0800, Jason Wang wrote:
>> On 2019/3/9 上午3:48, Andrea Arcangeli wrote:
>>> Hello Jeson,
>>>
>>> On Fri, Mar 08, 2019 at 04:50:36PM +0800, Jason Wang wrote:
>>>> Just to make sure I understand here. For boosting through huge TLB, do
>>>> you mean we can do that in the future (e.g by mapping more userspace
>>>> pages to kenrel) or it can be done by this series (only about three 4K
>>>> pages were vmapped per virtqueue)?
>>> When I answered about the advantages of mmu notifier and I mentioned
>>> guaranteed 2m/gigapages where available, I overlooked the detail you
>>> were using vmap instead of kmap. So with vmap you're actually doing
>>> the opposite, it slows down the access because it will always use a 4k
>>> TLB even if QEMU runs on THP or gigapages hugetlbfs.
>>>
>>> If there's just one page (or a few pages) in each vmap there's no need
>>> of vmap, the linearity vmap provides doesn't pay off in such
>>> case.
>>>
>>> So likely there's further room for improvement here that you can
>>> achieve in the current series by just dropping vmap/vunmap.
>>>
>>> You can just use kmap (or kmap_atomic if you're in preemptible
>>> section, should work from bh/irq).
>>>
>>> In short the mmu notifier to invalidate only sets a "struct page *
>>> userringpage" pointer to NULL without calls to vunmap.
>>>
>>> In all cases immediately after gup_fast returns you can always call
>>> put_page immediately (which explains why I'd like an option to drop
>>> FOLL_GET from gup_fast to speed it up).
>>>
>>> Then you can check the sequence_counter and inc/dec counter increased
>>> by _start/_end. That will tell you if the page you got and you called
>>> put_page to immediately unpin it or even to free it, cannot go away
>>> under you until the invalidate is called.
>>>
>>> If sequence counters and counter tells that gup_fast raced with anyt
>>> mmu notifier invalidate you can just repeat gup_fast. Otherwise you're
>>> done, the page cannot go away under you, the host virtual to host
>>> physical mapping cannot change either. And the page is not pinned
>>> either. So you can just set the "struct page * userringpage = page"
>>> where "page" was the one setup by gup_fast.
>>>
>>> When later the invalidate runs, you can just call set_page_dirty if
>>> gup_fast was called with "write = 1" and then you clear the pointer
>>> "userringpage = NULL".
>>>
>>> When you need to read/write to the memory
>>> kmap/kmap_atomic(userringpage) should work.
>> Yes, I've considered kmap() from the start. The reason I don't do that is
>> large virtqueue may need more than one page so VA might not be contiguous.
>> But this is probably not a big issue which just need more tricks in the
>> vhost memory accessors.
>>
>>
>>> In short because there's no hardware involvement here, the established
>>> mapping is just the pointer to the page, there is no need of setting
>>> up any pagetables or to do any TLB flushes (except on 32bit archs if
>>> the page is above the direct mapping but it never happens on 64bit
>>> archs).
>> I see, I believe we don't care much about the performance of 32bit archs (or
>> we can just fallback to copy_to_user() friends).
> Using copyXuser is better I guess.
Ok.
>
>> Using direct mapping (I
>> guess kernel will always try hugepage for that?) should be better and we can
>> even use it for the data transfer not only for the metadata.
>>
>> Thanks
> We can't really. The big issue is get user pages. Doing that on data
> path will be slower than copyXuser.
I meant if we can find a way to avoid doing gup in datapath. E.g vhost
maintain a range tree and add or remove ranges through MMU notifier.
Then in datapath, if we find the range, then use direct mapping
otherwise copy_to_user().
Thanks
> Or maybe it won't with the
> amount of mitigations spread around. Go ahead and try.
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-03-12 2:56 UTC (permalink / raw)
To: Andrea Arcangeli, Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, linux-mm,
Jerome Glisse
In-Reply-To: <20190311134305.GC23321@redhat.com>
On 2019/3/11 下午9:43, Andrea Arcangeli wrote:
> On Mon, Mar 11, 2019 at 08:48:37AM -0400, Michael S. Tsirkin wrote:
>> Using copyXuser is better I guess.
> It certainly would be faster there, but I don't think it's needed if
> that would be the only use case left that justifies supporting two
> different models. On small 32bit systems with little RAM kmap won't
> perform measurably different on 32bit or 64bit systems. If the 32bit
> host has a lot of ram it all gets slow anyway at accessing RAM above
> the direct mapping, if compared to 64bit host kernels, it's not just
> an issue for vhost + mmu notifier + kmap and the best way to optimize
> things is to run 64bit host kernels.
>
> Like Christoph pointed out, the main use case for retaining the
> copy-user model would be CPUs with virtually indexed not physically
> tagged data caches (they'll still suffer from the spectre-v1 fix,
> although I exclude they have to suffer the SMAP
> slowdown/feature). Those may require some additional flushing than the
> current copy-user model requires.
>
> As a rule of thumb any arch where copy_user_page doesn't define as
> copy_page will require some additional cache flushing after the
> kmap. Supposedly with vmap, the vmap layer should have taken care of
> that (I didn't verify that yet).
vmap_page_range()/free_unmap_vmap_area() will call
fluch_cache_vmap()/flush_cache_vunmap(). So vmap layer should be ok.
Thanks
>
> There are some accessories like copy_to_user_page()
> copy_from_user_page() that could work and obviously defines to raw
> memcpy on x86 (the main cons is they don't provide word granular
> access) and at least on sparc they're tailored to ptrace assumptions
> so then we'd need to evaluate what happens if this is used outside of
> ptrace context. kmap has been used generally either to access whole
> pages (i.e. copy_user_page), so ptrace may actually be the only use
> case with subpage granularity access.
>
> #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
> do { \
> flush_cache_page(vma, vaddr, page_to_pfn(page)); \
> memcpy(dst, src, len); \
> flush_ptrace_access(vma, page, vaddr, src, len, 0); \
> } while (0)
>
> So I wouldn't rule out the need for a dual model, until we solve how
> to run this stable on non-x86 arches with not physically tagged
> caches.
>
> Thanks,
> Andrea
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-03-12 2:59 UTC (permalink / raw)
To: David Miller, mst
Cc: aarcange, linux-parisc, kvm, netdev, linux-kernel, virtualization,
hch, linux-mm, linux-arm-kernel
In-Reply-To: <20190311.111413.1140896328197448401.davem@davemloft.net>
On 2019/3/12 上午2:14, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Mon, 11 Mar 2019 09:59:28 -0400
>
>> On Mon, Mar 11, 2019 at 03:13:17PM +0800, Jason Wang wrote:
>>> On 2019/3/8 下午10:12, Christoph Hellwig wrote:
>>>> On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
>>>>> This series tries to access virtqueue metadata through kernel virtual
>>>>> address instead of copy_user() friends since they had too much
>>>>> overheads like checks, spec barriers or even hardware feature
>>>>> toggling. This is done through setup kernel address through vmap() and
>>>>> resigter MMU notifier for invalidation.
>>>>>
>>>>> Test shows about 24% improvement on TX PPS. TCP_STREAM doesn't see
>>>>> obvious improvement.
>>>> How is this going to work for CPUs with virtually tagged caches?
>>>
>>> Anything different that you worry?
>> If caches have virtual tags then kernel and userspace view of memory
>> might not be automatically in sync if they access memory
>> through different virtual addresses. You need to do things like
>> flush_cache_page, probably multiple times.
> "flush_dcache_page()"
I get this. Then I think the current set_bit_to_user() is suspicious, we
probably miss a flush_dcache_page() there:
static int set_bit_to_user(int nr, void __user *addr)
{
unsigned long log = (unsigned long)addr;
struct page *page;
void *base;
int bit = nr + (log % PAGE_SIZE) * 8;
int r;
r = get_user_pages_fast(log, 1, 1, &page);
if (r < 0)
return r;
BUG_ON(r != 1);
base = kmap_atomic(page);
set_bit(bit, base);
kunmap_atomic(base);
set_page_dirty_lock(page);
put_page(page);
return 0;
}
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Michael S. Tsirkin @ 2019-03-12 3:50 UTC (permalink / raw)
To: Jason Wang
Cc: Andrea Arcangeli, kvm, netdev, linux-kernel, virtualization,
linux-mm, Jerome Glisse
In-Reply-To: <ff45ea43-1145-5ea6-767c-1a99d55a9c61@redhat.com>
On Tue, Mar 12, 2019 at 10:52:15AM +0800, Jason Wang wrote:
>
> On 2019/3/11 下午8:48, Michael S. Tsirkin wrote:
> > On Mon, Mar 11, 2019 at 03:40:31PM +0800, Jason Wang wrote:
> > > On 2019/3/9 上午3:48, Andrea Arcangeli wrote:
> > > > Hello Jeson,
> > > >
> > > > On Fri, Mar 08, 2019 at 04:50:36PM +0800, Jason Wang wrote:
> > > > > Just to make sure I understand here. For boosting through huge TLB, do
> > > > > you mean we can do that in the future (e.g by mapping more userspace
> > > > > pages to kenrel) or it can be done by this series (only about three 4K
> > > > > pages were vmapped per virtqueue)?
> > > > When I answered about the advantages of mmu notifier and I mentioned
> > > > guaranteed 2m/gigapages where available, I overlooked the detail you
> > > > were using vmap instead of kmap. So with vmap you're actually doing
> > > > the opposite, it slows down the access because it will always use a 4k
> > > > TLB even if QEMU runs on THP or gigapages hugetlbfs.
> > > >
> > > > If there's just one page (or a few pages) in each vmap there's no need
> > > > of vmap, the linearity vmap provides doesn't pay off in such
> > > > case.
> > > >
> > > > So likely there's further room for improvement here that you can
> > > > achieve in the current series by just dropping vmap/vunmap.
> > > >
> > > > You can just use kmap (or kmap_atomic if you're in preemptible
> > > > section, should work from bh/irq).
> > > >
> > > > In short the mmu notifier to invalidate only sets a "struct page *
> > > > userringpage" pointer to NULL without calls to vunmap.
> > > >
> > > > In all cases immediately after gup_fast returns you can always call
> > > > put_page immediately (which explains why I'd like an option to drop
> > > > FOLL_GET from gup_fast to speed it up).
> > > >
> > > > Then you can check the sequence_counter and inc/dec counter increased
> > > > by _start/_end. That will tell you if the page you got and you called
> > > > put_page to immediately unpin it or even to free it, cannot go away
> > > > under you until the invalidate is called.
> > > >
> > > > If sequence counters and counter tells that gup_fast raced with anyt
> > > > mmu notifier invalidate you can just repeat gup_fast. Otherwise you're
> > > > done, the page cannot go away under you, the host virtual to host
> > > > physical mapping cannot change either. And the page is not pinned
> > > > either. So you can just set the "struct page * userringpage = page"
> > > > where "page" was the one setup by gup_fast.
> > > >
> > > > When later the invalidate runs, you can just call set_page_dirty if
> > > > gup_fast was called with "write = 1" and then you clear the pointer
> > > > "userringpage = NULL".
> > > >
> > > > When you need to read/write to the memory
> > > > kmap/kmap_atomic(userringpage) should work.
> > > Yes, I've considered kmap() from the start. The reason I don't do that is
> > > large virtqueue may need more than one page so VA might not be contiguous.
> > > But this is probably not a big issue which just need more tricks in the
> > > vhost memory accessors.
> > >
> > >
> > > > In short because there's no hardware involvement here, the established
> > > > mapping is just the pointer to the page, there is no need of setting
> > > > up any pagetables or to do any TLB flushes (except on 32bit archs if
> > > > the page is above the direct mapping but it never happens on 64bit
> > > > archs).
> > > I see, I believe we don't care much about the performance of 32bit archs (or
> > > we can just fallback to copy_to_user() friends).
> > Using copyXuser is better I guess.
>
>
> Ok.
>
>
> >
> > > Using direct mapping (I
> > > guess kernel will always try hugepage for that?) should be better and we can
> > > even use it for the data transfer not only for the metadata.
> > >
> > > Thanks
> > We can't really. The big issue is get user pages. Doing that on data
> > path will be slower than copyXuser.
>
>
> I meant if we can find a way to avoid doing gup in datapath. E.g vhost
> maintain a range tree and add or remove ranges through MMU notifier. Then in
> datapath, if we find the range, then use direct mapping otherwise
> copy_to_user().
>
> Thanks
We can try. But I'm not sure there's any reason to think there's any
locality there.
>
> > Or maybe it won't with the
> > amount of mitigations spread around. Go ahead and try.
> >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Michael S. Tsirkin @ 2019-03-12 3:51 UTC (permalink / raw)
To: Jason Wang
Cc: Andrea Arcangeli, kvm, netdev, linux-kernel, virtualization,
linux-mm, Jerome Glisse
In-Reply-To: <4979eed5-9e3f-5ee0-f4f4-1a5e2a839b21@redhat.com>
On Tue, Mar 12, 2019 at 10:56:20AM +0800, Jason Wang wrote:
>
> On 2019/3/11 下午9:43, Andrea Arcangeli wrote:
> > On Mon, Mar 11, 2019 at 08:48:37AM -0400, Michael S. Tsirkin wrote:
> > > Using copyXuser is better I guess.
> > It certainly would be faster there, but I don't think it's needed if
> > that would be the only use case left that justifies supporting two
> > different models. On small 32bit systems with little RAM kmap won't
> > perform measurably different on 32bit or 64bit systems. If the 32bit
> > host has a lot of ram it all gets slow anyway at accessing RAM above
> > the direct mapping, if compared to 64bit host kernels, it's not just
> > an issue for vhost + mmu notifier + kmap and the best way to optimize
> > things is to run 64bit host kernels.
> >
> > Like Christoph pointed out, the main use case for retaining the
> > copy-user model would be CPUs with virtually indexed not physically
> > tagged data caches (they'll still suffer from the spectre-v1 fix,
> > although I exclude they have to suffer the SMAP
> > slowdown/feature). Those may require some additional flushing than the
> > current copy-user model requires.
> >
> > As a rule of thumb any arch where copy_user_page doesn't define as
> > copy_page will require some additional cache flushing after the
> > kmap. Supposedly with vmap, the vmap layer should have taken care of
> > that (I didn't verify that yet).
>
>
> vmap_page_range()/free_unmap_vmap_area() will call
> fluch_cache_vmap()/flush_cache_vunmap(). So vmap layer should be ok.
>
> Thanks
You only unmap from mmu notifier though.
You don't do it after any access.
>
> >
> > There are some accessories like copy_to_user_page()
> > copy_from_user_page() that could work and obviously defines to raw
> > memcpy on x86 (the main cons is they don't provide word granular
> > access) and at least on sparc they're tailored to ptrace assumptions
> > so then we'd need to evaluate what happens if this is used outside of
> > ptrace context. kmap has been used generally either to access whole
> > pages (i.e. copy_user_page), so ptrace may actually be the only use
> > case with subpage granularity access.
> >
> > #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
> > do { \
> > flush_cache_page(vma, vaddr, page_to_pfn(page)); \
> > memcpy(dst, src, len); \
> > flush_ptrace_access(vma, page, vaddr, src, len, 0); \
> > } while (0)
> >
> > So I wouldn't rule out the need for a dual model, until we solve how
> > to run this stable on non-x86 arches with not physically tagged
> > caches.
> >
> > Thanks,
> > Andrea
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Michael S. Tsirkin @ 2019-03-12 3:52 UTC (permalink / raw)
To: Jason Wang
Cc: aarcange, linux-parisc, kvm, netdev, linux-kernel, virtualization,
hch, linux-mm, David Miller, linux-arm-kernel
In-Reply-To: <6b6dcc4a-2f08-ba67-0423-35787f3b966c@redhat.com>
On Tue, Mar 12, 2019 at 10:59:09AM +0800, Jason Wang wrote:
>
> On 2019/3/12 上午2:14, David Miller wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > Date: Mon, 11 Mar 2019 09:59:28 -0400
> >
> > > On Mon, Mar 11, 2019 at 03:13:17PM +0800, Jason Wang wrote:
> > > > On 2019/3/8 下午10:12, Christoph Hellwig wrote:
> > > > > On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
> > > > > > This series tries to access virtqueue metadata through kernel virtual
> > > > > > address instead of copy_user() friends since they had too much
> > > > > > overheads like checks, spec barriers or even hardware feature
> > > > > > toggling. This is done through setup kernel address through vmap() and
> > > > > > resigter MMU notifier for invalidation.
> > > > > >
> > > > > > Test shows about 24% improvement on TX PPS. TCP_STREAM doesn't see
> > > > > > obvious improvement.
> > > > > How is this going to work for CPUs with virtually tagged caches?
> > > >
> > > > Anything different that you worry?
> > > If caches have virtual tags then kernel and userspace view of memory
> > > might not be automatically in sync if they access memory
> > > through different virtual addresses. You need to do things like
> > > flush_cache_page, probably multiple times.
> > "flush_dcache_page()"
>
>
> I get this. Then I think the current set_bit_to_user() is suspicious, we
> probably miss a flush_dcache_page() there:
>
>
> static int set_bit_to_user(int nr, void __user *addr)
> {
> unsigned long log = (unsigned long)addr;
> struct page *page;
> void *base;
> int bit = nr + (log % PAGE_SIZE) * 8;
> int r;
>
> r = get_user_pages_fast(log, 1, 1, &page);
> if (r < 0)
> return r;
> BUG_ON(r != 1);
> base = kmap_atomic(page);
> set_bit(bit, base);
> kunmap_atomic(base);
> set_page_dirty_lock(page);
> put_page(page);
> return 0;
> }
>
> Thanks
I think you are right. The correct fix though is to re-implement
it using asm and handling pagefault, not gup.
Three atomic ops per bit is way to expensive.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-03-12 7:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Andrea Arcangeli, kvm, netdev, linux-kernel, virtualization,
linux-mm, Jerome Glisse
In-Reply-To: <20190311234956-mutt-send-email-mst@kernel.org>
On 2019/3/12 上午11:50, Michael S. Tsirkin wrote:
>>>> Using direct mapping (I
>>>> guess kernel will always try hugepage for that?) should be better and we can
>>>> even use it for the data transfer not only for the metadata.
>>>>
>>>> Thanks
>>> We can't really. The big issue is get user pages. Doing that on data
>>> path will be slower than copyXuser.
>> I meant if we can find a way to avoid doing gup in datapath. E.g vhost
>> maintain a range tree and add or remove ranges through MMU notifier. Then in
>> datapath, if we find the range, then use direct mapping otherwise
>> copy_to_user().
>>
>> Thanks
> We can try. But I'm not sure there's any reason to think there's any
> locality there.
>
Ok, but what kind of locality do you mean here?
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-03-12 7:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, linux-parisc, kvm, netdev, linux-kernel, virtualization,
hch, linux-mm, David Miller, linux-arm-kernel
In-Reply-To: <20190311235140-mutt-send-email-mst@kernel.org>
On 2019/3/12 上午11:52, Michael S. Tsirkin wrote:
> On Tue, Mar 12, 2019 at 10:59:09AM +0800, Jason Wang wrote:
>> On 2019/3/12 上午2:14, David Miller wrote:
>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>> Date: Mon, 11 Mar 2019 09:59:28 -0400
>>>
>>>> On Mon, Mar 11, 2019 at 03:13:17PM +0800, Jason Wang wrote:
>>>>> On 2019/3/8 下午10:12, Christoph Hellwig wrote:
>>>>>> On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
>>>>>>> This series tries to access virtqueue metadata through kernel virtual
>>>>>>> address instead of copy_user() friends since they had too much
>>>>>>> overheads like checks, spec barriers or even hardware feature
>>>>>>> toggling. This is done through setup kernel address through vmap() and
>>>>>>> resigter MMU notifier for invalidation.
>>>>>>>
>>>>>>> Test shows about 24% improvement on TX PPS. TCP_STREAM doesn't see
>>>>>>> obvious improvement.
>>>>>> How is this going to work for CPUs with virtually tagged caches?
>>>>> Anything different that you worry?
>>>> If caches have virtual tags then kernel and userspace view of memory
>>>> might not be automatically in sync if they access memory
>>>> through different virtual addresses. You need to do things like
>>>> flush_cache_page, probably multiple times.
>>> "flush_dcache_page()"
>>
>> I get this. Then I think the current set_bit_to_user() is suspicious, we
>> probably miss a flush_dcache_page() there:
>>
>>
>> static int set_bit_to_user(int nr, void __user *addr)
>> {
>> unsigned long log = (unsigned long)addr;
>> struct page *page;
>> void *base;
>> int bit = nr + (log % PAGE_SIZE) * 8;
>> int r;
>>
>> r = get_user_pages_fast(log, 1, 1, &page);
>> if (r < 0)
>> return r;
>> BUG_ON(r != 1);
>> base = kmap_atomic(page);
>> set_bit(bit, base);
>> kunmap_atomic(base);
>> set_page_dirty_lock(page);
>> put_page(page);
>> return 0;
>> }
>>
>> Thanks
> I think you are right. The correct fix though is to re-implement
> it using asm and handling pagefault, not gup.
I agree but it needs to introduce new helpers in asm for all archs
which is not trivial. At least for -stable, we need the flush?
> Three atomic ops per bit is way to expensive.
Yes.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-03-12 7:51 UTC (permalink / raw)
To: James Bottomley, David Miller, mst
Cc: aarcange, linux-parisc, kvm, netdev, linux-kernel, virtualization,
hch, linux-mm, linux-arm-kernel
In-Reply-To: <1552367685.23859.22.camel@HansenPartnership.com>
On 2019/3/12 下午1:14, James Bottomley wrote:
> On Tue, 2019-03-12 at 10:59 +0800, Jason Wang wrote:
>> On 2019/3/12 上午2:14, David Miller wrote:
>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>> Date: Mon, 11 Mar 2019 09:59:28 -0400
>>>
>>>> On Mon, Mar 11, 2019 at 03:13:17PM +0800, Jason Wang wrote:
>>>>> On 2019/3/8 下午10:12, Christoph Hellwig wrote:
>>>>>> On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
>>>>>>> This series tries to access virtqueue metadata through
>>>>>>> kernel virtual
>>>>>>> address instead of copy_user() friends since they had too
>>>>>>> much
>>>>>>> overheads like checks, spec barriers or even hardware
>>>>>>> feature
>>>>>>> toggling. This is done through setup kernel address through
>>>>>>> vmap() and
>>>>>>> resigter MMU notifier for invalidation.
>>>>>>>
>>>>>>> Test shows about 24% improvement on TX PPS. TCP_STREAM
>>>>>>> doesn't see
>>>>>>> obvious improvement.
>>>>>> How is this going to work for CPUs with virtually tagged
>>>>>> caches?
>>>>> Anything different that you worry?
>>>> If caches have virtual tags then kernel and userspace view of
>>>> memory
>>>> might not be automatically in sync if they access memory
>>>> through different virtual addresses. You need to do things like
>>>> flush_cache_page, probably multiple times.
>>> "flush_dcache_page()"
>>
>> I get this. Then I think the current set_bit_to_user() is suspicious,
>> we
>> probably miss a flush_dcache_page() there:
>>
>>
>> static int set_bit_to_user(int nr, void __user *addr)
>> {
>> unsigned long log = (unsigned long)addr;
>> struct page *page;
>> void *base;
>> int bit = nr + (log % PAGE_SIZE) * 8;
>> int r;
>>
>> r = get_user_pages_fast(log, 1, 1, &page);
>> if (r < 0)
>> return r;
>> BUG_ON(r != 1);
>> base = kmap_atomic(page);
>> set_bit(bit, base);
>> kunmap_atomic(base);
> This sequence should be OK. get_user_pages() contains a flush which
> clears the cache above the user virtual address, so on kmap, the page
> is coherent at the new alias. On parisc at least, kunmap embodies a
> flush_dcache_page() which pushes any changes in the cache above the
> kernel virtual address back to main memory and makes it coherent again
> for the user alias to pick it up.
It would be good if kmap()/kunmap() can do this but looks like we can
not assume this? For example, sparc's flush_dcache_page() doesn't do
flush_dcache_page(). And bio_copy_data_iter() do flush_dcache_page()
after kunmap_atomic().
Thanks
>
> James
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-03-12 7:53 UTC (permalink / raw)
To: James Bottomley, David Miller, mst
Cc: aarcange, linux-parisc, kvm, netdev, linux-kernel, virtualization,
hch, linux-mm, linux-arm-kernel
In-Reply-To: <f9e52313-0a06-22b6-140c-ded75eecde20@redhat.com>
On 2019/3/12 下午3:51, Jason Wang wrote:
>
> On 2019/3/12 下午1:14, James Bottomley wrote:
>> On Tue, 2019-03-12 at 10:59 +0800, Jason Wang wrote:
>>> On 2019/3/12 上午2:14, David Miller wrote:
>>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Date: Mon, 11 Mar 2019 09:59:28 -0400
>>>>
>>>>> On Mon, Mar 11, 2019 at 03:13:17PM +0800, Jason Wang wrote:
>>>>>> On 2019/3/8 下午10:12, Christoph Hellwig wrote:
>>>>>>> On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
>>>>>>>> This series tries to access virtqueue metadata through
>>>>>>>> kernel virtual
>>>>>>>> address instead of copy_user() friends since they had too
>>>>>>>> much
>>>>>>>> overheads like checks, spec barriers or even hardware
>>>>>>>> feature
>>>>>>>> toggling. This is done through setup kernel address through
>>>>>>>> vmap() and
>>>>>>>> resigter MMU notifier for invalidation.
>>>>>>>>
>>>>>>>> Test shows about 24% improvement on TX PPS. TCP_STREAM
>>>>>>>> doesn't see
>>>>>>>> obvious improvement.
>>>>>>> How is this going to work for CPUs with virtually tagged
>>>>>>> caches?
>>>>>> Anything different that you worry?
>>>>> If caches have virtual tags then kernel and userspace view of
>>>>> memory
>>>>> might not be automatically in sync if they access memory
>>>>> through different virtual addresses. You need to do things like
>>>>> flush_cache_page, probably multiple times.
>>>> "flush_dcache_page()"
>>>
>>> I get this. Then I think the current set_bit_to_user() is suspicious,
>>> we
>>> probably miss a flush_dcache_page() there:
>>>
>>>
>>> static int set_bit_to_user(int nr, void __user *addr)
>>> {
>>> unsigned long log = (unsigned long)addr;
>>> struct page *page;
>>> void *base;
>>> int bit = nr + (log % PAGE_SIZE) * 8;
>>> int r;
>>>
>>> r = get_user_pages_fast(log, 1, 1, &page);
>>> if (r < 0)
>>> return r;
>>> BUG_ON(r != 1);
>>> base = kmap_atomic(page);
>>> set_bit(bit, base);
>>> kunmap_atomic(base);
>> This sequence should be OK. get_user_pages() contains a flush which
>> clears the cache above the user virtual address, so on kmap, the page
>> is coherent at the new alias. On parisc at least, kunmap embodies a
>> flush_dcache_page() which pushes any changes in the cache above the
>> kernel virtual address back to main memory and makes it coherent again
>> for the user alias to pick it up.
>
>
> It would be good if kmap()/kunmap() can do this but looks like we can
> not assume this? For example, sparc's flush_dcache_page()
Sorry, I meant kunmap_atomic().
Thanks
> doesn't do flush_dcache_page(). And bio_copy_data_iter() do
> flush_dcache_page() after kunmap_atomic().
>
> Thanks
>
>
>>
>> James
>>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio_ring: Fix potential mem leak in virtqueue_add_indirect_packed
From: Jason Wang @ 2019-03-12 8:01 UTC (permalink / raw)
To: Yue Haibing, mst; +Cc: linux-kernel, virtualization
In-Reply-To: <20190312070653.16392-1-yuehaibing@huawei.com>
On 2019/3/12 下午3:06, Yue Haibing wrote:
> From: YueHaibing <yuehaibing@huawei.com>
>
> 'desc' should be freed before leaving from err handing path.
>
> Fixes: 1ce9e6055fa0 ("virtio_ring: introduce packed ring support")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/virtio/virtio_ring.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index a0b07c3..9d95d9c 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -991,6 +991,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>
> if (unlikely(vq->vq.num_free < 1)) {
> pr_debug("Can't add buf len 1 - avail = 0\n");
> + kfree(desc);
> END_USE(vq);
> return -ENOSPC;
> }
Or you can move the check before the allocation.
Acked-by: Jason Wang <jasowang@redhat.com>
Please cc stable next time.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Michael S. Tsirkin @ 2019-03-12 11:54 UTC (permalink / raw)
To: Jason Wang
Cc: aarcange, linux-parisc, kvm, netdev, linux-kernel, virtualization,
hch, linux-mm, David Miller, linux-arm-kernel
In-Reply-To: <76c353ed-d6de-99a9-76f9-f258074c1462@redhat.com>
On Tue, Mar 12, 2019 at 03:17:00PM +0800, Jason Wang wrote:
>
> On 2019/3/12 上午11:52, Michael S. Tsirkin wrote:
> > On Tue, Mar 12, 2019 at 10:59:09AM +0800, Jason Wang wrote:
> > > On 2019/3/12 上午2:14, David Miller wrote:
> > > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > > Date: Mon, 11 Mar 2019 09:59:28 -0400
> > > >
> > > > > On Mon, Mar 11, 2019 at 03:13:17PM +0800, Jason Wang wrote:
> > > > > > On 2019/3/8 下午10:12, Christoph Hellwig wrote:
> > > > > > > On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
> > > > > > > > This series tries to access virtqueue metadata through kernel virtual
> > > > > > > > address instead of copy_user() friends since they had too much
> > > > > > > > overheads like checks, spec barriers or even hardware feature
> > > > > > > > toggling. This is done through setup kernel address through vmap() and
> > > > > > > > resigter MMU notifier for invalidation.
> > > > > > > >
> > > > > > > > Test shows about 24% improvement on TX PPS. TCP_STREAM doesn't see
> > > > > > > > obvious improvement.
> > > > > > > How is this going to work for CPUs with virtually tagged caches?
> > > > > > Anything different that you worry?
> > > > > If caches have virtual tags then kernel and userspace view of memory
> > > > > might not be automatically in sync if they access memory
> > > > > through different virtual addresses. You need to do things like
> > > > > flush_cache_page, probably multiple times.
> > > > "flush_dcache_page()"
> > >
> > > I get this. Then I think the current set_bit_to_user() is suspicious, we
> > > probably miss a flush_dcache_page() there:
> > >
> > >
> > > static int set_bit_to_user(int nr, void __user *addr)
> > > {
> > > unsigned long log = (unsigned long)addr;
> > > struct page *page;
> > > void *base;
> > > int bit = nr + (log % PAGE_SIZE) * 8;
> > > int r;
> > >
> > > r = get_user_pages_fast(log, 1, 1, &page);
> > > if (r < 0)
> > > return r;
> > > BUG_ON(r != 1);
> > > base = kmap_atomic(page);
> > > set_bit(bit, base);
> > > kunmap_atomic(base);
> > > set_page_dirty_lock(page);
> > > put_page(page);
> > > return 0;
> > > }
> > >
> > > Thanks
> > I think you are right. The correct fix though is to re-implement
> > it using asm and handling pagefault, not gup.
>
>
> I agree but it needs to introduce new helpers in asm for all archs which is
> not trivial.
We can have a generic implementation using kmap.
> At least for -stable, we need the flush?
>
>
> > Three atomic ops per bit is way to expensive.
>
>
> Yes.
>
> Thanks
See James's reply - I stand corrected we do kunmap so no need to flush.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Dongli Zhang @ 2019-03-12 17:22 UTC (permalink / raw)
To: virtualization, linux-block; +Cc: axboe, linux-kernel, mst
I observed that there is one msix vector for config and one shared vector
for all queues in below qemu cmdline, when the num-queues for virtio-blk
is more than the number of possible cpus:
qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=6"
# cat /proc/interrupts
CPU0 CPU1 CPU2 CPU3
... ...
24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
25: 0 0 0 59 PCI-MSI 65537-edge virtio0-virtqueues
... ...
However, when num-queues is the same as number of possible cpus:
qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=4"
# cat /proc/interrupts
CPU0 CPU1 CPU2 CPU3
... ...
24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
25: 2 0 0 0 PCI-MSI 65537-edge virtio0-req.0
26: 0 35 0 0 PCI-MSI 65538-edge virtio0-req.1
27: 0 0 32 0 PCI-MSI 65539-edge virtio0-req.2
28: 0 0 0 0 PCI-MSI 65540-edge virtio0-req.3
... ...
In above case, there is one msix vector per queue.
This is because the max number of queues is not limited by the number of
possible cpus.
By default, nvme (regardless about write_queues and poll_queues) and
xen-blkfront limit the number of queues with num_possible_cpus().
Is this by design on purpose, or can we fix with below?
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4bc083b..df95ce3 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
if (err)
num_vqs = 1;
+ num_vqs = min(num_possible_cpus(), num_vqs);
+
vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
if (!vblk->vqs)
return -ENOMEM;
--
PS: The same issue is applicable to virtio-scsi as well.
Thank you very much!
Dongli Zhang
^ permalink raw reply related
* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Cornelia Huck @ 2019-03-12 17:33 UTC (permalink / raw)
To: Dongli Zhang; +Cc: linux-block, axboe, mst, linux-kernel, virtualization
In-Reply-To: <e4afe4c5-0262-4500-aeec-60f30734b4fc@default>
On Tue, 12 Mar 2019 10:22:46 -0700 (PDT)
Dongli Zhang <dongli.zhang@oracle.com> wrote:
> I observed that there is one msix vector for config and one shared vector
> for all queues in below qemu cmdline, when the num-queues for virtio-blk
> is more than the number of possible cpus:
>
> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=6"
>
> # cat /proc/interrupts
> CPU0 CPU1 CPU2 CPU3
> ... ...
> 24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
> 25: 0 0 0 59 PCI-MSI 65537-edge virtio0-virtqueues
> ... ...
>
>
> However, when num-queues is the same as number of possible cpus:
>
> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=4"
>
> # cat /proc/interrupts
> CPU0 CPU1 CPU2 CPU3
> ... ...
> 24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
> 25: 2 0 0 0 PCI-MSI 65537-edge virtio0-req.0
> 26: 0 35 0 0 PCI-MSI 65538-edge virtio0-req.1
> 27: 0 0 32 0 PCI-MSI 65539-edge virtio0-req.2
> 28: 0 0 0 0 PCI-MSI 65540-edge virtio0-req.3
> ... ...
>
> In above case, there is one msix vector per queue.
Please note that this is pci-specific...
>
>
> This is because the max number of queues is not limited by the number of
> possible cpus.
>
> By default, nvme (regardless about write_queues and poll_queues) and
> xen-blkfront limit the number of queues with num_possible_cpus().
...and these are probably pci-specific as well.
>
>
> Is this by design on purpose, or can we fix with below?
>
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4bc083b..df95ce3 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
> if (err)
> num_vqs = 1;
>
> + num_vqs = min(num_possible_cpus(), num_vqs);
> +
> vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
> if (!vblk->vqs)
> return -ENOMEM;
virtio-blk, however, is not pci-specific.
If we are using the ccw transport on s390, a completely different
interrupt mechanism is in use ('floating' interrupts, which are not
per-cpu). A check like that should therefore not go into the generic
driver.
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Andrea Arcangeli @ 2019-03-12 20:04 UTC (permalink / raw)
To: James Bottomley
Cc: linux-parisc, kvm, Michael S. Tsirkin, netdev, linux-kernel,
virtualization, hch, linux-mm, David Miller, linux-arm-kernel
In-Reply-To: <1552405610.3083.17.camel@HansenPartnership.com>
On Tue, Mar 12, 2019 at 08:46:50AM -0700, James Bottomley wrote:
> On Tue, 2019-03-12 at 07:54 -0400, Michael S. Tsirkin wrote:
> > On Tue, Mar 12, 2019 at 03:17:00PM +0800, Jason Wang wrote:
> > >
> > > On 2019/3/12 上åˆ11:52, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 12, 2019 at 10:59:09AM +0800, Jason Wang wrote:
> [...]
> > > At least for -stable, we need the flush?
> > >
> > >
> > > > Three atomic ops per bit is way to expensive.
> > >
> > >
> > > Yes.
> > >
> > > Thanks
> >
> > See James's reply - I stand corrected we do kunmap so no need to
> > flush.
>
> Well, I said that's what we do on Parisc. The cachetlb document
> definitely says if you alter the data between kmap and kunmap you are
> responsible for the flush. It's just that flush_dcache_page() is a no-
> op on x86 so they never remember to add it and since it will crash
> parisc if you get it wrong we finally gave up trying to make them.
>
> But that's the point: it is a no-op on your favourite architecture so
> it costs you nothing to add it.
Yes, the fact Parisc gave up and is doing it on kunmap is reasonable
approach for Parisc, but it doesn't move the needle as far as vhost
common code is concerned, because other archs don't flush any cache on
kunmap.
So either all other archs give up trying to optimize, or vhost still
has to call flush_dcache_page() after kunmap.
Which means after we fix vhost to add the flush_dcache_page after
kunmap, Parisc will get a double hit (but it also means Parisc was the
only one of those archs needed explicit cache flushes, where vhost
worked correctly so far.. so it kinds of proofs your point of giving
up being the safe choice).
Thanks,
Andrea
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Andrea Arcangeli @ 2019-03-12 21:11 UTC (permalink / raw)
To: James Bottomley
Cc: linux-parisc, kvm, Michael S. Tsirkin, netdev, linux-kernel,
virtualization, hch, linux-mm, David Miller, linux-arm-kernel
In-Reply-To: <1552424017.14432.11.camel@HansenPartnership.com>
On Tue, Mar 12, 2019 at 01:53:37PM -0700, James Bottomley wrote:
> I've got to say: optimize what? What code do we ever have in the
> kernel that kmap's a page and then doesn't do anything with it? You can
> guarantee that on kunmap the page is either referenced (needs
> invalidating) or updated (needs flushing). The in-kernel use of kmap is
> always
>
> kmap
> do something with the mapped page
> kunmap
>
> In a very short interval. It seems just a simplification to make
> kunmap do the flush if needed rather than try to have the users
> remember. The thing which makes this really simple is that on most
> architectures flush and invalidate is the same operation. If you
> really want to optimize you can use the referenced and dirty bits on
> the kmapped pte to tell you what operation to do, but if your flush is
> your invalidate, you simply assume the data needs flushing on kunmap
> without checking anything.
Except other archs like arm64 and sparc do the cache flushing on
copy_to_user_page and copy_user_page, not on kunmap.
#define copy_user_page(to,from,vaddr,pg) __cpu_copy_user_page(to, from, vaddr)
void __cpu_copy_user_page(void *kto, const void *kfrom, unsigned long vaddr)
{
struct page *page = virt_to_page(kto);
copy_page(kto, kfrom);
flush_dcache_page(page);
}
#define copy_user_page(to, from, vaddr, page) \
do { copy_page(to, from); \
sparc_flush_page_to_ram(page); \
} while (0)
And they do nothing on kunmap:
static inline void kunmap(struct page *page)
{
BUG_ON(in_interrupt());
if (!PageHighMem(page))
return;
kunmap_high(page);
}
void kunmap_high(struct page *page)
{
unsigned long vaddr;
unsigned long nr;
unsigned long flags;
int need_wakeup;
unsigned int color = get_pkmap_color(page);
wait_queue_head_t *pkmap_map_wait;
lock_kmap_any(flags);
vaddr = (unsigned long)page_address(page);
BUG_ON(!vaddr);
nr = PKMAP_NR(vaddr);
/*
* A count must never go down to zero
* without a TLB flush!
*/
need_wakeup = 0;
switch (--pkmap_count[nr]) {
case 0:
BUG();
case 1:
/*
* Avoid an unnecessary wake_up() function call.
* The common case is pkmap_count[] == 1, but
* no waiters.
* The tasks queued in the wait-queue are guarded
* by both the lock in the wait-queue-head and by
* the kmap_lock. As the kmap_lock is held here,
* no need for the wait-queue-head's lock. Simply
* test if the queue is empty.
*/
pkmap_map_wait = get_pkmap_wait_queue_head(color);
need_wakeup = waitqueue_active(pkmap_map_wait);
}
unlock_kmap_any(flags);
/* do wake-up, if needed, race-free outside of the spin lock */
if (need_wakeup)
wake_up(pkmap_map_wait);
}
static inline void kunmap(struct page *page)
{
}
because they already did it just above.
> > Which means after we fix vhost to add the flush_dcache_page after
> > kunmap, Parisc will get a double hit (but it also means Parisc was
> > the only one of those archs needed explicit cache flushes, where
> > vhost worked correctly so far.. so it kinds of proofs your point of
> > giving up being the safe choice).
>
> What double hit? If there's no cache to flush then cache flush is a
> no-op. It's also a highly piplineable no-op because the CPU has the L1
> cache within easy reach. The only event when flush takes a large
> amount time is if we actually have dirty data to write back to main
> memory.
The double hit is in parisc copy_to_user_page:
#define copy_to_user_page(vma, page, vaddr, dst, src, len) \
do { \
flush_cache_page(vma, vaddr, page_to_pfn(page)); \
memcpy(dst, src, len); \
flush_kernel_dcache_range_asm((unsigned long)dst, (unsigned long)dst + len); \
} while (0)
That is executed just before kunmap:
static inline void kunmap(struct page *page)
{
flush_kernel_dcache_page_addr(page_address(page));
}
Can't argue about the fact your "safer" kunmap is safer, but we cannot
rely on common code unless we remove some optimization from the common
code abstractions and we make all archs do kunmap like parisc.
Thanks,
Andrea
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Andrea Arcangeli @ 2019-03-12 21:53 UTC (permalink / raw)
To: James Bottomley
Cc: linux-parisc, kvm, Michael S. Tsirkin, netdev, linux-kernel,
virtualization, hch, linux-mm, David Miller, linux-arm-kernel
In-Reply-To: <1552425555.14432.14.camel@HansenPartnership.com>
On Tue, Mar 12, 2019 at 02:19:15PM -0700, James Bottomley wrote:
> I mean in the sequence
>
> flush_dcache_page(page);
> flush_dcache_page(page);
>
> The first flush_dcache_page did all the work and the second it a
> tightly pipelined no-op. That's what I mean by there not really being
> a double hit.
Ok I wasn't sure it was clear there was a double (profiling) hit on
that function.
void flush_kernel_dcache_page_addr(void *addr)
{
unsigned long flags;
flush_kernel_dcache_page_asm(addr);
purge_tlb_start(flags);
pdtlb_kernel(addr);
purge_tlb_end(flags);
}
#define purge_tlb_start(flags) spin_lock_irqsave(&pa_tlb_lock, flags)
#define purge_tlb_end(flags) spin_unlock_irqrestore(&pa_tlb_lock, flags)
You got a system-wide spinlock in there that won't just go away the
second time. So it's a bit more than a tightly pipelined "noop".
Your logic of adding the flush on kunmap makes sense, all I'm saying
is that it's sacrificing some performance for safety. You asked
"optimized what", I meant to optimize away all the above quoted code
that will end running twice for each vhost set_bit when it should run
just once like in other archs. And it clearly paid off until now
(until now it run just once and it was the only safe one).
Before we can leverage your idea to flush the dcache on kunmap in
common code without having to sacrifice performance in arch code, we'd
need to change all other archs to add the cache flushes on kunmap too,
and then remove the cache flushes from the other places like copy_page
or we'd waste CPU. Then you'd have the best of both words, no double
flush and kunmap would be enough.
Thanks,
Andrea
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Andrea Arcangeli @ 2019-03-12 22:50 UTC (permalink / raw)
To: James Bottomley
Cc: linux-parisc, kvm, Michael S. Tsirkin, netdev, linux-kernel,
virtualization, hch, linux-mm, David Miller, linux-arm-kernel
In-Reply-To: <1552428174.14432.39.camel@HansenPartnership.com>
On Tue, Mar 12, 2019 at 03:02:54PM -0700, James Bottomley wrote:
> I'm sure there must be workarounds elsewhere in the other arch code
> otherwise things like this, which appear all over drivers/, wouldn't
> work:
>
> drivers/scsi/isci/request.c:1430
>
> kaddr = kmap_atomic(page);
> memcpy(kaddr + sg->offset, src_addr, copy_len);
> kunmap_atomic(kaddr);
>
Are you sure "page" is an userland page with an alias address?
sg->page_link = (unsigned long)virt_to_page(addr);
page_link seems to point to kernel memory.
I found an apparent solution like parisc on arm 32bit:
void __kunmap_atomic(void *kvaddr)
{
unsigned long vaddr = (unsigned long) kvaddr & PAGE_MASK;
int idx, type;
if (kvaddr >= (void *)FIXADDR_START) {
type = kmap_atomic_idx();
idx = FIX_KMAP_BEGIN + type + KM_TYPE_NR * smp_processor_id();
if (cache_is_vivt())
__cpuc_flush_dcache_area((void *)vaddr, PAGE_SIZE);
However on arm 64bit kunmap_atomic is not implemented at all and other
32bit implementations don't do it, for example sparc seems to do the
cache flush too if the kernel is built with CONFIG_DEBUG_HIGHMEM
(which makes the flushing conditional to the debug option).
The kunmap_atomic where fixmap is used, is flushing the tlb lazily so
even on 32bit you can't even be sure if there was a tlb flush for each
single page you unmapped, so it's hard to see how the above can work
safe, is "page" would have been an userland page mapped with aliased
CPU cache.
> the sequence dirties the kernel virtual address but doesn't flush
> before doing kunmap. There are hundreds of other examples which is why
> I think adding flush_kernel_dcache_page() is an already lost cause.
In lots of cases kmap is needed to just modify kernel memory not to
modify userland memory (where get/put_user is more commonly used
instead..), there's no cache aliasing in such case.
> Actually copy_user_page() is unused in the main kernel. The big
> problem is copy_user_highpage() but that's mostly highly optimised by
> the VIPT architectures (in other words you can fiddle with kmap without
> impacting it).
copy_user_page is not unused, it's called precisely by
copy_user_highpage, which is why the cache flushes are done inside
copy_user_page.
static inline void copy_user_highpage(struct page *to, struct page *from,
unsigned long vaddr, struct vm_area_struct *vma)
{
char *vfrom, *vto;
vfrom = kmap_atomic(from);
vto = kmap_atomic(to);
copy_user_page(vto, vfrom, vaddr, to);
kunmap_atomic(vto);
kunmap_atomic(vfrom);
}
^ permalink raw reply
* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Dongli Zhang @ 2019-03-13 3:26 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-block, axboe, mst, linux-kernel, virtualization
In-Reply-To: <20190312183351.74764f4f.cohuck@redhat.com>
On 3/13/19 1:33 AM, Cornelia Huck wrote:
> On Tue, 12 Mar 2019 10:22:46 -0700 (PDT)
> Dongli Zhang <dongli.zhang@oracle.com> wrote:
>
>> I observed that there is one msix vector for config and one shared vector
>> for all queues in below qemu cmdline, when the num-queues for virtio-blk
>> is more than the number of possible cpus:
>>
>> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=6"
>>
>> # cat /proc/interrupts
>> CPU0 CPU1 CPU2 CPU3
>> ... ...
>> 24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
>> 25: 0 0 0 59 PCI-MSI 65537-edge virtio0-virtqueues
>> ... ...
>>
>>
>> However, when num-queues is the same as number of possible cpus:
>>
>> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=4"
>>
>> # cat /proc/interrupts
>> CPU0 CPU1 CPU2 CPU3
>> ... ...
>> 24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
>> 25: 2 0 0 0 PCI-MSI 65537-edge virtio0-req.0
>> 26: 0 35 0 0 PCI-MSI 65538-edge virtio0-req.1
>> 27: 0 0 32 0 PCI-MSI 65539-edge virtio0-req.2
>> 28: 0 0 0 0 PCI-MSI 65540-edge virtio0-req.3
>> ... ...
>>
>> In above case, there is one msix vector per queue.
>
> Please note that this is pci-specific...
>
>>
>>
>> This is because the max number of queues is not limited by the number of
>> possible cpus.
>>
>> By default, nvme (regardless about write_queues and poll_queues) and
>> xen-blkfront limit the number of queues with num_possible_cpus().
>
> ...and these are probably pci-specific as well.
Not pci-specific, but per-cpu as well.
>
>>
>>
>> Is this by design on purpose, or can we fix with below?
>>
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 4bc083b..df95ce3 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
>> if (err)
>> num_vqs = 1;
>>
>> + num_vqs = min(num_possible_cpus(), num_vqs);
>> +
>> vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
>> if (!vblk->vqs)
>> return -ENOMEM;
>
> virtio-blk, however, is not pci-specific.
>
> If we are using the ccw transport on s390, a completely different
> interrupt mechanism is in use ('floating' interrupts, which are not
> per-cpu). A check like that should therefore not go into the generic
> driver.
>
So far there seems two options.
The 1st option is to ask the qemu user to always specify "-num-queues" with the
same number of vcpus when running x86 guest with pci for virtio-blk or
virtio-scsi, in order to assign a vector for each queue.
Or, is it fine for virtio folks to add a new hook to 'struct virtio_config_ops'
so that different platforms (e.g., pci or ccw) would use different ways to limit
the max number of queues in guest, with something like below?
So far both virtio-scsi and virtio-blk would benefit from the new hook.
---
drivers/block/virtio_blk.c | 2 ++
drivers/virtio/virtio_pci_common.c | 6 ++++++
drivers/virtio/virtio_pci_common.h | 2 ++
drivers/virtio/virtio_pci_legacy.c | 1 +
drivers/virtio/virtio_pci_modern.c | 2 ++
include/linux/virtio_config.h | 11 +++++++++++
6 files changed, 24 insertions(+)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4bc083b..93cfeda 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
if (err)
num_vqs = 1;
+ num_vqs = virtio_calc_num_vqs(vdev, num_vqs);
+
vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
if (!vblk->vqs)
return -ENOMEM;
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index d0584c0..ce021d1 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -409,6 +409,12 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
}
+/* the config->calc_num_vqs() implementation */
+unsigned short vp_calc_num_vqs(unsigned short num_vqs)
+{
+ return min_t(unsigned short, num_possible_cpus(), num_vqs);
+}
+
const char *vp_bus_name(struct virtio_device *vdev)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 0227100..cc5ac80 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -134,6 +134,8 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[], vq_callback_t *callbacks[],
const char * const names[], const bool *ctx,
struct irq_affinity *desc);
+/* the config->calc_num_vqs() implementation */
+unsigned short vp_calc_num_vqs(unsigned short num_vqs);
const char *vp_bus_name(struct virtio_device *vdev);
/* Setup the affinity for a virtqueue:
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index eff9ddc..69d1050 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -209,6 +209,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
.bus_name = vp_bus_name,
.set_vq_affinity = vp_set_vq_affinity,
.get_vq_affinity = vp_get_vq_affinity,
+ .calc_num_vqs = vp_calc_num_vqs,
};
/* the PCI probing function */
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 07571da..f04e44a 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -460,6 +460,7 @@ static const struct virtio_config_ops
virtio_pci_config_nodev_ops = {
.bus_name = vp_bus_name,
.set_vq_affinity = vp_set_vq_affinity,
.get_vq_affinity = vp_get_vq_affinity,
+ .calc_num_vqs = vp_calc_num_vqs,
};
static const struct virtio_config_ops virtio_pci_config_ops = {
@@ -476,6 +477,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
.bus_name = vp_bus_name,
.set_vq_affinity = vp_set_vq_affinity,
.get_vq_affinity = vp_get_vq_affinity,
+ .calc_num_vqs = vp_calc_num_vqs,
};
/**
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index bb4cc49..f32368b 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -65,6 +65,7 @@ struct irq_affinity;
* the caller can then copy.
* @set_vq_affinity: set the affinity for a virtqueue (optional).
* @get_vq_affinity: get the affinity for a virtqueue (optional).
+ * @calc_num_vqs: calculate the number of virtqueue (optional)
*/
typedef void vq_callback_t(struct virtqueue *);
struct virtio_config_ops {
@@ -88,6 +89,7 @@ struct virtio_config_ops {
const struct cpumask *cpu_mask);
const struct cpumask *(*get_vq_affinity)(struct virtio_device *vdev,
int index);
+ unsigned short (*calc_num_vqs)(unsigned short num_vqs);
};
/* If driver didn't advertise the feature, it will never appear. */
@@ -207,6 +209,15 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev,
unsigned nvqs,
desc);
}
+static inline
+unsigned short virtio_calc_num_vqs(struct virtio_device *vdev,
+ unsigned short num_vqs)
+{
+ if (vdev->config->calc_num_vqs)
+ return vdev->config->calc_num_vqs(num_vqs);
+ return num_vqs;
+}
+
/**
* virtio_device_ready - enable vq use in probe function
* @vdev: the device
--
2.7.4
Thank you very much!
Dongli Zhang
^ permalink raw reply related
* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Cornelia Huck @ 2019-03-13 9:39 UTC (permalink / raw)
To: Dongli Zhang; +Cc: linux-block, axboe, mst, linux-kernel, virtualization
In-Reply-To: <173d19c9-24db-35f2-269f-0b9b83bd0ad6@oracle.com>
On Wed, 13 Mar 2019 11:26:04 +0800
Dongli Zhang <dongli.zhang@oracle.com> wrote:
> On 3/13/19 1:33 AM, Cornelia Huck wrote:
> > On Tue, 12 Mar 2019 10:22:46 -0700 (PDT)
> > Dongli Zhang <dongli.zhang@oracle.com> wrote:
> >
> >> I observed that there is one msix vector for config and one shared vector
> >> for all queues in below qemu cmdline, when the num-queues for virtio-blk
> >> is more than the number of possible cpus:
> >>
> >> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=6"
> >>
> >> # cat /proc/interrupts
> >> CPU0 CPU1 CPU2 CPU3
> >> ... ...
> >> 24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
> >> 25: 0 0 0 59 PCI-MSI 65537-edge virtio0-virtqueues
> >> ... ...
> >>
> >>
> >> However, when num-queues is the same as number of possible cpus:
> >>
> >> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=4"
> >>
> >> # cat /proc/interrupts
> >> CPU0 CPU1 CPU2 CPU3
> >> ... ...
> >> 24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
> >> 25: 2 0 0 0 PCI-MSI 65537-edge virtio0-req.0
> >> 26: 0 35 0 0 PCI-MSI 65538-edge virtio0-req.1
> >> 27: 0 0 32 0 PCI-MSI 65539-edge virtio0-req.2
> >> 28: 0 0 0 0 PCI-MSI 65540-edge virtio0-req.3
> >> ... ...
> >>
> >> In above case, there is one msix vector per queue.
> >
> > Please note that this is pci-specific...
> >
> >>
> >>
> >> This is because the max number of queues is not limited by the number of
> >> possible cpus.
> >>
> >> By default, nvme (regardless about write_queues and poll_queues) and
> >> xen-blkfront limit the number of queues with num_possible_cpus().
> >
> > ...and these are probably pci-specific as well.
>
> Not pci-specific, but per-cpu as well.
Ah, I meant that those are pci devices.
>
> >
> >>
> >>
> >> Is this by design on purpose, or can we fix with below?
> >>
> >>
> >> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> >> index 4bc083b..df95ce3 100644
> >> --- a/drivers/block/virtio_blk.c
> >> +++ b/drivers/block/virtio_blk.c
> >> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
> >> if (err)
> >> num_vqs = 1;
> >>
> >> + num_vqs = min(num_possible_cpus(), num_vqs);
> >> +
> >> vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
> >> if (!vblk->vqs)
> >> return -ENOMEM;
> >
> > virtio-blk, however, is not pci-specific.
> >
> > If we are using the ccw transport on s390, a completely different
> > interrupt mechanism is in use ('floating' interrupts, which are not
> > per-cpu). A check like that should therefore not go into the generic
> > driver.
> >
>
> So far there seems two options.
>
> The 1st option is to ask the qemu user to always specify "-num-queues" with the
> same number of vcpus when running x86 guest with pci for virtio-blk or
> virtio-scsi, in order to assign a vector for each queue.
That does seem like an extra burden for the user: IIUC, things work
even if you have too many queues, it's just not optimal. It sounds like
something that can be done by a management layer (e.g. libvirt), though.
> Or, is it fine for virtio folks to add a new hook to 'struct virtio_config_ops'
> so that different platforms (e.g., pci or ccw) would use different ways to limit
> the max number of queues in guest, with something like below?
That sounds better, as both transports and drivers can opt-in here.
However, maybe it would be even better to try to come up with a better
strategy of allocating msix vectors in virtio-pci. More vectors in the
num_queues > num_cpus case, even if they still need to be shared?
Individual vectors for n-1 cpus and then a shared one for the remaining
queues?
It might even be device-specific: Have some low-traffic status queues
share a vector, and provide an individual vector for high-traffic
queues. Would need some device<->transport interface, obviously.
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Christoph Hellwig @ 2019-03-13 16:05 UTC (permalink / raw)
To: James Bottomley
Cc: Andrea Arcangeli, linux-parisc, kvm, Michael S. Tsirkin, netdev,
linux-kernel, virtualization, hch, linux-mm, David Miller,
linux-arm-kernel
In-Reply-To: <1552424017.14432.11.camel@HansenPartnership.com>
On Tue, Mar 12, 2019 at 01:53:37PM -0700, James Bottomley wrote:
> I've got to say: optimize what? What code do we ever have in the
> kernel that kmap's a page and then doesn't do anything with it? You can
> guarantee that on kunmap the page is either referenced (needs
> invalidating) or updated (needs flushing). The in-kernel use of kmap is
> always
>
> kmap
> do something with the mapped page
> kunmap
>
> In a very short interval. It seems just a simplification to make
> kunmap do the flush if needed rather than try to have the users
> remember. The thing which makes this really simple is that on most
> architectures flush and invalidate is the same operation. If you
> really want to optimize you can use the referenced and dirty bits on
> the kmapped pte to tell you what operation to do, but if your flush is
> your invalidate, you simply assume the data needs flushing on kunmap
> without checking anything.
I agree that this would be a good way to simplify the API. Now
we'd just need volunteers to implement this for all architectures
that need cache flushing and then remove the explicit flushing in
the callers..
> > Which means after we fix vhost to add the flush_dcache_page after
> > kunmap, Parisc will get a double hit (but it also means Parisc was
> > the only one of those archs needed explicit cache flushes, where
> > vhost worked correctly so far.. so it kinds of proofs your point of
> > giving up being the safe choice).
>
> What double hit? If there's no cache to flush then cache flush is a
> no-op. It's also a highly piplineable no-op because the CPU has the L1
> cache within easy reach. The only event when flush takes a large
> amount time is if we actually have dirty data to write back to main
> memory.
I've heard people complaining that on some microarchitectures even
no-op cache flushes are relatively expensive. Don't ask me why,
but if we can easily avoid double flushes we should do that.
^ permalink raw reply
* [RFC] vhost: select TAP if VHOST is configured
From: Stephen Hemminger @ 2019-03-13 17:49 UTC (permalink / raw)
To: mst, jasowang, sainath.grandhi; +Cc: virtualization
If VHOST_NET is configured but TUN and TAP are not, then the
kernel will build but vhost will not work correctly since it can't
setup the necessary tap device.
A solution is to select it.
Fixes: 9a393b5d5988 ("tap: tap as an independent module")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
drivers/vhost/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index b580885243f7..a24c69598241 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -1,7 +1,8 @@
config VHOST_NET
tristate "Host kernel accelerator for virtio net"
- depends on NET && EVENTFD && (TUN || !TUN) && (TAP || !TAP)
+ depends on NET && EVENTFD
select VHOST
+ select TAP
---help---
This kernel module can be loaded in host kernel to accelerate
guest networking with virtio_net. Not to be confused with virtio_net
--
2.17.1
^ permalink raw reply related
* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Dongli Zhang @ 2019-03-14 6:12 UTC (permalink / raw)
To: Cornelia Huck, mst, jasowang
Cc: linux-block, axboe, linux-kernel, virtualization
In-Reply-To: <20190313103900.1ea7f996.cohuck@redhat.com>
On 3/13/19 5:39 PM, Cornelia Huck wrote:
> On Wed, 13 Mar 2019 11:26:04 +0800
> Dongli Zhang <dongli.zhang@oracle.com> wrote:
>
>> On 3/13/19 1:33 AM, Cornelia Huck wrote:
>>> On Tue, 12 Mar 2019 10:22:46 -0700 (PDT)
>>> Dongli Zhang <dongli.zhang@oracle.com> wrote:
>>>
>>>> I observed that there is one msix vector for config and one shared vector
>>>> for all queues in below qemu cmdline, when the num-queues for virtio-blk
>>>> is more than the number of possible cpus:
>>>>
>>>> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=6"
>>>>
>>>> # cat /proc/interrupts
>>>> CPU0 CPU1 CPU2 CPU3
>>>> ... ...
>>>> 24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
>>>> 25: 0 0 0 59 PCI-MSI 65537-edge virtio0-virtqueues
>>>> ... ...
>>>>
>>>>
>>>> However, when num-queues is the same as number of possible cpus:
>>>>
>>>> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=4"
>>>>
>>>> # cat /proc/interrupts
>>>> CPU0 CPU1 CPU2 CPU3
>>>> ... ...
>>>> 24: 0 0 0 0 PCI-MSI 65536-edge virtio0-config
>>>> 25: 2 0 0 0 PCI-MSI 65537-edge virtio0-req.0
>>>> 26: 0 35 0 0 PCI-MSI 65538-edge virtio0-req.1
>>>> 27: 0 0 32 0 PCI-MSI 65539-edge virtio0-req.2
>>>> 28: 0 0 0 0 PCI-MSI 65540-edge virtio0-req.3
>>>> ... ...
>>>>
>>>> In above case, there is one msix vector per queue.
>>>
>>> Please note that this is pci-specific...
>>>
>>>>
>>>>
>>>> This is because the max number of queues is not limited by the number of
>>>> possible cpus.
>>>>
>>>> By default, nvme (regardless about write_queues and poll_queues) and
>>>> xen-blkfront limit the number of queues with num_possible_cpus().
>>>
>>> ...and these are probably pci-specific as well.
>>
>> Not pci-specific, but per-cpu as well.
>
> Ah, I meant that those are pci devices.
>
>>
>>>
>>>>
>>>>
>>>> Is this by design on purpose, or can we fix with below?
>>>>
>>>>
>>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>>>> index 4bc083b..df95ce3 100644
>>>> --- a/drivers/block/virtio_blk.c
>>>> +++ b/drivers/block/virtio_blk.c
>>>> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
>>>> if (err)
>>>> num_vqs = 1;
>>>>
>>>> + num_vqs = min(num_possible_cpus(), num_vqs);
>>>> +
>>>> vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
>>>> if (!vblk->vqs)
>>>> return -ENOMEM;
>>>
>>> virtio-blk, however, is not pci-specific.
>>>
>>> If we are using the ccw transport on s390, a completely different
>>> interrupt mechanism is in use ('floating' interrupts, which are not
>>> per-cpu). A check like that should therefore not go into the generic
>>> driver.
>>>
>>
>> So far there seems two options.
>>
>> The 1st option is to ask the qemu user to always specify "-num-queues" with the
>> same number of vcpus when running x86 guest with pci for virtio-blk or
>> virtio-scsi, in order to assign a vector for each queue.
>
> That does seem like an extra burden for the user: IIUC, things work
> even if you have too many queues, it's just not optimal. It sounds like
> something that can be done by a management layer (e.g. libvirt), though.
>
>> Or, is it fine for virtio folks to add a new hook to 'struct virtio_config_ops'
>> so that different platforms (e.g., pci or ccw) would use different ways to limit
>> the max number of queues in guest, with something like below?
>
> That sounds better, as both transports and drivers can opt-in here.
>
> However, maybe it would be even better to try to come up with a better
> strategy of allocating msix vectors in virtio-pci. More vectors in the
> num_queues > num_cpus case, even if they still need to be shared?
> Individual vectors for n-1 cpus and then a shared one for the remaining
> queues?
>
> It might even be device-specific: Have some low-traffic status queues
> share a vector, and provide an individual vector for high-traffic
> queues. Would need some device<->transport interface, obviously.
>
This sounds a little bit similar to multiple hctx maps?
So far, as virtio-blk only supports set->nr_maps = 1, no matter how many hw
queues are assigned for virtio-blk, blk_mq_alloc_tag_set() would use at most
nr_cpu_ids hw queues.
2981 int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
... ...
3021 /*
3022 * There is no use for more h/w queues than cpus if we just have
3023 * a single map
3024 */
3025 if (set->nr_maps == 1 && set->nr_hw_queues > nr_cpu_ids)
3026 set->nr_hw_queues = nr_cpu_ids;
Even the block layer would limit the number of hw queues by nr_cpu_ids when
(set->nr_maps == 1).
That's why I think virtio-blk should use the similar solution as nvme
(regardless about write_queues and poll_queues) and xen-blkfront.
Added Jason again. I do not know why the mailing list of
virtualization@lists.linux-foundation.org always filters out Jason's email...
Dongli Zhang
^ permalink raw reply
* Re: [RFC] vhost: select TAP if VHOST is configured
From: Michael S. Tsirkin @ 2019-03-14 10:38 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: virtualization, sainath.grandhi
In-Reply-To: <20190313174932.5793-1-stephen@networkplumber.org>
On Wed, Mar 13, 2019 at 10:49:32AM -0700, Stephen Hemminger wrote:
> If VHOST_NET is configured but TUN and TAP are not, then the
> kernel will build but vhost will not work correctly since it can't
> setup the necessary tap device.
>
> A solution is to select it.
>
> Fixes: 9a393b5d5988 ("tap: tap as an independent module")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Well vhost can also work with a SOCK_RAW socket.
QEMU userspace does not seem to use that interface
pobably because we never got around to adding
offload support, but it's there and we don't know
that no one else does.
> ---
> drivers/vhost/Kconfig | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index b580885243f7..a24c69598241 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -1,7 +1,8 @@
> config VHOST_NET
> tristate "Host kernel accelerator for virtio net"
> - depends on NET && EVENTFD && (TUN || !TUN) && (TAP || !TAP)
> + depends on NET && EVENTFD
> select VHOST
> + select TAP
> ---help---
> This kernel module can be loaded in host kernel to accelerate
> guest networking with virtio_net. Not to be confused with virtio_net
> --
> 2.17.1
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Michael S. Tsirkin @ 2019-03-14 10:42 UTC (permalink / raw)
To: James Bottomley
Cc: Andrea Arcangeli, linux-parisc, kvm, netdev, linux-kernel,
virtualization, Christoph Hellwig, linux-mm, David Miller,
linux-arm-kernel
In-Reply-To: <1552495028.3022.37.camel@HansenPartnership.com>
On Wed, Mar 13, 2019 at 09:37:08AM -0700, James Bottomley wrote:
> On Wed, 2019-03-13 at 09:05 -0700, Christoph Hellwig wrote:
> > On Tue, Mar 12, 2019 at 01:53:37PM -0700, James Bottomley wrote:
> > > I've got to say: optimize what? What code do we ever have in the
> > > kernel that kmap's a page and then doesn't do anything with it? You
> > > can
> > > guarantee that on kunmap the page is either referenced (needs
> > > invalidating) or updated (needs flushing). The in-kernel use of
> > > kmap is
> > > always
> > >
> > > kmap
> > > do something with the mapped page
> > > kunmap
> > >
> > > In a very short interval. It seems just a simplification to make
> > > kunmap do the flush if needed rather than try to have the users
> > > remember. The thing which makes this really simple is that on most
> > > architectures flush and invalidate is the same operation. If you
> > > really want to optimize you can use the referenced and dirty bits
> > > on the kmapped pte to tell you what operation to do, but if your
> > > flush is your invalidate, you simply assume the data needs flushing
> > > on kunmap without checking anything.
> >
> > I agree that this would be a good way to simplify the API. Now
> > we'd just need volunteers to implement this for all architectures
> > that need cache flushing and then remove the explicit flushing in
> > the callers..
>
> Well, it's already done on parisc ... I can help with this if we agree
> it's the best way forward. It's really only architectures that
> implement flush_dcache_page that would need modifying.
>
> It may also improve performance because some kmap/use/flush/kunmap
> sequences have flush_dcache_page() instead of
> flush_kernel_dcache_page() and the former is hugely expensive and
> usually unnecessary because GUP already flushed all the user aliases.
>
> In the interests of full disclosure the reason we do it for parisc is
> because our later machines have problems even with clean aliases. So
> on most VIPT systems, doing kmap/read/kunmap creates a fairly harmless
> clean alias. Technically it should be invalidated, because if you
> remap the same page to the same colour you get cached stale data, but
> in practice the data is expired from the cache long before that
> happens, so the problem is almost never seen if the flush is forgotten.
> Our problem is on the P9xxx processor: they have a L1/L2 VIPT L3 PIPT
> cache. As the L1/L2 caches expire clean data, they place the expiring
> contents into L3, but because L3 is PIPT, the stale alias suddenly
> becomes the default for any read of they physical page because any
> update which dirtied the cache line often gets written to main memory
> and placed into the L3 as clean *before* the clean alias in L1/L2 gets
> expired, so the older clean alias replaces it.
>
> Our only recourse is to kill all aliases with prejudice before the
> kernel loses ownership.
>
> > > > Which means after we fix vhost to add the flush_dcache_page after
> > > > kunmap, Parisc will get a double hit (but it also means Parisc
> > > > was the only one of those archs needed explicit cache flushes,
> > > > where vhost worked correctly so far.. so it kinds of proofs your
> > > > point of giving up being the safe choice).
> > >
> > > What double hit? If there's no cache to flush then cache flush is
> > > a no-op. It's also a highly piplineable no-op because the CPU has
> > > the L1 cache within easy reach. The only event when flush takes a
> > > large amount time is if we actually have dirty data to write back
> > > to main memory.
> >
> > I've heard people complaining that on some microarchitectures even
> > no-op cache flushes are relatively expensive. Don't ask me why,
> > but if we can easily avoid double flushes we should do that.
>
> It's still not entirely free for us. Our internal cache line is around
> 32 bytes (some have 16 and some have 64) but that means we need 128
> flushes for a page ... we definitely can't pipeline them all. So I
> agree duplicate flush elimination would be a small improvement.
>
> James
I suspect we'll keep the copyXuser path around for 32 bit anyway -
right Jason?
So we can also keep using that on parisc...
--
MST
^ 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