Re: [PATCH 4/5] x86/traps: Drop exception_table[] and use if/else dispatching

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.11.2021 19:21, Andrew Cooper wrote:
> There is also a lot of redundancy in the table.  8 vectors head to do_trap(),
> 3 are handled in the IST logic, and that only leaves 7 others not heading to
> the do_reserved_trap() catch-all.  This also removes the fragility that any
> accidental NULL entry in the table becomes a ticking timebomb.
> 
> Function pointers are expensive under retpoline, and different vectors have
> wildly different frequences.  Drop the indirect call, and use an if/else chain
> instead, which is a code layout technique used by profile-guided optimsiation.
> 
> Using Xen's own perfcounter infrastructure, we see the following frequences of
> vectors measured from boot until I can SSH into dom0 and collect the stats:
> 
>   vec | CFL-R   | Milan   | Notes
>   ----+---------+---------+
>   NMI |     345 |    3768 | Watchdog.  Milan has many more CPUs.
>   ----+---------+---------+
>   #PF | 1233234 | 2006441 |
>   #GP |   90054 |   96193 |
>   #UD |     848 |     851 |
>   #NM |       0 |     132 | Per-vendor lazy vs eager FPU policy.
>   #DB |      67 |      67 | No clue, but it's something in userspace.
> 
> Bloat-o-meter (after some manual insertion of ELF metadata) reports:
> 
>   add/remove: 0/1 grow/shrink: 2/0 up/down: 102/-256 (-154)
>   Function                                     old     new   delta
>   handle_exception_saved                       148     226     +78
>   handle_ist_exception                         453     477     +24
>   exception_table                              256       -    -256
> 
> showing that the if/else chains are less than half the size that
> exception_table[] was in the first place.
> 
> As part of this change, make two other minor changes.  do_reserved_trap() is
> renamed to do_unhandled_trap() because it is the catchall, and already covers
> things that aren't reserved any more (#VE/#VC/#HV/#SX).
> 
> Furthermore, don't forward #TS to guests.  #TS is specifically for errors
> relating to the Task State Segment, which is a Xen-owned structure, not a
> guest-owned structure.  Even in the 32bit days, we never let guests register
> their own Task State Segments.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

As it feels like we won't be coming to an agreement here, despite
my reservations against the specific form of some of this and on
the basis that the code change itself is certainly an improvement:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

I'd nevertheless be curious whether in five or ten years time you'd
still agree with your choice of basing a decision on not really
representative data (at least as far as what the description says).

Jan