From owner-freebsd-net@freebsd.org Wed Feb 24 01:52:45 2016 Return-Path: Delivered-To: freebsd-net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7D338AB1825 for ; Wed, 24 Feb 2016 01:52:45 +0000 (UTC) (envelope-from daemon-user@freebsd.org) Received: from phabric-backend.rbsd.freebsd.org (unknown [IPv6:2607:fc50:2000:101::1bb:73]) by mx1.freebsd.org (Postfix) with ESMTP id 67C811399 for ; Wed, 24 Feb 2016 01:52:45 +0000 (UTC) (envelope-from daemon-user@freebsd.org) Received: by phabric-backend.rbsd.freebsd.org (Postfix, from userid 1346) id 655CA3321D8C; Wed, 24 Feb 2016 01:52:45 +0000 (UTC) Date: Wed, 24 Feb 2016 01:52:45 +0000 To: freebsd-net@freebsd.org From: "sepherosa_gmail.com (Sepherosa Ziehau)" Reply-to: D5416+325+d90172de5c58ed8c@reviews.freebsd.org Subject: [Differential] [Request, 33 lines] D5416: buf_ring/drbr: Add buf_ring_peek_clear_sc and use it in drbr_peek Message-ID: X-Priority: 3 X-Phabricator-Sent-This-Message: Yes X-Mail-Transport-Agent: MetaMTA X-Auto-Response-Suppress: All X-Phabricator-Mail-Tags: , , , Thread-Topic: D5416: buf_ring/drbr: Add buf_ring_peek_clear_sc and use it in drbr_peek X-Herald-Rules: none X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-Cc: Precedence: bulk Thread-Index: Mjg4YWY1NmZjMjNkYWVhODFjM2QxMWJiNjA5 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="b1_0d2cf905543354752928c7081326154e" X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.20 List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Feb 2016 01:52:45 -0000 --b1_0d2cf905543354752928c7081326154e Content-Type: text/plain; charset = "utf-8" Content-Transfer-Encoding: 8bit sepherosa_gmail.com created this revision. sepherosa_gmail.com added reviewers: network, adrian, delphij, rrs, glebius, kmacy, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com. sepherosa_gmail.com added a subscriber: freebsd-net-list. REVISION SUMMARY 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. REVISION DETAIL https://reviews.freebsd.org/D5416 AFFECTED FILES sys/net/ifq.h sys/sys/buf_ring.h CHANGE DETAILS diff --git a/sys/sys/buf_ring.h b/sys/sys/buf_ring.h --- a/sys/sys/buf_ring.h +++ b/sys/sys/buf_ring.h @@ -268,6 +268,37 @@ 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) { diff --git a/sys/net/ifq.h b/sys/net/ifq.h --- a/sys/net/ifq.h +++ b/sys/net/ifq.h @@ -369,7 +369,7 @@ return (m); } #endif - return(buf_ring_peek(br)); + return(buf_ring_peek_clear_sc(br)); } static __inline void EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, adrian, delphij, rrs, glebius, kmacy, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com Cc: freebsd-net-list --b1_0d2cf905543354752928c7081326154e Content-Type: text/x-patch; charset=utf-8; name="D5416.13669.patch" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="D5416.13669.patch" ZGlmZiAtLWdpdCBhL3N5cy9zeXMvYnVmX3JpbmcuaCBiL3N5cy9zeXMvYnVmX3JpbmcuaAotLS0g YS9zeXMvc3lzL2J1Zl9yaW5nLmgKKysrIGIvc3lzL3N5cy9idWZfcmluZy5oCkBAIC0yNjgsNiAr MjY4LDM3IEBACiAJcmV0dXJuIChici0+YnJfcmluZ1tici0+YnJfY29uc19oZWFkXSk7CiB9CiAK K3N0YXRpYyBfX2lubGluZSB2b2lkICoKK2J1Zl9yaW5nX3BlZWtfY2xlYXJfc2Moc3RydWN0IGJ1 Zl9yaW5nICpicikKK3sKKyNpZmRlZiBERUJVR19CVUZSSU5HCisJdm9pZCAqcmV0OworCisJaWYg KCFtdHhfb3duZWQoYnItPmJyX2xvY2spKQorCQlwYW5pYygibG9jayBub3QgaGVsZCBvbiBzaW5n bGUgY29uc3VtZXIgZGVxdWV1ZSIpOworI2VuZGlmCQorCS8qCisJICogSSBiZWxpZXZlIGl0IGlz IHNhZmUgdG8gbm90IGhhdmUgYSBtZW1vcnkgYmFycmllcgorCSAqIGhlcmUgYmVjYXVzZSB3ZSBj b250cm9sIGNvbnMgYW5kIHRhaWwgaXMgd29yc3QgY2FzZQorCSAqIGEgbGFnZ2luZyBpbmRpY2F0 b3Igc28gd2Ugd29yc3QgY2FzZSB3ZSBtaWdodAorCSAqIHJldHVybiBOVUxMIGltbWVkaWF0ZWx5 IGFmdGVyIGEgYnVmZmVyIGhhcyBiZWVuIGVucXVldWVkCisJICovCisJaWYgKGJyLT5icl9jb25z X2hlYWQgPT0gYnItPmJyX3Byb2RfdGFpbCkKKwkJcmV0dXJuIChOVUxMKTsKKworI2lmZGVmIERF QlVHX0JVRlJJTkcKKwkvKgorCSAqIFNpbmdsZSBjb25zdW1lciwgaS5lLiBjb25zX2hlYWQgd2ls bCBub3QgbW92ZSB3aGlsZSB3ZSBhcmUKKwkgKiBydW5uaW5nLCBzbyBhdG9taWNfc3dhcF9wdHIo KSBpcyBub3QgbmVjZXNzYXJ5IGhlcmUuCisJICovCisJcmV0ID0gYnItPmJyX3JpbmdbYnItPmJy X2NvbnNfaGVhZF07CisJYnItPmJyX3JpbmdbYnItPmJyX2NvbnNfaGVhZF0gPSBOVUxMOworCXJl dHVybiAocmV0KTsKKyNlbHNlCisJcmV0dXJuIChici0+YnJfcmluZ1tici0+YnJfY29uc19oZWFk XSk7CisjZW5kaWYKK30KKwogc3RhdGljIF9faW5saW5lIGludAogYnVmX3JpbmdfZnVsbChzdHJ1 Y3QgYnVmX3JpbmcgKmJyKQogewpkaWZmIC0tZ2l0IGEvc3lzL25ldC9pZnEuaCBiL3N5cy9uZXQv aWZxLmgKLS0tIGEvc3lzL25ldC9pZnEuaAorKysgYi9zeXMvbmV0L2lmcS5oCkBAIC0zNjksNyAr MzY5LDcgQEAKIAkJcmV0dXJuIChtKTsKIAl9CiAjZW5kaWYKLQlyZXR1cm4oYnVmX3JpbmdfcGVl ayhicikpOworCXJldHVybihidWZfcmluZ19wZWVrX2NsZWFyX3NjKGJyKSk7CiB9CiAKIHN0YXRp YyBfX2lubGluZSB2b2lkCgo= --b1_0d2cf905543354752928c7081326154e--