| Message ID | 20200213125449.14226-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: don't let keyhandlers block indefinitely on locks | expand |
On 13.02.2020 13:54, Juergen Gross wrote: > Most keyhandlers are used to dump hypervisor data to the console and > they are used mostly for debugging purposes. In those cases it might > happen that some data structures are locked and thus are blocking the > handler to access the data. > > In order to be able to still get some information don't use plain > locking functions in the keyhandlers, but a variant of trylocks with > a timeout value. This allows to wait for some time and to give up in > case the lock was not obtained. > > Add the main infrastructure for this feature including a new runtime > parameter allowing to specify the timeout value in milliseconds. > > Use the new locking scheme in the handlers defined in keyhandler.c. Personally I think trylock (as already used in some places) is the way to go. Iirc others disagreed, but also didn't like the approach taken here. I'm not intending to stand in the way if a majority approves of this model, but I'm not going to ack these changes myself. Jan
On 05.03.20 16:25, Jan Beulich wrote: > On 13.02.2020 13:54, Juergen Gross wrote: >> Most keyhandlers are used to dump hypervisor data to the console and >> they are used mostly for debugging purposes. In those cases it might >> happen that some data structures are locked and thus are blocking the >> handler to access the data. >> >> In order to be able to still get some information don't use plain >> locking functions in the keyhandlers, but a variant of trylocks with >> a timeout value. This allows to wait for some time and to give up in >> case the lock was not obtained. >> >> Add the main infrastructure for this feature including a new runtime >> parameter allowing to specify the timeout value in milliseconds. >> >> Use the new locking scheme in the handlers defined in keyhandler.c. > > Personally I think trylock (as already used in some places) is the > way to go. Iirc others disagreed, but also didn't like the approach > taken here. I'm not intending to stand in the way if a majority > approves of this model, but I'm not going to ack these changes > myself. Fair enough. BTW, trylock is used at exactly three places: for dumping vNUMA, MSI and livepatch info. And TBH: the vNUMA case is really strange, as this is a rwlock which is held as writer only in one place for a very brief time period when freeing the domain's vnuma data. The MSI case is more complicated and looking at it in more detail I've realized that there is another trylock hidden in a subfunction: vpci_msix_arch_print(). As vpci_msix_arch_print() will drop the lock in the error case the interface to this function is rather weird. In addition the occasional softirq processing is erro prone, too, as it will happen only if a single domain has at least 64 MSI entries. In case of lots of domains with up to 63 entries watchdog timeouts can still happen, so I'll send a patch repairing this issue by letting vpci_msix_arch_print() dump only one entry and putting the loop and softirq handling into vpci_dump_msi(). Juergen
On 06.03.20 09:08, Jürgen Groß wrote: > On 05.03.20 16:25, Jan Beulich wrote: >> On 13.02.2020 13:54, Juergen Gross wrote: >>> Most keyhandlers are used to dump hypervisor data to the console and >>> they are used mostly for debugging purposes. In those cases it might >>> happen that some data structures are locked and thus are blocking the >>> handler to access the data. >>> >>> In order to be able to still get some information don't use plain >>> locking functions in the keyhandlers, but a variant of trylocks with >>> a timeout value. This allows to wait for some time and to give up in >>> case the lock was not obtained. >>> >>> Add the main infrastructure for this feature including a new runtime >>> parameter allowing to specify the timeout value in milliseconds. >>> >>> Use the new locking scheme in the handlers defined in keyhandler.c. >> >> Personally I think trylock (as already used in some places) is the >> way to go. Iirc others disagreed, but also didn't like the approach >> taken here. I'm not intending to stand in the way if a majority >> approves of this model, but I'm not going to ack these changes >> myself. > > Fair enough. > > BTW, trylock is used at exactly three places: for dumping vNUMA, MSI and > livepatch info. > > And TBH: the vNUMA case is really strange, as this is a rwlock which is > held as writer only in one place for a very brief time period when > freeing the domain's vnuma data. > > The MSI case is more complicated and looking at it in more detail I've > realized that there is another trylock hidden in a subfunction: > vpci_msix_arch_print(). As vpci_msix_arch_print() will drop the lock in > the error case the interface to this function is rather weird. In > addition the occasional softirq processing is erro prone, too, as it > will happen only if a single domain has at least 64 MSI entries. In case > of lots of domains with up to 63 entries watchdog timeouts can still > happen, so I'll send a patch repairing this issue by letting > vpci_msix_arch_print() dump only one entry and putting the loop and > softirq handling into vpci_dump_msi(). Oh, sorry, softirqs are processed often enough, I missed one call. Nevertheless the locking interface wants to be corrected IMO. Juergen
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 5051583a5d..ee3d031771 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -1384,6 +1384,15 @@ Force the use of `[<seg>:]<bus>:<device>.<func>` as device ID of IO-APIC `<ioapic>` instead of the one specified by the IVHD sub-tables of the IVRS ACPI table. +### keyhandler-lock-timeout +> `= <integer>` + +> Default: `1` + +> Can be modified at runtime + +Specify the lock timeout of keyhandlers in milliseconds. + ### lapic (x86) > `= <boolean>` diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index f53ae5ff86..1d09911dc0 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -22,6 +22,7 @@ #include <xen/grant_table.h> #include <xen/iocap.h> #include <xen/kernel.h> +#include <xen/keyhandler.h> #include <xen/hypercall.h> #include <xen/multicall.h> #include <xen/irq.h> @@ -222,7 +223,8 @@ void dump_pageframe_info(struct domain *d) { printk(" DomPage list too long to display\n"); } - else + else if ( keyhandler_spin_lock(&d->page_alloc_lock, + "could not read page_list") ) { unsigned long total[MASK_EXTR(PGT_type_mask, PGT_type_mask) + 1] = {}; @@ -251,7 +253,10 @@ void dump_pageframe_info(struct domain *d) if ( is_hvm_domain(d) ) p2m_pod_dump_data(d); - spin_lock(&d->page_alloc_lock); + if ( !keyhandler_spin_lock(&d->page_alloc_lock, + "could not read page_list") ) + return; + page_list_for_each ( page, &d->xenpage_list ) { printk(" XenPage %p: caf=%08lx, taf=%" PRtype_info "\n", diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c index f50490d0f3..c393d83b70 100644 --- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -14,8 +14,10 @@ #include <xen/rangeset.h> #include <xen/compat.h> #include <xen/ctype.h> +#include <xen/param.h> #include <xen/perfc.h> #include <xen/mm.h> +#include <xen/time.h> #include <xen/watchdog.h> #include <xen/init.h> #include <asm/debugger.h> @@ -71,6 +73,30 @@ static struct keyhandler { #undef KEYHANDLER }; +static unsigned int lock_timeout = 1; +integer_runtime_param("keyhandler-lock-timeout", lock_timeout); + +s_time_t keyhandler_lock_timeout(void) +{ + return NOW() + MILLISECS(lock_timeout); +} + +bool keyhandler_spin_lock(spinlock_t *lock, const char *msg) +{ + keyhandler_lock_body(bool, spin_trylock(lock), "%s\n", msg); +} + +bool keyhandler_spin_lock_irqsave(spinlock_t *lock, unsigned long *flags, + const char *msg) +{ + keyhandler_lock_body(bool, spin_trylock_irqsave(lock, *flags), "%s\n", msg); +} + +bool keyhandler_read_lock(rwlock_t *lock, const char *msg) +{ + keyhandler_lock_body(bool, read_trylock(lock), "%s\n", msg); +} + static void keypress_action(void *unused) { handle_keypress(keypress_key, NULL); @@ -378,7 +404,8 @@ static void read_clocks(unsigned char key) static u32 count = 0; static DEFINE_SPINLOCK(lock); - spin_lock(&lock); + if ( !keyhandler_spin_lock(&lock, "could not read clock stats") ) + return; smp_call_function(read_clocks_slave, NULL, 0); diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c index 4ebba30ba3..97104abb45 100644 --- a/xen/common/rangeset.c +++ b/xen/common/rangeset.c @@ -9,6 +9,7 @@ #include <xen/sched.h> #include <xen/errno.h> +#include <xen/keyhandler.h> #include <xen/rangeset.h> #include <xsm/xsm.h> @@ -546,7 +547,8 @@ static void rangeset_printk(struct rangeset *r) int nr_printed = 0; struct range *x; - read_lock(&r->lock); + if ( !keyhandler_read_lock(&r->lock, "could not read rangeset") ) + return; printk("%-10s {", r->name); @@ -575,7 +577,8 @@ void rangeset_domain_printk( printk("Rangesets belonging to domain %u:\n", d->domain_id); - spin_lock(&d->rangesets_lock); + if ( !keyhandler_spin_lock(&d->rangesets_lock, "could not get rangesets") ) + return; if ( list_empty(&d->rangesets) ) printk(" None\n"); diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h index 5131e86cbc..cc8e0b18f5 100644 --- a/xen/include/xen/keyhandler.h +++ b/xen/include/xen/keyhandler.h @@ -10,6 +10,9 @@ #ifndef __XEN_KEYHANDLER_H__ #define __XEN_KEYHANDLER_H__ +#include <xen/rwlock.h> +#include <xen/spinlock.h> +#include <xen/time.h> #include <xen/types.h> /* @@ -48,4 +51,27 @@ void register_irq_keyhandler(unsigned char key, /* Inject a keypress into the key-handling subsystem. */ extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs); +/* Locking primitives for inside keyhandlers (like trylock). */ +bool keyhandler_spin_lock(spinlock_t *lock, const char *msg); +bool keyhandler_spin_lock_irqsave(spinlock_t *lock, unsigned long *flags, + const char *msg); +bool keyhandler_read_lock(rwlock_t *lock, const char *msg); + +/* Primitives for custom keyhandler lock functions. */ +s_time_t keyhandler_lock_timeout(void); +#define keyhandler_lock_body(type, lockfunc, arg...) \ + s_time_t end = keyhandler_lock_timeout(); \ + type ret; \ + \ + do { \ + ret = lockfunc; \ + if ( ret ) \ + return ret; \ + cpu_relax(); \ + } while ( NOW() < end ); \ + \ + printk("-->lock conflict: " arg); \ + \ + return ret + #endif /* __XEN_KEYHANDLER_H__ */
Most keyhandlers are used to dump hypervisor data to the console and they are used mostly for debugging purposes. In those cases it might happen that some data structures are locked and thus are blocking the handler to access the data. In order to be able to still get some information don't use plain locking functions in the keyhandlers, but a variant of trylocks with a timeout value. This allows to wait for some time and to give up in case the lock was not obtained. Add the main infrastructure for this feature including a new runtime parameter allowing to specify the timeout value in milliseconds. Use the new locking scheme in the handlers defined in keyhandler.c. Signed-off-by: Juergen Gross <jgross@suse.com> --- docs/misc/xen-command-line.pandoc | 9 +++++++++ xen/arch/x86/domain.c | 9 +++++++-- xen/common/keyhandler.c | 29 ++++++++++++++++++++++++++++- xen/common/rangeset.c | 7 +++++-- xen/include/xen/keyhandler.h | 26 ++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 5 deletions(-)