Re: [RFC] x86: Remove CALL_FUNCTION_VECTOR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] x86/xen: memory leak of callfunc_name in xen_smp_intr_init()
- [Low] x86/xen: unused xen_call_function_interrupt() causes build failure
- [Low] x86: dead call_function tracepoints and fred_sysvec_call_function macro
--

commit a87f5828c6202eb98c865ff7619fe6fc331a81ac
Author: Peter Zijlstra <peterz@infradead.org>

x86: Remove CALL_FUNCTION_VECTOR

This commit removes CALL_FUNCTION_VECTOR and routes multi-CPU function call
IPIs through CALL_FUNCTION_SINGLE_VECTOR across native x86 APIC, FRED, IDT,
and Xen SMP paths.

> diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
> index f400cfac69a6b..d429a2720e8e7 100644
> --- a/arch/x86/include/asm/idtentry.h
> +++ b/arch/x86/include/asm/idtentry.h
> @@ -679,7 +679,6 @@ DECLARE_IDTENTRY_SYSVEC(X86_PLATFORM_IPI_VECTOR,	sysvec_x86_platform_ipi);
>  DECLARE_IDTENTRY(RESCHEDULE_VECTOR,			sysvec_reschedule_ipi);
>  DECLARE_IDTENTRY_SYSVEC(REBOOT_VECTOR,			sysvec_reboot);
>  DECLARE_IDTENTRY_SYSVEC(CALL_FUNCTION_SINGLE_VECTOR,	sysvec_call_function_single);
> -DECLARE_IDTENTRY_SYSVEC(CALL_FUNCTION_VECTOR,		sysvec_call_function);
>  #else
>  # define fred_sysvec_reschedule_ipi			NULL
>  # define fred_sysvec_reboot				NULL

[ ... ]

> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 985103cab16c4..c7397816b311a 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -255,15 +255,6 @@ DEFINE_IDTENTRY_SYSVEC_SIMPLE(sysvec_reschedule_ipi)
>  	trace_reschedule_exit(RESCHEDULE_VECTOR);
>  }
>  
> -DEFINE_IDTENTRY_SYSVEC(sysvec_call_function)
> -{
> -	apic_eoi();
> -	trace_call_function_entry(CALL_FUNCTION_VECTOR);
> -	inc_irq_stat(CALL_FUNCTION);
> -	generic_smp_call_function_interrupt();
> -	trace_call_function_exit(CALL_FUNCTION_VECTOR);
> -}
> -
>  DEFINE_IDTENTRY_SYSVEC(sysvec_call_function_single)
>  {
>  	apic_eoi();

[Severity: Low]
Removing sysvec_call_function() in arch/x86/kernel/smp.c drops the only call
sites of trace_call_function_entry() and trace_call_function_exit(), while
DEFINE_IRQ_VECTOR_EVENT(call_function) remains in
arch/x86/include/asm/trace/irq_vectors.h:

    DEFINE_IRQ_VECTOR_EVENT(call_function);

Those dead tracepoints are also still registered in
arch/x86/kernel/trace.c:osnoise_arch_register() and osnoise_arch_unregister():

    ret = register_trace_call_function_entry(trace_intel_irq_entry, NULL);
    if (ret)
            goto out_call_function_single_exit;

    ret = register_trace_call_function_exit(trace_intel_irq_exit, "call_function");

as well as in kernel/trace/rv/monitors/nrp/nrp.c:attach_vector_irq() and
kernel/trace/rv/monitors/sts/sts.c:attach_vector_irq() (and their
corresponding detach_vector_irq() functions):

    rv_attach_trace_probe("nrp", call_function_entry, handle_vector_irq_entry);
    rv_attach_trace_probe("sts", call_function_entry, handle_vector_irq_entry);

In addition, in arch/x86/include/asm/idtentry.h, the !CONFIG_SMP branch still
defines:

    # define fred_sysvec_call_function			NULL

Should the call_function tracepoint definitions, their osnoise and RV monitor
registrations, and fred_sysvec_call_function also be removed so the unused
tracepoints do not remain exported and trigger warnings when building with
UT=1?

> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index 05ee0d3b0874e..d5544026ede5a 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c

[ ... ]

> @@ -80,16 +73,6 @@ int xen_smp_intr_init(unsigned int cpu)
>  	callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
>  	if (!callfunc_name)
>  		goto fail_mem;
> -	per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
> -	rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR,
> -				    cpu,
> -				    xen_call_function_interrupt,
> -				    IRQF_PERCPU|IRQF_NOBALANCING,
> -				    callfunc_name,
> -				    NULL);
> -	if (rc < 0)
> -		goto fail;
> -	per_cpu(xen_callfunc_irq, cpu).irq = rc;

[Severity: Medium]
Does xen_smp_intr_init() in arch/x86/xen/smp.c leak callfunc_name on every CPU
initialization under Xen?

The kasprintf() allocation for "callfunc%d" is left in place:

    callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu);
    if (!callfunc_name)
            goto fail_mem;

Because per_cpu(xen_callfunc_irq, cpu).name = callfunc_name was removed, that
buffer is never saved, so it is leaked if the subsequent VIRQ_DEBUG setup
jumps to fail_mem or fail, and is unconditionally overwritten shortly after by
the "callfuncsingle%d" allocation:

    callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu);
    if (!callfunc_name)
            goto fail_mem;

    per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;

[Severity: Low]
Also in arch/x86/xen/smp.c, removing bind_ipi_to_irqhandler() for
XEN_CALL_FUNCTION_VECTOR in xen_smp_intr_init() leaves
xen_call_function_interrupt() without any callers:

    static irqreturn_t xen_call_function_interrupt(int irq, void *dev_id);
    ...
    static irqreturn_t xen_call_function_interrupt(int irq, void *dev_id)
    {
            generic_smp_call_function_interrupt();
            inc_irq_stat(CALL_FUNCTION);

            return IRQ_HANDLED;
    }

Won't leaving this static function defined in arch/x86/xen/smp.c trigger an
unused-function warning and break the build when CONFIG_XEN=y, CONFIG_SMP=y,
and CONFIG_WERROR=y?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028100930.GP2668564@noisy.programming.kicks-ass.net?part=1