* Re: [PATCH] kvm/arm: return 0 when the number of objects is not less than min
@ 2018-12-05 8:32 ` Andrew Jones
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Jones @ 2018-12-05 8:32 UTC (permalink / raw)
To: Peng Hao
Cc: marc.zyngier, linux-kernel, christoffer.dall, linux-arm-kernel, kvmarm
On Wed, Dec 05, 2018 at 09:15:51AM +0800, Peng Hao wrote:
> Return 0 when there is enough kvm_mmu_memory_cache object.
>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
> virt/kvm/arm/mmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> index ed162a6..fcda0ce 100644
> --- a/virt/kvm/arm/mmu.c
> +++ b/virt/kvm/arm/mmu.c
> @@ -127,7 +127,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> while (cache->nobjs < max) {
> page = (void *)__get_free_page(PGALLOC_GFP);
> if (!page)
> - return -ENOMEM;
> + return cache->nobjs >= min ? 0 : -ENOMEM;
This condition will never be true here, as the exact same condition is
already checked above, and if it had been true, then we wouldn't be here.
What problem are you attempting to solve?
> cache->objects[cache->nobjs++] = page;
> }
> return 0;
> --
> 1.8.3.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] kvm/arm: return 0 when the number of objects is not lessthan min
2018-12-05 8:32 ` Andrew Jones
(?)
@ 2018-12-06 1:56 ` peng.hao2
2018-12-06 14:29 ` Andrew Jones
2018-12-10 13:13 ` Christoffer Dall
-1 siblings, 2 replies; 11+ messages in thread
From: peng.hao2 @ 2018-12-06 1:56 UTC (permalink / raw)
To: drjones; +Cc: marc.zyngier, linux-kernel, linux-arm-kernel, kvmarm
[-- Attachment #1.1: Type: text/plain, Size: 1595 bytes --]
>On Wed, Dec 05, 2018 at 09:15:51AM +0800, Peng Hao wrote:
>> Return 0 when there is enough kvm_mmu_memory_cache object.
>>
>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>> ---
>> virt/kvm/arm/mmu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index ed162a6..fcda0ce 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -127,7 +127,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
>> while (cache->nobjs < max) {
>> page = (void *)__get_free_page(PGALLOC_GFP);
>> if (!page)
>> - return -ENOMEM;
>> + return cache->nobjs >= min ? 0 : -ENOMEM;
>
>This condition will never be true here, as the exact same condition is
>already checked above, and if it had been true, then we wouldn't be here.
>
>What problem are you attempting to solve?
>
if (cache->nobjs >= min) ------here cache->nobjs<min,it can continue downward
return 0;
while (cache->nobjs < max) {
page = (void *)__get_free_page(PGALLOC_GFP);
if (!page)
return -ENOMEM; -----here it is possible that (cache->nobjs >= min) and (cache->nobjs<max)
cache->objects[cache->nobjs++] = page; ---here cache->nobjs increasing
}
I just think the logic of this function is to return 0 as long as (cache->nobjs >= min).
thanks.
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[-- Attachment #2: Type: text/plain, Size: 151 bytes --]
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] kvm/arm: return 0 when the number of objects is not lessthan min
2018-12-06 1:56 ` [PATCH] kvm/arm: return 0 when the number of objects is not lessthan min peng.hao2
@ 2018-12-06 14:29 ` Andrew Jones
2018-12-10 13:13 ` Christoffer Dall
1 sibling, 0 replies; 11+ messages in thread
From: Andrew Jones @ 2018-12-06 14:29 UTC (permalink / raw)
To: peng.hao2
Cc: christoffer.dall, marc.zyngier, kvmarm, linux-arm-kernel, linux-kernel
On Thu, Dec 06, 2018 at 09:56:30AM +0800, peng.hao2@zte.com.cn wrote:
> >On Wed, Dec 05, 2018 at 09:15:51AM +0800, Peng Hao wrote:
> >> Return 0 when there is enough kvm_mmu_memory_cache object.
> >>
> >> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> >> ---
> >> virt/kvm/arm/mmu.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> >> index ed162a6..fcda0ce 100644
> >> --- a/virt/kvm/arm/mmu.c
> >> +++ b/virt/kvm/arm/mmu.c
> >> @@ -127,7 +127,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> >> while (cache->nobjs < max) {
> >> page = (void *)__get_free_page(PGALLOC_GFP);
> >> if (!page)
> >> - return -ENOMEM;
> >> + return cache->nobjs >= min ? 0 : -ENOMEM;
> >
> >This condition will never be true here, as the exact same condition is
> >already checked above, and if it had been true, then we wouldn't be here.
> >
> >What problem are you attempting to solve?
> >
> if (cache->nobjs >= min) ------here cache->nobjs<min,it can continue downward
> return 0;
> while (cache->nobjs < max) {
> page = (void *)__get_free_page(PGALLOC_GFP);
> if (!page)
> return -ENOMEM; -----here it is possible that (cache->nobjs >= min) and (cache->nobjs<max)
> cache->objects[cache->nobjs++] = page; ---here cache->nobjs increasing
> }
>
> I just think the logic of this function is to return 0 as long as (cache->nobjs >= min).
> thanks.
Oh, I see now. This is the case where we can do enough allocating to over
the min line, but fail before we get to the max.
Thanks,
drew
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] kvm/arm: return 0 when the number of objects is not lessthan min
@ 2018-12-06 14:29 ` Andrew Jones
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Jones @ 2018-12-06 14:29 UTC (permalink / raw)
To: peng.hao2
Cc: marc.zyngier, linux-kernel, christoffer.dall, linux-arm-kernel, kvmarm
On Thu, Dec 06, 2018 at 09:56:30AM +0800, peng.hao2@zte.com.cn wrote:
> >On Wed, Dec 05, 2018 at 09:15:51AM +0800, Peng Hao wrote:
> >> Return 0 when there is enough kvm_mmu_memory_cache object.
> >>
> >> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> >> ---
> >> virt/kvm/arm/mmu.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> >> index ed162a6..fcda0ce 100644
> >> --- a/virt/kvm/arm/mmu.c
> >> +++ b/virt/kvm/arm/mmu.c
> >> @@ -127,7 +127,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> >> while (cache->nobjs < max) {
> >> page = (void *)__get_free_page(PGALLOC_GFP);
> >> if (!page)
> >> - return -ENOMEM;
> >> + return cache->nobjs >= min ? 0 : -ENOMEM;
> >
> >This condition will never be true here, as the exact same condition is
> >already checked above, and if it had been true, then we wouldn't be here.
> >
> >What problem are you attempting to solve?
> >
> if (cache->nobjs >= min) ------here cache->nobjs<min,it can continue downward
> return 0;
> while (cache->nobjs < max) {
> page = (void *)__get_free_page(PGALLOC_GFP);
> if (!page)
> return -ENOMEM; -----here it is possible that (cache->nobjs >= min) and (cache->nobjs<max)
> cache->objects[cache->nobjs++] = page; ---here cache->nobjs increasing
> }
>
> I just think the logic of this function is to return 0 as long as (cache->nobjs >= min).
> thanks.
Oh, I see now. This is the case where we can do enough allocating to over
the min line, but fail before we get to the max.
Thanks,
drew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kvm/arm: return 0 when the number of objects is not lessthan min
2018-12-06 1:56 ` [PATCH] kvm/arm: return 0 when the number of objects is not lessthan min peng.hao2
@ 2018-12-10 13:13 ` Christoffer Dall
2018-12-10 13:13 ` Christoffer Dall
1 sibling, 0 replies; 11+ messages in thread
From: Christoffer Dall @ 2018-12-10 13:13 UTC (permalink / raw)
To: peng.hao2; +Cc: drjones, marc.zyngier, kvmarm, linux-arm-kernel, linux-kernel
On Thu, Dec 06, 2018 at 09:56:30AM +0800, peng.hao2@zte.com.cn wrote:
> >On Wed, Dec 05, 2018 at 09:15:51AM +0800, Peng Hao wrote:
> >> Return 0 when there is enough kvm_mmu_memory_cache object.
> >>
> >> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> >> ---
> >> virt/kvm/arm/mmu.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> >> index ed162a6..fcda0ce 100644
> >> --- a/virt/kvm/arm/mmu.c
> >> +++ b/virt/kvm/arm/mmu.c
> >> @@ -127,7 +127,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> >> while (cache->nobjs < max) {
> >> page = (void *)__get_free_page(PGALLOC_GFP);
> >> if (!page)
> >> - return -ENOMEM;
> >> + return cache->nobjs >= min ? 0 : -ENOMEM;
> >
> >This condition will never be true here, as the exact same condition is
> >already checked above, and if it had been true, then we wouldn't be here.
> >
> >What problem are you attempting to solve?
> >
> if (cache->nobjs >= min) ------here cache->nobjs<min,it can continue downward
> return 0;
> while (cache->nobjs < max) {
> page = (void *)__get_free_page(PGALLOC_GFP);
> if (!page)
> return -ENOMEM; -----here it is possible that (cache->nobjs >= min) and (cache->nobjs<max)
> cache->objects[cache->nobjs++] = page; ---here cache->nobjs increasing
> }
>
> I just think the logic of this function is to return 0 as long as (cache->nobjs >= min).
> thanks.
That's not the intention, nor is it on any of the other architectures
implementing the same thing (this one goes on the list of stuff we
should be sharing between architectures).
The idea is that you fill up the cache when it goes below min, and you
are always able to fill up to max.
If you're not able to fill up to max, then your system is seriously low
on memory and continuing to run this VM is not likely to be a good idea,
so you might as well tell user space to do something now instead of
waiting until the situation is even worse.
Thanks,
Christoffer
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] kvm/arm: return 0 when the number of objects is not lessthan min
@ 2018-12-10 13:13 ` Christoffer Dall
0 siblings, 0 replies; 11+ messages in thread
From: Christoffer Dall @ 2018-12-10 13:13 UTC (permalink / raw)
To: peng.hao2; +Cc: marc.zyngier, drjones, kvmarm, linux-arm-kernel, linux-kernel
On Thu, Dec 06, 2018 at 09:56:30AM +0800, peng.hao2@zte.com.cn wrote:
> >On Wed, Dec 05, 2018 at 09:15:51AM +0800, Peng Hao wrote:
> >> Return 0 when there is enough kvm_mmu_memory_cache object.
> >>
> >> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> >> ---
> >> virt/kvm/arm/mmu.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> >> index ed162a6..fcda0ce 100644
> >> --- a/virt/kvm/arm/mmu.c
> >> +++ b/virt/kvm/arm/mmu.c
> >> @@ -127,7 +127,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> >> while (cache->nobjs < max) {
> >> page = (void *)__get_free_page(PGALLOC_GFP);
> >> if (!page)
> >> - return -ENOMEM;
> >> + return cache->nobjs >= min ? 0 : -ENOMEM;
> >
> >This condition will never be true here, as the exact same condition is
> >already checked above, and if it had been true, then we wouldn't be here.
> >
> >What problem are you attempting to solve?
> >
> if (cache->nobjs >= min) ------here cache->nobjs<min,it can continue downward
> return 0;
> while (cache->nobjs < max) {
> page = (void *)__get_free_page(PGALLOC_GFP);
> if (!page)
> return -ENOMEM; -----here it is possible that (cache->nobjs >= min) and (cache->nobjs<max)
> cache->objects[cache->nobjs++] = page; ---here cache->nobjs increasing
> }
>
> I just think the logic of this function is to return 0 as long as (cache->nobjs >= min).
> thanks.
That's not the intention, nor is it on any of the other architectures
implementing the same thing (this one goes on the list of stuff we
should be sharing between architectures).
The idea is that you fill up the cache when it goes below min, and you
are always able to fill up to max.
If you're not able to fill up to max, then your system is seriously low
on memory and continuing to run this VM is not likely to be a good idea,
so you might as well tell user space to do something now instead of
waiting until the situation is even worse.
Thanks,
Christoffer
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kvm/arm: return 0 when the number of objects is not less than min
2018-12-05 8:32 ` Andrew Jones
@ 2018-12-07 14:49 ` Steven Price
-1 siblings, 0 replies; 11+ messages in thread
From: Steven Price @ 2018-12-07 14:49 UTC (permalink / raw)
To: Andrew Jones, Peng Hao
Cc: marc.zyngier, linux-kernel, linux-arm-kernel, kvmarm
On 05/12/2018 08:32, Andrew Jones wrote:
> On Wed, Dec 05, 2018 at 09:15:51AM +0800, Peng Hao wrote:
>> Return 0 when there is enough kvm_mmu_memory_cache object.
>>
>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>> ---
>> virt/kvm/arm/mmu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index ed162a6..fcda0ce 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -127,7 +127,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
>> while (cache->nobjs < max) {
>> page = (void *)__get_free_page(PGALLOC_GFP);
>> if (!page)
>> - return -ENOMEM;
>> + return cache->nobjs >= min ? 0 : -ENOMEM;
>
> This condition will never be true here, as the exact same condition is
> already checked above, and if it had been true, then we wouldn't be here.
The condition can be true if the loop is executed at least once. This
change would appear to allow the call to succeed in the case where the
cache can be topped up to at least "min", but not as far as "max".
It would be good to know in what situation this has actually been hit
though (and indeed whether this has actually been seen) - the system
must be very short on memory to need this, and I'd be surprised if
further failures didn't happen later on.
> What problem are you attempting to solve?
>
>> cache->objects[cache->nobjs++] = page;
>> }
>> return 0;
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] kvm/arm: return 0 when the number of objects is not less than min
@ 2018-12-07 14:49 ` Steven Price
0 siblings, 0 replies; 11+ messages in thread
From: Steven Price @ 2018-12-07 14:49 UTC (permalink / raw)
To: Andrew Jones, Peng Hao
Cc: marc.zyngier, linux-kernel, linux-arm-kernel, kvmarm
On 05/12/2018 08:32, Andrew Jones wrote:
> On Wed, Dec 05, 2018 at 09:15:51AM +0800, Peng Hao wrote:
>> Return 0 when there is enough kvm_mmu_memory_cache object.
>>
>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>> ---
>> virt/kvm/arm/mmu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index ed162a6..fcda0ce 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -127,7 +127,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
>> while (cache->nobjs < max) {
>> page = (void *)__get_free_page(PGALLOC_GFP);
>> if (!page)
>> - return -ENOMEM;
>> + return cache->nobjs >= min ? 0 : -ENOMEM;
>
> This condition will never be true here, as the exact same condition is
> already checked above, and if it had been true, then we wouldn't be here.
The condition can be true if the loop is executed at least once. This
change would appear to allow the call to succeed in the case where the
cache can be topped up to at least "min", but not as far as "max".
It would be good to know in what situation this has actually been hit
though (and indeed whether this has actually been seen) - the system
must be very short on memory to need this, and I'd be surprised if
further failures didn't happen later on.
> What problem are you attempting to solve?
>
>> cache->objects[cache->nobjs++] = page;
>> }
>> return 0;
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread