Simplifies the code and improves code generation a bit:

   text	   data	    bss	    dec	    hex	filename
  14769	   1017	   4112	  19898	   4dba	alternative.o.before
  14742	   1017	   4112	  19871	   4d9f	alternative.o.after

Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/alternative.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 1df8fac6740d..5293929488f0 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -2750,6 +2750,9 @@ static void smp_text_poke_batch_process(void)
 		if (unlikely(!atomic_dec_and_test(refs)))
 			atomic_cond_read_acquire(refs, !VAL);
 	}
+
+	/* They are all completed: */
+	text_poke_array.nr_entries = 0;
 }
 
 static void __smp_text_poke_batch_add(void *addr, const void *opcode, size_t len, const void *emulate)
@@ -2857,20 +2860,16 @@ static bool text_poke_addr_ordered(void *addr)
 
 void smp_text_poke_batch_finish(void)
 {
-	if (text_poke_array.nr_entries) {
+	if (text_poke_array.nr_entries)
 		smp_text_poke_batch_process();
-		text_poke_array.nr_entries = 0;
-	}
 }
 
 static void smp_text_poke_batch_flush(void *addr)
 {
 	lockdep_assert_held(&text_mutex);
 
-	if (text_poke_array.nr_entries == TP_ARRAY_NR_ENTRIES_MAX || !text_poke_addr_ordered(addr)) {
+	if (text_poke_array.nr_entries == TP_ARRAY_NR_ENTRIES_MAX || !text_poke_addr_ordered(addr))
 		smp_text_poke_batch_process();
-		text_poke_array.nr_entries = 0;
-	}
 }
 
 /**
-- 
2.45.2

On 28.03.25 г. 15:26 ч., Ingo Molnar wrote:
> Simplifies the code and improves code generation a bit:
> 
>     text	   data	    bss	    dec	    hex	filename
>    14769	   1017	   4112	  19898	   4dba	alternative.o.before
>    14742	   1017	   4112	  19871	   4d9f	alternative.o.after
> 
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>   arch/x86/kernel/alternative.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 1df8fac6740d..5293929488f0 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -2750,6 +2750,9 @@ static void smp_text_poke_batch_process(void)
>   		if (unlikely(!atomic_dec_and_test(refs)))
>   			atomic_cond_read_acquire(refs, !VAL);
>   	}
> +
> +	/* They are all completed: */
> +	text_poke_array.nr_entries = 0;
>   }
>   
>   static void __smp_text_poke_batch_add(void *addr, const void *opcode, size_t len, const void *emulate)
> @@ -2857,20 +2860,16 @@ static bool text_poke_addr_ordered(void *addr)
>   
>   void smp_text_poke_batch_finish(void)
>   {
> -	if (text_poke_array.nr_entries) {
> +	if (text_poke_array.nr_entries)
>   		smp_text_poke_batch_process();
> -		text_poke_array.nr_entries = 0;
> -	}
>   }

This function becomes trivial, why not simply move the check into 
smp_text_poke_batch_process and rename it to smp_text_poke_batch_finish ?

>   
>   static void smp_text_poke_batch_flush(void *addr)
>   {
>   	lockdep_assert_held(&text_mutex);
>   
> -	if (text_poke_array.nr_entries == TP_ARRAY_NR_ENTRIES_MAX || !text_poke_addr_ordered(addr)) {
> +	if (text_poke_array.nr_entries == TP_ARRAY_NR_ENTRIES_MAX || !text_poke_addr_ordered(addr))
>   		smp_text_poke_batch_process();
> -		text_poke_array.nr_entries = 0;
> -	}
>   }
>   
>   /**

* Nikolay Borisov <nik.borisov@suse.com> wrote:

> 
> 
> On 28.03.25 г. 15:26 ч., Ingo Molnar wrote:
> > Simplifies the code and improves code generation a bit:
> > 
> >     text	   data	    bss	    dec	    hex	filename
> >    14769	   1017	   4112	  19898	   4dba	alternative.o.before
> >    14742	   1017	   4112	  19871	   4d9f	alternative.o.after
> > 
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > ---
> >   arch/x86/kernel/alternative.c | 11 +++++------
> >   1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 1df8fac6740d..5293929488f0 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -2750,6 +2750,9 @@ static void smp_text_poke_batch_process(void)
> >   		if (unlikely(!atomic_dec_and_test(refs)))
> >   			atomic_cond_read_acquire(refs, !VAL);
> >   	}
> > +
> > +	/* They are all completed: */
> > +	text_poke_array.nr_entries = 0;
> >   }
> >   static void __smp_text_poke_batch_add(void *addr, const void *opcode, size_t len, const void *emulate)
> > @@ -2857,20 +2860,16 @@ static bool text_poke_addr_ordered(void *addr)
> >   void smp_text_poke_batch_finish(void)
> >   {
> > -	if (text_poke_array.nr_entries) {
> > +	if (text_poke_array.nr_entries)
> >   		smp_text_poke_batch_process();
> > -		text_poke_array.nr_entries = 0;
> > -	}
> >   }
> 
> This function becomes trivial, why not simply move the check into
> smp_text_poke_batch_process and rename it to smp_text_poke_batch_finish ?

Yeah, that's pretty much what happens in patch #47:

  x86/alternatives: Remove 'smp_text_poke_batch_flush()'

Thanks,

	Ingo

On 3.04.25 г. 16:38 ч., Ingo Molnar wrote:
> 
> * Nikolay Borisov <nik.borisov@suse.com> wrote:
> 
>>
>>
>> On 28.03.25 г. 15:26 ч., Ingo Molnar wrote:
>>> Simplifies the code and improves code generation a bit:
>>>
>>>      text	   data	    bss	    dec	    hex	filename
>>>     14769	   1017	   4112	  19898	   4dba	alternative.o.before
>>>     14742	   1017	   4112	  19871	   4d9f	alternative.o.after
>>>
>>> Signed-off-by: Ingo Molnar <mingo@kernel.org>
>>> ---
>>>    arch/x86/kernel/alternative.c | 11 +++++------
>>>    1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>>> index 1df8fac6740d..5293929488f0 100644
>>> --- a/arch/x86/kernel/alternative.c
>>> +++ b/arch/x86/kernel/alternative.c
>>> @@ -2750,6 +2750,9 @@ static void smp_text_poke_batch_process(void)
>>>    		if (unlikely(!atomic_dec_and_test(refs)))
>>>    			atomic_cond_read_acquire(refs, !VAL);
>>>    	}
>>> +
>>> +	/* They are all completed: */
>>> +	text_poke_array.nr_entries = 0;
>>>    }
>>>    static void __smp_text_poke_batch_add(void *addr, const void *opcode, size_t len, const void *emulate)
>>> @@ -2857,20 +2860,16 @@ static bool text_poke_addr_ordered(void *addr)
>>>    void smp_text_poke_batch_finish(void)
>>>    {
>>> -	if (text_poke_array.nr_entries) {
>>> +	if (text_poke_array.nr_entries)
>>>    		smp_text_poke_batch_process();
>>> -		text_poke_array.nr_entries = 0;
>>> -	}
>>>    }
>>
>> This function becomes trivial, why not simply move the check into
>> smp_text_poke_batch_process and rename it to smp_text_poke_batch_finish ?
> 
> Yeah, that's pretty much what happens in patch #47:
> 
>    x86/alternatives: Remove 'smp_text_poke_batch_flush()'

Well, that patch removes poke_batch_flush but still retains 
poke_batch_finish + poke_batch_process. My suggestion is to eliminate 
poke_batch_process name and turn it into poke_batch_finish by moving the 
check from poke_batch_finish into poke_batch_process.

> 
> Thanks,
> 
> 	Ingo

* Nikolay Borisov <nik.borisov@suse.com> wrote:

> 
> 
> On 3.04.25 г. 16:38 ч., Ingo Molnar wrote:
> > 
> > * Nikolay Borisov <nik.borisov@suse.com> wrote:
> > 
> > > 
> > > 
> > > On 28.03.25 г. 15:26 ч., Ingo Molnar wrote:
> > > > Simplifies the code and improves code generation a bit:
> > > > 
> > > >      text	   data	    bss	    dec	    hex	filename
> > > >     14769	   1017	   4112	  19898	   4dba	alternative.o.before
> > > >     14742	   1017	   4112	  19871	   4d9f	alternative.o.after
> > > > 
> > > > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > > > ---
> > > >    arch/x86/kernel/alternative.c | 11 +++++------
> > > >    1 file changed, 5 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > > > index 1df8fac6740d..5293929488f0 100644
> > > > --- a/arch/x86/kernel/alternative.c
> > > > +++ b/arch/x86/kernel/alternative.c
> > > > @@ -2750,6 +2750,9 @@ static void smp_text_poke_batch_process(void)
> > > >    		if (unlikely(!atomic_dec_and_test(refs)))
> > > >    			atomic_cond_read_acquire(refs, !VAL);
> > > >    	}
> > > > +
> > > > +	/* They are all completed: */
> > > > +	text_poke_array.nr_entries = 0;
> > > >    }
> > > >    static void __smp_text_poke_batch_add(void *addr, const void *opcode, size_t len, const void *emulate)
> > > > @@ -2857,20 +2860,16 @@ static bool text_poke_addr_ordered(void *addr)
> > > >    void smp_text_poke_batch_finish(void)
> > > >    {
> > > > -	if (text_poke_array.nr_entries) {
> > > > +	if (text_poke_array.nr_entries)
> > > >    		smp_text_poke_batch_process();
> > > > -		text_poke_array.nr_entries = 0;
> > > > -	}
> > > >    }
> > > 
> > > This function becomes trivial, why not simply move the check into
> > > smp_text_poke_batch_process and rename it to smp_text_poke_batch_finish ?
> > 
> > Yeah, that's pretty much what happens in patch #47:
> > 
> >    x86/alternatives: Remove 'smp_text_poke_batch_flush()'
> 
> Well, that patch removes poke_batch_flush but still retains
> poke_batch_finish + poke_batch_process. My suggestion is to eliminate
> poke_batch_process name and turn it into poke_batch_finish by moving the
> check from poke_batch_finish into poke_batch_process.

So, in the context of the full tree at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mingo/tip.git WIP.x86/alternatives

Standalone smp_text_poke_batch_process() is still needed, because 
smp_text_poke_batch_add() uses it to drive the whole 'batching' 
machinery.

If smp_text_poke_batch_process() finishes for each call, if I 
understand your suggestion correctly, that reduces the amount of 
batching done, which is a disadvantage.

Note how right now it's possible to do something like this:

	smp_text_poke_batch_add(1);
	smp_text_poke_batch_add(1);
	smp_text_poke_batch_add(1);
	smp_text_poke_batch_add(1);
	smp_text_poke_batch_finish();

This results in a single call to smp_text_poke_batch_process(), with 4 
entries, a single 4-entry flush in essence.

Thanks,

	Ingo

On 3.04.25 г. 17:13 ч., Ingo Molnar wrote:
> 
> * Nikolay Borisov <nik.borisov@suse.com> wrote:
> 
>>
>>
>> On 3.04.25 г. 16:38 ч., Ingo Molnar wrote:
>>>
>>> * Nikolay Borisov <nik.borisov@suse.com> wrote:
>>>
>>>>
>>>>
>>>> On 28.03.25 г. 15:26 ч., Ingo Molnar wrote:
>>>>> Simplifies the code and improves code generation a bit:
>>>>>
>>>>>       text	   data	    bss	    dec	    hex	filename
>>>>>      14769	   1017	   4112	  19898	   4dba	alternative.o.before
>>>>>      14742	   1017	   4112	  19871	   4d9f	alternative.o.after
>>>>>
>>>>> Signed-off-by: Ingo Molnar <mingo@kernel.org>
>>>>> ---
>>>>>     arch/x86/kernel/alternative.c | 11 +++++------
>>>>>     1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>>>>> index 1df8fac6740d..5293929488f0 100644
>>>>> --- a/arch/x86/kernel/alternative.c
>>>>> +++ b/arch/x86/kernel/alternative.c
>>>>> @@ -2750,6 +2750,9 @@ static void smp_text_poke_batch_process(void)
>>>>>     		if (unlikely(!atomic_dec_and_test(refs)))
>>>>>     			atomic_cond_read_acquire(refs, !VAL);
>>>>>     	}
>>>>> +
>>>>> +	/* They are all completed: */
>>>>> +	text_poke_array.nr_entries = 0;
>>>>>     }
>>>>>     static void __smp_text_poke_batch_add(void *addr, const void *opcode, size_t len, const void *emulate)
>>>>> @@ -2857,20 +2860,16 @@ static bool text_poke_addr_ordered(void *addr)
>>>>>     void smp_text_poke_batch_finish(void)
>>>>>     {
>>>>> -	if (text_poke_array.nr_entries) {
>>>>> +	if (text_poke_array.nr_entries)
>>>>>     		smp_text_poke_batch_process();
>>>>> -		text_poke_array.nr_entries = 0;
>>>>> -	}
>>>>>     }
>>>>
>>>> This function becomes trivial, why not simply move the check into
>>>> smp_text_poke_batch_process and rename it to smp_text_poke_batch_finish ?
>>>
>>> Yeah, that's pretty much what happens in patch #47:
>>>
>>>     x86/alternatives: Remove 'smp_text_poke_batch_flush()'
>>
>> Well, that patch removes poke_batch_flush but still retains
>> poke_batch_finish + poke_batch_process. My suggestion is to eliminate
>> poke_batch_process name and turn it into poke_batch_finish by moving the
>> check from poke_batch_finish into poke_batch_process.
> 
> So, in the context of the full tree at:
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/mingo/tip.git WIP.x86/alternatives
> 
> Standalone smp_text_poke_batch_process() is still needed, because
> smp_text_poke_batch_add() uses it to drive the whole 'batching'
> machinery.
> 
> If smp_text_poke_batch_process() finishes for each call, if I
> understand your suggestion correctly, that reduces the amount of
> batching done, which is a disadvantage.
> 
> Note how right now it's possible to do something like this:
> 
> 	smp_text_poke_batch_add(1);
> 	smp_text_poke_batch_add(1);
> 	smp_text_poke_batch_add(1);
> 	smp_text_poke_batch_add(1);
> 	smp_text_poke_batch_finish();
> 
> This results in a single call to smp_text_poke_batch_process(), with 4
> entries, a single 4-entry flush in essence.

I meant doing this:

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 5b1a6252a4b9..b6a781b9de26 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -2587,12 +2587,16 @@ noinstr int 
smp_text_poke_int3_trap_handler(struct pt_regs *regs)
   *               replacing opcode
   *     - SMP sync all CPUs
   */
-static void smp_text_poke_batch_process(void)
+void smp_text_poke_batch_finish(void)
  {
         unsigned char int3 = INT3_INSN_OPCODE;
         unsigned int i;
         int do_sync;

+
+       if (!text_poke_array.nr_entries)
+               return;
+
         lockdep_assert_held(&text_mutex);

         /*
@@ -2832,12 +2836,6 @@ static bool text_poke_addr_ordered(void *addr)
         return true;
  }

-void smp_text_poke_batch_finish(void)
-{
-       if (text_poke_array.nr_entries)
-               smp_text_poke_batch_process();
-}
-
  /**
   * smp_text_poke_batch_add() -- update instruction on live kernel on 
SMP, batched
   * @addr:      address to patch
@@ -2854,7 +2852,7 @@ void smp_text_poke_batch_finish(void)
  void __ref smp_text_poke_batch_add(void *addr, const void *opcode, 
size_t len, const void *emulate)
  {
         if (text_poke_array.nr_entries == TEXT_POKE_ARRAY_MAX || 
!text_poke_addr_ordered(addr))
-               smp_text_poke_batch_process();
+               smp_text_poke_batch_finish();
         __smp_text_poke_batch_add(addr, opcode, len, emulate);
  }


AFAICS this doesn't change the semantics. I.e smp_text_poke_batch_add 
will call poke_batch_finish iff the address to be added violates the 
sorted order of text_poke_array. The net effect is we have 1 less 
function name to care about.

> 
> Thanks,
> 
> 	Ingo

* Nikolay Borisov <nik.borisov@suse.com> wrote:

> I meant doing this:
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 5b1a6252a4b9..b6a781b9de26 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -2587,12 +2587,16 @@ noinstr int smp_text_poke_int3_trap_handler(struct
> pt_regs *regs)
>   *               replacing opcode
>   *     - SMP sync all CPUs
>   */
> -static void smp_text_poke_batch_process(void)
> +void smp_text_poke_batch_finish(void)
>  {
>         unsigned char int3 = INT3_INSN_OPCODE;
>         unsigned int i;
>         int do_sync;
> 
> +
> +       if (!text_poke_array.nr_entries)
> +               return;

> -               smp_text_poke_batch_process();
> +               smp_text_poke_batch_finish();

I suppose we could do this - it adds one more check to 
smp_text_poke_batch_add() though.

> AFAICS this doesn't change the semantics. I.e smp_text_poke_batch_add 
> will call poke_batch_finish iff the address to be added violates the 
> sorted order of text_poke_array. The net effect is we have 1 less 
> function name to care about.

Yeah, it doesn't change semantics, but it's a very small 
deoptimization.

Mind sending a patch? It does simplify the facility some more and that 
single branch will wash away against costs like the CR3 flushes done 
...

Thanks,

	Ingo

On 3.04.25 г. 18:29 ч., Ingo Molnar wrote:
> 
> * Nikolay Borisov <nik.borisov@suse.com> wrote:
> 
>> I meant doing this:
>>
>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>> index 5b1a6252a4b9..b6a781b9de26 100644
>> --- a/arch/x86/kernel/alternative.c
>> +++ b/arch/x86/kernel/alternative.c
>> @@ -2587,12 +2587,16 @@ noinstr int smp_text_poke_int3_trap_handler(struct
>> pt_regs *regs)
>>    *               replacing opcode
>>    *     - SMP sync all CPUs
>>    */
>> -static void smp_text_poke_batch_process(void)
>> +void smp_text_poke_batch_finish(void)
>>   {
>>          unsigned char int3 = INT3_INSN_OPCODE;
>>          unsigned int i;
>>          int do_sync;
>>
>> +
>> +       if (!text_poke_array.nr_entries)
>> +               return;
> 
>> -               smp_text_poke_batch_process();
>> +               smp_text_poke_batch_finish();
> 
> I suppose we could do this - it adds one more check to
> smp_text_poke_batch_add() though.

poke_batch_finish you meant?

> 
>> AFAICS this doesn't change the semantics. I.e smp_text_poke_batch_add
>> will call poke_batch_finish iff the address to be added violates the
>> sorted order of text_poke_array. The net effect is we have 1 less
>> function name to care about.
> 
> Yeah, it doesn't change semantics, but it's a very small
> deoptimization.
 > > Mind sending a patch? It does simplify the facility some more and that
> single branch will wash away against costs like the CR3 flushes done

Given that poke_batch_finish does a cond_resched and sync_each_cpu which 
is an IPI can it even be considered a performance critical path ?

> ...
> 
> Thanks,
> 
> 	Ingo

* Nikolay Borisov <nik.borisov@suse.com> wrote:

> > Yeah, it doesn't change semantics, but it's a very small
> > deoptimization.
> > > Mind sending a patch? It does simplify the facility some more and that
> > single branch will wash away against costs like the CR3 flushes done
> 
> Given that poke_batch_finish does a cond_resched and sync_each_cpu 
> which is an IPI can it even be considered a performance critical path 
> ?

Probably not, but even if it was, I think your change would still be an 
overall win, so please send a changelogged patch against 
WIP.x86/alternatives.

Thanks,

	Ingo