Date: Wed, 15 Jul 2015 17:36:36 +0000 (UTC) From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r285607 - head/sys/kern Message-ID: <201507151736.t6FHaaBA076834@svnmir.geo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: kib Date: Wed Jul 15 17:36:35 2015 New Revision: 285607 URL: https://svnweb.freebsd.org/changeset/base/285607 Log: Reset non-zero it_need indicator to zero atomically with fetching its current value. It is believed that the change is the real fix for the issue which was covered over by the r252683. With the current code, if the interrupt handler sets it_need between read and consequent reset, the update could be lost and ithread_execute_handlers() would not be called in response to the lost update. The r252683 could have hide the issue since at the moment of commit, atomic_load_acq_int() did locked cmpxchg on the variable, which puts the cache line into the exclusive owned state and clears store buffers. Then the immediate store of zero has very high chance of reusing the exclusive state of the cache line and make the load and store sequence operate as atomic swap. For now, add the acq+rel fence immediately after the swap, to not disturb current (but excessive) ordering. Acquire is needed for the ih_need reads after the load, while release does not serve a useful purpose [*]. Reviewed by: alc Noted by: alc [*] Discussed with: bde Tested by: pho Sponsored by: The FreeBSD Foundation MFC after: 2 weeks Modified: head/sys/kern/kern_intr.c Modified: head/sys/kern/kern_intr.c ============================================================================== --- head/sys/kern/kern_intr.c Wed Jul 15 17:14:05 2015 (r285606) +++ head/sys/kern/kern_intr.c Wed Jul 15 17:36:35 2015 (r285607) @@ -1327,14 +1327,13 @@ ithread_loop(void *arg) * we are running, it will set it_need to note that we * should make another pass. */ - while (atomic_load_acq_int(&ithd->it_need) != 0) { + while (atomic_swap_int(&ithd->it_need, 0) != 0) { /* - * This might need a full read and write barrier - * to make sure that this write posts before any - * of the memory or device accesses in the - * handlers. + * This needs a release barrier to make sure + * that this write posts before any of the + * memory or device accesses in the handlers. */ - atomic_store_rel_int(&ithd->it_need, 0); + atomic_thread_fence_acq_rel(); ithread_execute_handlers(p, ie); } WITNESS_WARN(WARN_PANIC, NULL, "suspending ithread"); @@ -1507,14 +1506,13 @@ ithread_loop(void *arg) * we are running, it will set it_need to note that we * should make another pass. */ - while (atomic_load_acq_int(&ithd->it_need) != 0) { + while (atomic_swap_int(&ithd->it_need, 0) != 0) { /* - * This might need a full read and write barrier - * to make sure that this write posts before any - * of the memory or device accesses in the - * handlers. + * This needs a release barrier to make sure + * that this write posts before any of the + * memory or device accesses in the handlers. */ - atomic_store_rel_int(&ithd->it_need, 0); + atomic_thread_fence_acq_rel(); if (priv) priv_ithread_execute_handler(p, ih); else
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201507151736.t6FHaaBA076834>