Skip site navigation (1)Skip section navigation (2) 
Date:      Wed, 24 Feb 2016 01:52:45 +0000
From:      "sepherosa_gmail.com (Sepherosa Ziehau)" <phabric-noreply@FreeBSD.org>
To:        freebsd-net@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:  <differential-rev-PHID-DREV-ujjgotek7ybhtamwk6ba-req@FreeBSD.org>
next in thread | raw e-mail | index | archive | help 

--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--

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?differential-rev-PHID-DREV-ujjgotek7ybhtamwk6ba-req>