[PATCH] lib: drop size parameter from sort()'s swap callback

Jan Beulich posted 1 patch 3 months ago
Failed in applying to current master (apply log)
[PATCH] lib: drop size parameter from sort()'s swap callback
Posted by Jan Beulich 3 months ago
This was needed only for generic_swap(), which disappeared in
8cb0341a61fa ("xen/sort: Switch to an extern inline implementation").

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -100,7 +100,7 @@ static int cmp_mmio_handler(const void *
     return 0;
 }
 
-static void swap_mmio_handler(void *_a, void *_b, size_t size)
+static void swap_mmio_handler(void *_a, void *_b)
 {
     struct mmio_handler *a = _a, *b = _b;
 
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -39,7 +39,7 @@ static int init_or_livepatch cf_check cm
 	return 0;
 }
 
-static void init_or_livepatch cf_check swap_ex(void *a, void *b, size_t size)
+static void init_or_livepatch cf_check swap_ex(void *a, void *b)
 {
 	struct exception_table_entry *l = a, *r = b, tmp;
 	long delta = b - a;
--- a/xen/common/device-tree/bootinfo-fdt.c
+++ b/xen/common/device-tree/bootinfo-fdt.c
@@ -449,7 +449,7 @@ static int __init cmp_memory_node(const
     return 0;
 }
 
-static void __init swap_memory_node(void *_a, void *_b, size_t size)
+static void __init swap_memory_node(void *_a, void *_b)
 {
     struct membank *a = _a, *b = _b;
 
--- a/xen/include/xen/sort.h
+++ b/xen/include/xen/sort.h
@@ -24,7 +24,7 @@ extern gnu_inline
 #endif
 void sort(void *base, size_t num, size_t size,
           int (*cmp)(const void *a, const void *b),
-          void (*swap)(void *a, void *b, size_t size))
+          void (*swap)(void *a, void *b))
 {
     /* pre-scale counters for performance */
     size_t i = (num / 2) * size, n = num * size, c, r;
@@ -39,7 +39,7 @@ void sort(void *base, size_t num, size_t
                 c += size;
             if ( cmp(base + r, base + c) >= 0 )
                 break;
-            swap(base + r, base + c, size);
+            swap(base + r, base + c);
         }
     }
 
@@ -47,7 +47,7 @@ void sort(void *base, size_t num, size_t
     for ( i = n; i > 0; )
     {
         i -= size;
-        swap(base, base + i, size);
+        swap(base, base + i);
         for ( r = 0; r * 2 + size < i; r = c )
         {
             c = r * 2 + size;
@@ -55,7 +55,7 @@ void sort(void *base, size_t num, size_t
                 c += size;
             if ( cmp(base + r, base + c) >= 0 )
                 break;
-            swap(base + r, base + c, size);
+            swap(base + r, base + c);
         }
     }
 }
Re: [PATCH] lib: drop size parameter from sort()'s swap callback
Posted by Bertrand Marquis 3 months ago
Hi,

> On 29 Jul 2025, at 16:26, Jan Beulich <jbeulich@suse.com> wrote:
> 
> This was needed only for generic_swap(), which disappeared in
> 8cb0341a61fa ("xen/sort: Switch to an extern inline implementation").
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For arm part:

Acked-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -100,7 +100,7 @@ static int cmp_mmio_handler(const void *
>     return 0;
> }
> 
> -static void swap_mmio_handler(void *_a, void *_b, size_t size)
> +static void swap_mmio_handler(void *_a, void *_b)
> {
>     struct mmio_handler *a = _a, *b = _b;
> 
> --- a/xen/arch/x86/extable.c
> +++ b/xen/arch/x86/extable.c
> @@ -39,7 +39,7 @@ static int init_or_livepatch cf_check cm
> return 0;
> }
> 
> -static void init_or_livepatch cf_check swap_ex(void *a, void *b, size_t size)
> +static void init_or_livepatch cf_check swap_ex(void *a, void *b)
> {
> struct exception_table_entry *l = a, *r = b, tmp;
> long delta = b - a;
> --- a/xen/common/device-tree/bootinfo-fdt.c
> +++ b/xen/common/device-tree/bootinfo-fdt.c
> @@ -449,7 +449,7 @@ static int __init cmp_memory_node(const
>     return 0;
> }
> 
> -static void __init swap_memory_node(void *_a, void *_b, size_t size)
> +static void __init swap_memory_node(void *_a, void *_b)
> {
>     struct membank *a = _a, *b = _b;
> 
> --- a/xen/include/xen/sort.h
> +++ b/xen/include/xen/sort.h
> @@ -24,7 +24,7 @@ extern gnu_inline
> #endif
> void sort(void *base, size_t num, size_t size,
>           int (*cmp)(const void *a, const void *b),
> -          void (*swap)(void *a, void *b, size_t size))
> +          void (*swap)(void *a, void *b))
> {
>     /* pre-scale counters for performance */
>     size_t i = (num / 2) * size, n = num * size, c, r;
> @@ -39,7 +39,7 @@ void sort(void *base, size_t num, size_t
>                 c += size;
>             if ( cmp(base + r, base + c) >= 0 )
>                 break;
> -            swap(base + r, base + c, size);
> +            swap(base + r, base + c);
>         }
>     }
> 
> @@ -47,7 +47,7 @@ void sort(void *base, size_t num, size_t
>     for ( i = n; i > 0; )
>     {
>         i -= size;
> -        swap(base, base + i, size);
> +        swap(base, base + i);
>         for ( r = 0; r * 2 + size < i; r = c )
>         {
>             c = r * 2 + size;
> @@ -55,7 +55,7 @@ void sort(void *base, size_t num, size_t
>                 c += size;
>             if ( cmp(base + r, base + c) >= 0 )
>                 break;
> -            swap(base + r, base + c, size);
> +            swap(base + r, base + c);
>         }
>     }
> }
Re: [PATCH] lib: drop size parameter from sort()'s swap callback
Posted by Andrew Cooper 3 months ago
On 29/07/2025 3:26 pm, Jan Beulich wrote:
> This was needed only for generic_swap(), which disappeared in
> 8cb0341a61fa ("xen/sort: Switch to an extern inline implementation").
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Oh, nice.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'd expect there to be no change in generated code here, as everything
gets inlined.

~Andrew
Re: [PATCH] lib: drop size parameter from sort()'s swap callback
Posted by Jan Beulich 3 months ago
On 29.07.2025 16:29, Andrew Cooper wrote:
> On 29/07/2025 3:26 pm, Jan Beulich wrote:
>> This was needed only for generic_swap(), which disappeared in
>> 8cb0341a61fa ("xen/sort: Switch to an extern inline implementation").
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Oh, nice.
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> I'd expect there to be no change in generated code here, as everything
> gets inlined.

Not really, no. With the change in place, both gcc7 and gcc14 consider the
inlining of swap_ex() (in x86'es extable.c) as less beneficial, and hence
(like cmp_ex()) an out-of-line function appears, while overall code size
reduces. I expect that's because inlining decisions are taken based on
some intermediate internal representation rather than based on the code
that would ultimately be generated. And that intermediate internal
representation now changes, resulting in less of a win by doing the
inlining.

Jan
Re: [PATCH] lib: drop size parameter from sort()'s swap callback
Posted by Andrew Cooper 3 months ago
On 29/07/2025 3:44 pm, Jan Beulich wrote:
> On 29.07.2025 16:29, Andrew Cooper wrote:
>> On 29/07/2025 3:26 pm, Jan Beulich wrote:
>>> This was needed only for generic_swap(), which disappeared in
>>> 8cb0341a61fa ("xen/sort: Switch to an extern inline implementation").
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Oh, nice.
>>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks.
>
>> I'd expect there to be no change in generated code here, as everything
>> gets inlined.
> Not really, no. With the change in place, both gcc7 and gcc14 consider the
> inlining of swap_ex() (in x86'es extable.c) as less beneficial, and hence
> (like cmp_ex()) an out-of-line function appears, while overall code size
> reduces. I expect that's because inlining decisions are taken based on
> some intermediate internal representation rather than based on the code
> that would ultimately be generated. And that intermediate internal
> representation now changes, resulting in less of a win by doing the
> inlining.

Hmm.  We might consider __always_inline, although it seems like gcc12
does decide to inline them with this patch as-is.

Either way, that's something for later.

~Andrew

Re: [PATCH] lib: drop size parameter from sort()'s swap callback
Posted by Jan Beulich 3 months ago
On 30.07.2025 11:29, Andrew Cooper wrote:
> On 29/07/2025 3:44 pm, Jan Beulich wrote:
>> On 29.07.2025 16:29, Andrew Cooper wrote:
>>> On 29/07/2025 3:26 pm, Jan Beulich wrote:
>>>> This was needed only for generic_swap(), which disappeared in
>>>> 8cb0341a61fa ("xen/sort: Switch to an extern inline implementation").
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Oh, nice.
>>>
>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Thanks.
>>
>>> I'd expect there to be no change in generated code here, as everything
>>> gets inlined.
>> Not really, no. With the change in place, both gcc7 and gcc14 consider the
>> inlining of swap_ex() (in x86'es extable.c) as less beneficial, and hence
>> (like cmp_ex()) an out-of-line function appears, while overall code size
>> reduces. I expect that's because inlining decisions are taken based on
>> some intermediate internal representation rather than based on the code
>> that would ultimately be generated. And that intermediate internal
>> representation now changes, resulting in less of a win by doing the
>> inlining.
> 
> Hmm.  We might consider __always_inline,

I would actually expect a compiler to oppose to an always-inline function
to have its address taken.

> although it seems like gcc12
> does decide to inline them with this patch as-is.

Interesting how things can move back and forth between versions ...

Jan