[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH 9/9] x86/smp: start APs in parallel during boot
On 12.03.2024 18:13, Krystian Hebel wrote: > > On 8.02.2024 13:37, Jan Beulich wrote: >> On 14.11.2023 18:50, Krystian Hebel wrote: >>> Multiple delays are required when sending IPIs and waiting for >>> responses. During boot, 4 such IPIs were sent per each AP. With this >>> change, only one set of broadcast IPIs is sent. This reduces boot time, >>> especially for platforms with large number of cores. >> Yet APs do their startup work in parallel only for a brief period of >> time, if I'm not mistaken. Othwerwise I can't see why you'd still have >> cpu_up() in __start_xen(). > cpu_up() is left because multiple notifiers aren't easy to convert to work > in parallel. In terms of lines of code it looks like a brief period, but all > the delays along the way were taking much more time than the actual > work. As the gain was already more than what I hoped for, I decided > against spending too much time trying to fix the notifiers' code for > minimal profit. Which is all fine. Just that by title of this patch and the cover letter I expected more. Adding "partly" or some such in both places may help. >>> --- a/xen/arch/x86/smpboot.c >>> +++ b/xen/arch/x86/smpboot.c >>> @@ -425,7 +425,7 @@ void start_secondary(unsigned int cpu) >>> >>> static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip) >>> { >>> - unsigned long send_status = 0, accept_status = 0; >>> + unsigned long send_status = 0, accept_status = 0, sh = 0; >> sh doesn't need to be 64 bits wide, does it? > No, will change. >> >>> int maxlvt, timeout, i; >>> >>> /* >>> @@ -445,6 +445,12 @@ static int wakeup_secondary_cpu(int phys_apicid, >>> unsigned long start_eip) >>> if ( tboot_in_measured_env() && !tboot_wake_ap(phys_apicid, >>> start_eip) ) >>> return 0; >>> >>> + /* >>> + * Use destination shorthand for broadcasting IPIs during boot. >>> + */ >> Nit (style): This is a single line comment. > Ack >> >>> + if ( phys_apicid == BAD_APICID ) >>> + sh = APIC_DEST_ALLBUT; >> I think the latest for this the function parameter wants changing to >> unsigned int (in another prereq patch). > What do you mean, phys_apicid in wakeup_secondary_cpu()? It is passed > as signed int since __cpu_up(), should I change all of those to unsigned? That would be best, yes. BAD_APICID, after all, is an unsigned constant (no matter that its definition involves a unary minus operator). Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |