QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr
@ 2019-07-30  0:37 Wei Yang
  2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check Wei Yang
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Wei Yang @ 2019-07-30  0:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, david, Wei Yang, mst

When we iterate the memory-device list to get the available range, it is not
necessary to iterate the whole list.

1) no more overlap for hinted range if tmp exceed it

v2:
   * remove #2 as suggested by Igor and David
   * add some comment to inform address assignment stay the same as before
     this change 

Wei Yang (2):
  memory-device: not necessary to use goto for the last check
  memory-device: break the loop if tmp exceed the hinted range

 hw/mem/memory-device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-07-30  0:37 [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr Wei Yang
@ 2019-07-30  0:37 ` Wei Yang
  2019-08-08  1:42   ` Zeng, Star
  2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range Wei Yang
  2019-09-13 23:47 ` [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr Wei Yang
  2 siblings, 1 reply; 15+ messages in thread
From: Wei Yang @ 2019-07-30  0:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, david, Wei Yang, mst

We are already at the last condition check.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 5f2c408036..df3261b32a 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -186,7 +186,6 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");
-        goto out;
     }
 out:
     g_slist_free(list);
-- 
2.17.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range
  2019-07-30  0:37 [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr Wei Yang
  2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check Wei Yang
@ 2019-07-30  0:37 ` Wei Yang
  2019-07-30  7:38   ` David Hildenbrand
  2019-07-30  9:30   ` Igor Mammedov
  2019-09-13 23:47 ` [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr Wei Yang
  2 siblings, 2 replies; 15+ messages in thread
From: Wei Yang @ 2019-07-30  0:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, david, Wei Yang, mst

The memory-device list built by memory_device_build_list is ordered by
its address, this means if the tmp range exceed the hinted range, all
the following range will not overlap with it.

And this won't change default pc-dimm mapping and address assignment stay
the same as before this change.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/mem/memory-device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index df3261b32a..df4e338b83 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -180,6 +180,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                 range_make_empty(&new);
                 break;
             }
+        } else if (range_lob(&tmp) > range_upb(&new)) {
+            break;
         }
     }
 
-- 
2.17.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range
  2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range Wei Yang
@ 2019-07-30  7:38   ` David Hildenbrand
  2019-07-30  9:30   ` Igor Mammedov
  1 sibling, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-07-30  7:38 UTC (permalink / raw)
  To: Wei Yang, qemu-devel; +Cc: imammedo, mst

On 30.07.19 02:37, Wei Yang wrote:
> The memory-device list built by memory_device_build_list is ordered by
> its address, this means if the tmp range exceed the hinted range, all
> the following range will not overlap with it.
> 
> And this won't change default pc-dimm mapping and address assignment stay
> the same as before this change.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/mem/memory-device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index df3261b32a..df4e338b83 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -180,6 +180,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>                  range_make_empty(&new);
>                  break;
>              }
> +        } else if (range_lob(&tmp) > range_upb(&new)) {
> +            break;
>          }
>      }
>  
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range
  2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range Wei Yang
  2019-07-30  7:38   ` David Hildenbrand
@ 2019-07-30  9:30   ` Igor Mammedov
  1 sibling, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-07-30  9:30 UTC (permalink / raw)
  To: Wei Yang; +Cc: mst, qemu-devel, david

On Tue, 30 Jul 2019 08:37:40 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> The memory-device list built by memory_device_build_list is ordered by
> its address, this means if the tmp range exceed the hinted range, all
> the following range will not overlap with it.
> 
> And this won't change default pc-dimm mapping and address assignment stay
> the same as before this change.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/mem/memory-device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index df3261b32a..df4e338b83 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -180,6 +180,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>                  range_make_empty(&new);
>                  break;
>              }
> +        } else if (range_lob(&tmp) > range_upb(&new)) {
> +            break;
>          }
>      }
>  



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check Wei Yang
@ 2019-08-08  1:42   ` Zeng, Star
  2019-08-08  2:13     ` Wei Yang
  0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2019-08-08  1:42 UTC (permalink / raw)
  To: Wei Yang, qemu-devel; +Cc: imammedo, mst, Zeng, Star, david

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
> Sent: Tuesday, July 30, 2019 8:38 AM
> To: qemu-devel@nongnu.org
> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
> <richardw.yang@linux.intel.com>; mst@redhat.com
> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use
> goto for the last check
> 
> We are already at the last condition check.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/memory-device.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index
> 5f2c408036..df3261b32a 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -186,7 +186,6 @@ static uint64_t
> memory_device_get_free_addr(MachineState *ms,
>      if (!range_contains_range(&as, &new)) {
>          error_setg(errp, "could not find position in guest address space for "
>                     "memory device - memory fragmented due to alignments");
> -        goto out;

Is it better to return 0 (or set new_addr to 0) for this error path and another remaining "goto out" path?


Thanks,
Star

>      }
>  out:
>      g_slist_free(list);
> --
> 2.17.1
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-08-08  1:42   ` Zeng, Star
@ 2019-08-08  2:13     ` Wei Yang
  2019-08-08  2:30       ` Zeng, Star
  0 siblings, 1 reply; 15+ messages in thread
From: Wei Yang @ 2019-08-08  2:13 UTC (permalink / raw)
  To: Zeng, Star; +Cc: imammedo, david, mst, Wei Yang, qemu-devel

On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>> -----Original Message-----
>> From: Qemu-devel [mailto:qemu-devel-
>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>> Sent: Tuesday, July 30, 2019 8:38 AM
>> To: qemu-devel@nongnu.org
>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>> <richardw.yang@linux.intel.com>; mst@redhat.com
>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use
>> goto for the last check
>> 
>> We are already at the last condition check.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/mem/memory-device.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index
>> 5f2c408036..df3261b32a 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -186,7 +186,6 @@ static uint64_t
>> memory_device_get_free_addr(MachineState *ms,
>>      if (!range_contains_range(&as, &new)) {
>>          error_setg(errp, "could not find position in guest address space for "
>>                     "memory device - memory fragmented due to alignments");
>> -        goto out;
>
>Is it better to return 0 (or set new_addr to 0) for this error path and another remaining "goto out" path?
>

I may not get your point.

We set errp which is handled in its caller. By doing so, the error is
propagated.

Do I miss something?

>
>Thanks,
>Star
>
>>      }
>>  out:
>>      g_slist_free(list);
>> --
>> 2.17.1
>> 

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-08-08  2:13     ` Wei Yang
@ 2019-08-08  2:30       ` Zeng, Star
  2019-08-08  2:38         ` Wei Yang
  0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2019-08-08  2:30 UTC (permalink / raw)
  To: Wei Yang; +Cc: imammedo, mst, qemu-devel, Zeng, Star, david

> -----Original Message-----
> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
> Sent: Thursday, August 8, 2019 10:13 AM
> To: Zeng, Star <star.zeng@intel.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
> imammedo@redhat.com; david@redhat.com; mst@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> use goto for the last check
> 
> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
> >> -----Original Message-----
> >> From: Qemu-devel [mailto:qemu-devel-
> >> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
> >> Sent: Tuesday, July 30, 2019 8:38 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
> >> <richardw.yang@linux.intel.com>; mst@redhat.com
> >> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> >> use goto for the last check
> >>
> >> We are already at the last condition check.
> >>
> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >> Reviewed-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/mem/memory-device.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index
> >> 5f2c408036..df3261b32a 100644
> >> --- a/hw/mem/memory-device.c
> >> +++ b/hw/mem/memory-device.c
> >> @@ -186,7 +186,6 @@ static uint64_t
> >> memory_device_get_free_addr(MachineState *ms,
> >>      if (!range_contains_range(&as, &new)) {
> >>          error_setg(errp, "could not find position in guest address space for "
> >>                     "memory device - memory fragmented due to alignments");
> >> -        goto out;
> >
> >Is it better to return 0 (or set new_addr to 0) for this error path and another
> remaining "goto out" path?
> >
> 
> I may not get your point.
> 
> We set errp which is handled in its caller. By doing so, the error is propagated.
> 
> Do I miss something?

Yes, you are right. Currently, the caller is checking errp, but not the returned address, so there should be no issue.
But when you see other error paths, you will find they all return 0. To be aligned (return 0 when error), so just suggest also returning 0 for these two "goto out" error path. :)

Thanks,
Star

> 
> >
> >Thanks,
> >Star
> >
> >>      }
> >>  out:
> >>      g_slist_free(list);
> >> --
> >> 2.17.1
> >>
> 
> --
> Wei Yang
> Help you, Help me


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-08-08  2:30       ` Zeng, Star
@ 2019-08-08  2:38         ` Wei Yang
  2019-08-08  7:06           ` David Hildenbrand
  0 siblings, 1 reply; 15+ messages in thread
From: Wei Yang @ 2019-08-08  2:38 UTC (permalink / raw)
  To: Zeng, Star; +Cc: imammedo, david, mst, Wei Yang, qemu-devel

On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>> -----Original Message-----
>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>> Sent: Thursday, August 8, 2019 10:13 AM
>> To: Zeng, Star <star.zeng@intel.com>
>> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
>> imammedo@redhat.com; david@redhat.com; mst@redhat.com
>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>> use goto for the last check
>> 
>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>> >> -----Original Message-----
>> >> From: Qemu-devel [mailto:qemu-devel-
>> >> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>> >> Sent: Tuesday, July 30, 2019 8:38 AM
>> >> To: qemu-devel@nongnu.org
>> >> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>> >> <richardw.yang@linux.intel.com>; mst@redhat.com
>> >> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>> >> use goto for the last check
>> >>
>> >> We are already at the last condition check.
>> >>
>> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> >> Reviewed-by: David Hildenbrand <david@redhat.com>
>> >> ---
>> >>  hw/mem/memory-device.c | 1 -
>> >>  1 file changed, 1 deletion(-)
>> >>
>> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index
>> >> 5f2c408036..df3261b32a 100644
>> >> --- a/hw/mem/memory-device.c
>> >> +++ b/hw/mem/memory-device.c
>> >> @@ -186,7 +186,6 @@ static uint64_t
>> >> memory_device_get_free_addr(MachineState *ms,
>> >>      if (!range_contains_range(&as, &new)) {
>> >>          error_setg(errp, "could not find position in guest address space for "
>> >>                     "memory device - memory fragmented due to alignments");
>> >> -        goto out;
>> >
>> >Is it better to return 0 (or set new_addr to 0) for this error path and another
>> remaining "goto out" path?
>> >
>> 
>> I may not get your point.
>> 
>> We set errp which is handled in its caller. By doing so, the error is propagated.
>> 
>> Do I miss something?
>
>Yes, you are right. Currently, the caller is checking errp, but not the returned address, so there should be no issue.
>But when you see other error paths, you will find they all return 0. To be aligned (return 0 when error), so just suggest also returning 0 for these two "goto out" error path. :)
>

You may have some point.

Let's see whether others have the same taste, or we can refine it separately.

>Thanks,
>Star
>
>> 
>> >
>> >Thanks,
>> >Star
>> >
>> >>      }
>> >>  out:
>> >>      g_slist_free(list);
>> >> --
>> >> 2.17.1
>> >>
>> 
>> --
>> Wei Yang
>> Help you, Help me

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-08-08  2:38         ` Wei Yang
@ 2019-08-08  7:06           ` David Hildenbrand
  2019-08-19  2:38             ` Wei Yang
  0 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2019-08-08  7:06 UTC (permalink / raw)
  To: Wei Yang, Zeng, Star; +Cc: imammedo, qemu-devel, mst

On 08.08.19 04:38, Wei Yang wrote:
> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>>> -----Original Message-----
>>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>>> Sent: Thursday, August 8, 2019 10:13 AM
>>> To: Zeng, Star <star.zeng@intel.com>
>>> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
>>> imammedo@redhat.com; david@redhat.com; mst@redhat.com
>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>> use goto for the last check
>>>
>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>>>>> -----Original Message-----
>>>>> From: Qemu-devel [mailto:qemu-devel-
>>>>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
>>>>> To: qemu-devel@nongnu.org
>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>>>> use goto for the last check
>>>>>
>>>>> We are already at the last condition check.
>>>>>
>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  hw/mem/memory-device.c | 1 -
>>>>>  1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>>> index
>>>>> 5f2c408036..df3261b32a 100644
>>>>> --- a/hw/mem/memory-device.c
>>>>> +++ b/hw/mem/memory-device.c
>>>>> @@ -186,7 +186,6 @@ static uint64_t
>>>>> memory_device_get_free_addr(MachineState *ms,
>>>>>      if (!range_contains_range(&as, &new)) {
>>>>>          error_setg(errp, "could not find position in guest address space for "
>>>>>                     "memory device - memory fragmented due to alignments");
>>>>> -        goto out;
>>>>
>>>> Is it better to return 0 (or set new_addr to 0) for this error path and another
>>> remaining "goto out" path?
>>>>
>>>
>>> I may not get your point.
>>>
>>> We set errp which is handled in its caller. By doing so, the error is propagated.
>>>
>>> Do I miss something?
>>
>> Yes, you are right. Currently, the caller is checking errp, but not the returned address, so there should be no issue.
>> But when you see other error paths, you will find they all return 0. To be aligned (return 0 when error), so just suggest also returning 0 for these two "goto out" error path. :)
>>
> 
> You may have some point.
> 
> Let's see whether others have the same taste, or we can refine it separately.
> 

I don't think that's necessary (callers really should check for errors
before using the return values), but I would also not object to that change.

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-08-08  7:06           ` David Hildenbrand
@ 2019-08-19  2:38             ` Wei Yang
  2019-08-19  5:32               ` Zeng, Star
  0 siblings, 1 reply; 15+ messages in thread
From: Wei Yang @ 2019-08-19  2:38 UTC (permalink / raw)
  To: David Hildenbrand; +Cc: imammedo, mst, Wei Yang, Zeng, Star, qemu-devel

On Thu, Aug 08, 2019 at 09:06:21AM +0200, David Hildenbrand wrote:
>On 08.08.19 04:38, Wei Yang wrote:
>> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>>>> -----Original Message-----
>>>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>>>> Sent: Thursday, August 8, 2019 10:13 AM
>>>> To: Zeng, Star <star.zeng@intel.com>
>>>> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
>>>> imammedo@redhat.com; david@redhat.com; mst@redhat.com
>>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>>> use goto for the last check
>>>>
>>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>>>>>> -----Original Message-----
>>>>>> From: Qemu-devel [mailto:qemu-devel-
>>>>>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
>>>>>> To: qemu-devel@nongnu.org
>>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
>>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>>>>> use goto for the last check
>>>>>>
>>>>>> We are already at the last condition check.
>>>>>>
>>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>>> ---
>>>>>>  hw/mem/memory-device.c | 1 -
>>>>>>  1 file changed, 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>>>> index
>>>>>> 5f2c408036..df3261b32a 100644
>>>>>> --- a/hw/mem/memory-device.c
>>>>>> +++ b/hw/mem/memory-device.c
>>>>>> @@ -186,7 +186,6 @@ static uint64_t
>>>>>> memory_device_get_free_addr(MachineState *ms,
>>>>>>      if (!range_contains_range(&as, &new)) {
>>>>>>          error_setg(errp, "could not find position in guest address space for "
>>>>>>                     "memory device - memory fragmented due to alignments");
>>>>>> -        goto out;
>>>>>
>>>>> Is it better to return 0 (or set new_addr to 0) for this error path and another
>>>> remaining "goto out" path?
>>>>>
>>>>
>>>> I may not get your point.
>>>>
>>>> We set errp which is handled in its caller. By doing so, the error is propagated.
>>>>
>>>> Do I miss something?
>>>
>>> Yes, you are right. Currently, the caller is checking errp, but not the returned address, so there should be no issue.
>>> But when you see other error paths, you will find they all return 0. To be aligned (return 0 when error), so just suggest also returning 0 for these two "goto out" error path. :)
>>>
>> 
>> You may have some point.
>> 
>> Let's see whether others have the same taste, or we can refine it separately.
>> 
>
>I don't think that's necessary (callers really should check for errors
>before using the return values), but I would also not object to that change.
>

In case there is no strong requirement to refactor the code. I would leave it
here.

>-- 
>
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-08-19  2:38             ` Wei Yang
@ 2019-08-19  5:32               ` Zeng, Star
  2019-08-19  6:36                 ` Wei Yang
  0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2019-08-19  5:32 UTC (permalink / raw)
  To: Wei Yang, David Hildenbrand; +Cc: imammedo, qemu-devel, Zeng, Star, mst



> -----Original Message-----
> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
> Sent: Monday, August 19, 2019 10:39 AM
> To: David Hildenbrand <david@redhat.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>; Zeng, Star
> <star.zeng@intel.com>; imammedo@redhat.com; qemu-devel@nongnu.org;
> mst@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> use goto for the last check
> 
> On Thu, Aug 08, 2019 at 09:06:21AM +0200, David Hildenbrand wrote:
> >On 08.08.19 04:38, Wei Yang wrote:
> >> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
> >>>> -----Original Message-----
> >>>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
> >>>> Sent: Thursday, August 8, 2019 10:13 AM
> >>>> To: Zeng, Star <star.zeng@intel.com>
> >>>> Cc: Wei Yang <richardw.yang@linux.intel.com>;
> >>>> qemu-devel@nongnu.org; imammedo@redhat.com;
> david@redhat.com;
> >>>> mst@redhat.com
> >>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not
> >>>> necessary to use goto for the last check
> >>>>
> >>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Qemu-devel [mailto:qemu-devel-
> >>>>>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
> >>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
> >>>>>> To: qemu-devel@nongnu.org
> >>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
> >>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
> >>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not
> necessary
> >>>>>> to use goto for the last check
> >>>>>>
> >>>>>> We are already at the last condition check.
> >>>>>>
> >>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
> >>>>>> ---
> >>>>>>  hw/mem/memory-device.c | 1 -
> >>>>>>  1 file changed, 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-
> device.c
> >>>> index
> >>>>>> 5f2c408036..df3261b32a 100644
> >>>>>> --- a/hw/mem/memory-device.c
> >>>>>> +++ b/hw/mem/memory-device.c
> >>>>>> @@ -186,7 +186,6 @@ static uint64_t
> >>>>>> memory_device_get_free_addr(MachineState *ms,
> >>>>>>      if (!range_contains_range(&as, &new)) {
> >>>>>>          error_setg(errp, "could not find position in guest address space
> for "
> >>>>>>                     "memory device - memory fragmented due to
> alignments");
> >>>>>> -        goto out;
> >>>>>
> >>>>> Is it better to return 0 (or set new_addr to 0) for this error
> >>>>> path and another
> >>>> remaining "goto out" path?
> >>>>>
> >>>>
> >>>> I may not get your point.
> >>>>
> >>>> We set errp which is handled in its caller. By doing so, the error is
> propagated.
> >>>>
> >>>> Do I miss something?
> >>>
> >>> Yes, you are right. Currently, the caller is checking errp, but not the
> returned address, so there should be no issue.
> >>> But when you see other error paths, you will find they all return 0.
> >>> To be aligned (return 0 when error), so just suggest also returning
> >>> 0 for these two "goto out" error path. :)
> >>>
> >>
> >> You may have some point.
> >>
> >> Let's see whether others have the same taste, or we can refine it
> separately.
> >>
> >
> >I don't think that's necessary (callers really should check for errors
> >before using the return values), but I would also not object to that change.
> >
> 
> In case there is no strong requirement to refactor the code. I would leave it
> here.

It was just my suggestion. I am fine with any preference you and other experts have.

Thanks,
Star

> 
> >--
> >
> >Thanks,
> >
> >David / dhildenb
> 
> --
> Wei Yang
> Help you, Help me


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
  2019-08-19  5:32               ` Zeng, Star
@ 2019-08-19  6:36                 ` Wei Yang
  0 siblings, 0 replies; 15+ messages in thread
From: Wei Yang @ 2019-08-19  6:36 UTC (permalink / raw)
  To: Zeng, Star; +Cc: qemu-devel, imammedo, mst, Wei Yang, David Hildenbrand

On Mon, Aug 19, 2019 at 05:32:14AM +0000, Zeng, Star wrote:
>
>
>> -----Original Message-----
>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>> Sent: Monday, August 19, 2019 10:39 AM
>> To: David Hildenbrand <david@redhat.com>
>> Cc: Wei Yang <richardw.yang@linux.intel.com>; Zeng, Star
>> <star.zeng@intel.com>; imammedo@redhat.com; qemu-devel@nongnu.org;
>> mst@redhat.com
>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>> use goto for the last check
>> 
>> On Thu, Aug 08, 2019 at 09:06:21AM +0200, David Hildenbrand wrote:
>> >On 08.08.19 04:38, Wei Yang wrote:
>> >> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>> >>>> -----Original Message-----
>> >>>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>> >>>> Sent: Thursday, August 8, 2019 10:13 AM
>> >>>> To: Zeng, Star <star.zeng@intel.com>
>> >>>> Cc: Wei Yang <richardw.yang@linux.intel.com>;
>> >>>> qemu-devel@nongnu.org; imammedo@redhat.com;
>> david@redhat.com;
>> >>>> mst@redhat.com
>> >>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not
>> >>>> necessary to use goto for the last check
>> >>>>
>> >>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>> >>>>>> -----Original Message-----
>> >>>>>> From: Qemu-devel [mailto:qemu-devel-
>> >>>>>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>> >>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
>> >>>>>> To: qemu-devel@nongnu.org
>> >>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>> >>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
>> >>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not
>> necessary
>> >>>>>> to use goto for the last check
>> >>>>>>
>> >>>>>> We are already at the last condition check.
>> >>>>>>
>> >>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> >>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> >>>>>> ---
>> >>>>>>  hw/mem/memory-device.c | 1 -
>> >>>>>>  1 file changed, 1 deletion(-)
>> >>>>>>
>> >>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-
>> device.c
>> >>>> index
>> >>>>>> 5f2c408036..df3261b32a 100644
>> >>>>>> --- a/hw/mem/memory-device.c
>> >>>>>> +++ b/hw/mem/memory-device.c
>> >>>>>> @@ -186,7 +186,6 @@ static uint64_t
>> >>>>>> memory_device_get_free_addr(MachineState *ms,
>> >>>>>>      if (!range_contains_range(&as, &new)) {
>> >>>>>>          error_setg(errp, "could not find position in guest address space
>> for "
>> >>>>>>                     "memory device - memory fragmented due to
>> alignments");
>> >>>>>> -        goto out;
>> >>>>>
>> >>>>> Is it better to return 0 (or set new_addr to 0) for this error
>> >>>>> path and another
>> >>>> remaining "goto out" path?
>> >>>>>
>> >>>>
>> >>>> I may not get your point.
>> >>>>
>> >>>> We set errp which is handled in its caller. By doing so, the error is
>> propagated.
>> >>>>
>> >>>> Do I miss something?
>> >>>
>> >>> Yes, you are right. Currently, the caller is checking errp, but not the
>> returned address, so there should be no issue.
>> >>> But when you see other error paths, you will find they all return 0.
>> >>> To be aligned (return 0 when error), so just suggest also returning
>> >>> 0 for these two "goto out" error path. :)
>> >>>
>> >>
>> >> You may have some point.
>> >>
>> >> Let's see whether others have the same taste, or we can refine it
>> separately.
>> >>
>> >
>> >I don't think that's necessary (callers really should check for errors
>> >before using the return values), but I would also not object to that change.
>> >
>> 
>> In case there is no strong requirement to refactor the code. I would leave it
>> here.
>
>It was just my suggestion. I am fine with any preference you and other experts have.
>

Thanks

>Thanks,
>Star
>
>> 
>> >--
>> >
>> >Thanks,
>> >
>> >David / dhildenb
>> 
>> --
>> Wei Yang
>> Help you, Help me

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr
  2019-07-30  0:37 [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr Wei Yang
  2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check Wei Yang
  2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range Wei Yang
@ 2019-09-13 23:47 ` Wei Yang
  2019-09-14 19:40   ` Michael S. Tsirkin
  2 siblings, 1 reply; 15+ messages in thread
From: Wei Yang @ 2019-09-13 23:47 UTC (permalink / raw)
  To: Wei Yang; +Cc: imammedo, mst, qemu-devel, david

On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote:
>When we iterate the memory-device list to get the available range, it is not
>necessary to iterate the whole list.
>
>1) no more overlap for hinted range if tmp exceed it
>
>v2:
>   * remove #2 as suggested by Igor and David
>   * add some comment to inform address assignment stay the same as before
>     this change 
>
>Wei Yang (2):
>  memory-device: not necessary to use goto for the last check
>  memory-device: break the loop if tmp exceed the hinted range
>
> hw/mem/memory-device.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>

Would someone take this patch set?

>-- 
>2.17.1
>

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr
  2019-09-13 23:47 ` [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr Wei Yang
@ 2019-09-14 19:40   ` Michael S. Tsirkin
  0 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2019-09-14 19:40 UTC (permalink / raw)
  To: Wei Yang; +Cc: imammedo, david, Wei Yang, ehabkost, qemu-devel

On Fri, Sep 13, 2019 at 11:47:46PM +0000, Wei Yang wrote:
> On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote:
> >When we iterate the memory-device list to get the available range, it is not
> >necessary to iterate the whole list.
> >
> >1) no more overlap for hinted range if tmp exceed it
> >
> >v2:
> >   * remove #2 as suggested by Igor and David
> >   * add some comment to inform address assignment stay the same as before
> >     this change 
> >
> >Wei Yang (2):
> >  memory-device: not necessary to use goto for the last check
> >  memory-device: break the loop if tmp exceed the hinted range
> >
> > hw/mem/memory-device.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> 
> Would someone take this patch set?

yes looks good to me too.
Eduardo?

> >-- 
> >2.17.1
> >
> 
> -- 
> Wei Yang
> Help you, Help me


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30  0:37 [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr Wei Yang
2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check Wei Yang
2019-08-08  1:42   ` Zeng, Star
2019-08-08  2:13     ` Wei Yang
2019-08-08  2:30       ` Zeng, Star
2019-08-08  2:38         ` Wei Yang
2019-08-08  7:06           ` David Hildenbrand
2019-08-19  2:38             ` Wei Yang
2019-08-19  5:32               ` Zeng, Star
2019-08-19  6:36                 ` Wei Yang
2019-07-30  0:37 ` [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range Wei Yang
2019-07-30  7:38   ` David Hildenbrand
2019-07-30  9:30   ` Igor Mammedov
2019-09-13 23:47 ` [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr Wei Yang
2019-09-14 19:40   ` Michael S. Tsirkin

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox