Date: Mon, 29 Feb 2016 03:54:51 +0000 (UTC) From: Sepherosa Ziehau <sephe@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r296178 - in head/sys: net sys Message-ID: <201602290354.u1T3spSw090156@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: sephe Date: Mon Feb 29 03:54:51 2016 New Revision: 296178 URL: https://svnweb.freebsd.org/changeset/base/296178 Log: buf_ring/drbr: Add buf_ring_peek_clear_sc and use it in drbr_peek Unlike buf_ring_peek, it only supports single consumer mode, and it clears the cons_head if DEBUG_BUFRING/INVARIANTS is defined. The normal use case of drbr_peek for network drivers is: m = drbr_peek(br); err = hw_spec_encap(&m); /* could m_defrag/m_collapse */ (*) if (err) { if (m == NULL) drbr_advance(br); else drbr_putback(br, m); /* break the loop */ } drbr_advance(br); The race is: If hw_spec_encap() m_defrag or m_collapse the mbuf, i.e. the old mbuf was freed, or like the Hyper-V's network driver, that transmission- done does not even require the TX lock; then on the other CPU at the (*) time, the freed mbuf could be recycled and being drbr_enqueue even before the current CPU had the chance to call drbr_{advance,putback}. This triggers a panic in drbr_enqueue duplicated element check, if DEBUG_BUFRING/INVARIANTS is defined. Use buf_ring_peek_clear_sc() in drbr_peek() to fix the above race. This change is a NO-OP, if neither DEBUG_BUFRING nor INVARIANTS are defined. MFC after: 1 week Sponsored by: Microsoft OSTC Differential Revision: https://reviews.freebsd.org/D5416 Modified: head/sys/net/ifq.h head/sys/sys/buf_ring.h Modified: head/sys/net/ifq.h ============================================================================== --- head/sys/net/ifq.h Mon Feb 29 03:38:00 2016 (r296177) +++ head/sys/net/ifq.h Mon Feb 29 03:54:51 2016 (r296178) @@ -369,7 +369,7 @@ drbr_peek(struct ifnet *ifp, struct buf_ return (m); } #endif - return(buf_ring_peek(br)); + return(buf_ring_peek_clear_sc(br)); } static __inline void Modified: head/sys/sys/buf_ring.h ============================================================================== --- head/sys/sys/buf_ring.h Mon Feb 29 03:38:00 2016 (r296177) +++ head/sys/sys/buf_ring.h Mon Feb 29 03:54:51 2016 (r296178) @@ -268,6 +268,37 @@ buf_ring_peek(struct buf_ring *br) return (br->br_ring[br->br_cons_head]); } +static __inline void * +buf_ring_peek_clear_sc(struct buf_ring *br) +{ +#ifdef DEBUG_BUFRING + void *ret; + + if (!mtx_owned(br->br_lock)) + panic("lock not held on single consumer dequeue"); +#endif + /* + * I believe it is safe to not have a memory barrier + * here because we control cons and tail is worst case + * a lagging indicator so we worst case we might + * return NULL immediately after a buffer has been enqueued + */ + if (br->br_cons_head == br->br_prod_tail) + return (NULL); + +#ifdef DEBUG_BUFRING + /* + * Single consumer, i.e. cons_head will not move while we are + * running, so atomic_swap_ptr() is not necessary here. + */ + ret = br->br_ring[br->br_cons_head]; + br->br_ring[br->br_cons_head] = NULL; + return (ret); +#else + return (br->br_ring[br->br_cons_head]); +#endif +} + static __inline int buf_ring_full(struct buf_ring *br) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201602290354.u1T3spSw090156>