Re: CVS commit: src/sys/net

[Ryota Ozaki]

On Mon, Apr 11, 2016 at 12:31 PM, Taylor R Campbell
 wrote:
>Date: Mon, 11 Apr 2016 02:04:14 +
>From: Ryota Ozaki 
>
>Module Name:src
>Committed By:   ozaki-r
>Date:   Mon Apr 11 02:04:14 UTC 2016
>
>Modified Files:
>src/sys/net: bridgestp.c if_bridge.c if_bridgevar.h
>
>Log Message:
>Use pslist(9) in bridge(4)
>
>This adds missing memory barriers to list operations for pserialize.
>
> Nice, thanks for doing this!  Have you tried subjecting it to load,
> with options DIAGNOSTIC?

Yes. It passes ATF tests (that enable DIAGNOSTIC).

>
>Index: src/sys/net/bridgestp.c
>diff -u src/sys/net/bridgestp.c:1.19 src/sys/net/bridgestp.c:1.20
>--- src/sys/net/bridgestp.c:1.19Mon Feb 15 01:11:41 2016
>+++ src/sys/net/bridgestp.c Mon Apr 11 02:04:14 2016
>@@ -341,7 +341,8 @@ bstp_config_bpdu_generation(struct bridg
> {
>struct bridge_iflist *bif;
>
>-   LIST_FOREACH(bif, >sc_iflist, bif_next) {
>+   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
>+   bif_next) {
>
> I'm pretty sure everything in bridgestp.c runs under a writer lock, so
> you should use PSLIST_WRITER_FOREACH here, not PSLIST_READER_FOREACH.
>
> (PSLIST_WRITER_FOREACH is faster on alpha -- but more important is the
> statement of intent that this is in an exclusive write section, not a
> shared read section.)
>
>@@ -828,7 +833,8 @@ bstp_initialization(struct bridge_softc
>
>BRIDGE_LOCK(sc);
>
>-   LIST_FOREACH(bif, >sc_iflist, bif_next) {
>+   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
>+   bif_next) {
>
> For example, this one is definitely done under a writer lock!

Oh yes. I think I already forgot bridgestp.c at all!

>
>Index: src/sys/net/if_bridge.c
>diff -u src/sys/net/if_bridge.c:1.111 src/sys/net/if_bridge.c:1.112
>--- src/sys/net/if_bridge.c:1.111   Mon Mar 28 04:38:04 2016
>+++ src/sys/net/if_bridge.c Mon Apr 11 02:04:14 2016
>@@ -447,8 +447,14 @@ bridge_clone_destroy(struct ifnet *ifp)
>bridge_stop(ifp, 1);
>
>BRIDGE_LOCK(sc);
>-   while ((bif = LIST_FIRST(>sc_iflist)) != NULL)
>+   for (;;) {
>+   bif = PSLIST_WRITER_FIRST(>sc_iflist, struct 
> bridge_iflist,
>+   bif_next);
>+   if (bif == NULL)
>+   break;
>bridge_delete_member(sc, bif);
>+   }
>+   PSLIST_DESTROY(>sc_iflist);
>
> Any particular reason you switched from `while ((bif = ...) != NULL)'
> to `for (;;) { bif = ...; if (bif == NULL) break;'?   I find the while
> construct clearer, myself.

Just because it's too long (for me) to put it in the condition.

>
>@@ -705,7 +712,8 @@ bridge_delete_member(struct bridge_softc
>ifs->if_bridge = NULL;
>ifs->if_bridgeif = NULL;
>
>-   LIST_REMOVE(bif, bif_next);
>+   PSLIST_WRITER_REMOVE(bif, bif_next);
>+   PSLIST_ENTRY_DESTROY(bif, bif_next);
>
>BRIDGE_PSZ_PERFORM(sc);
>
> This order is incorrect.  You cannot destroy the entry until you have
> confirmed all readers are done with it.  You must pserialize_perform
> before doing PSLIST_ENTRY_DESTROY.  See the note in the pslist(9) man
> page about this:
>
> `Either _element_ must never have been inserted into a list, or it
> must have been inserted and removed, and the caller must have waited
> for all parallel readers to finish reading it first.'
>
> The reason is that PSLIST_WRITER_REMOVE only changes the next pointer
> of the *previous* element so that no new readers can discover the
> current element, but leaves the next pointer of the *current* element
> intact so that readers that are still active can get to the next
> element.  Then PSLIST_ENTRY_DESTROY nullifies the next pointer of the
> current element and kasserts that you removed the entry.

I see. (I should be claimed RTFM :-/)

>
> Maybe PSLIST_ENTRY_DESTROY should really set it to some non-null
> poison pointer like QUEUEDEBUG does, so that anyone trying to use it
> afterward will actually crash instead of just thinking they hit the
> end of the list.
>
>@@ -939,7 +949,8 @@ bridge_ioctl_gifs(struct bridge_softc *s
> retry:
>BRIDGE_LOCK(sc);
>count = 0;
>-   LIST_FOREACH(bif, >sc_iflist, bif_next)
>+   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
>+   bif_next)
>
> Use PSLIST_WRITER_FOREACH here too.
>
>@@ -959,7 +970,8 @@ retry:
>BRIDGE_LOCK(sc);
>
>i = 0;
>-   LIST_FOREACH(bif, >sc_iflist, bif_next)
>+   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
>+   bif_next)
>
> PSLIST_WRITER_FOREACH
>
>i++;
>if (i > count) {
>/*

Re: CVS commit: src/sys/net

[Taylor R Campbell]

   Date: Mon, 11 Apr 2016 02:04:14 +
   From: Ryota Ozaki 

   Module Name:src
   Committed By:   ozaki-r
   Date:   Mon Apr 11 02:04:14 UTC 2016

   Modified Files:
   src/sys/net: bridgestp.c if_bridge.c if_bridgevar.h

   Log Message:
   Use pslist(9) in bridge(4)

   This adds missing memory barriers to list operations for pserialize.

Nice, thanks for doing this!  Have you tried subjecting it to load,
with options DIAGNOSTIC?

   Index: src/sys/net/bridgestp.c
   diff -u src/sys/net/bridgestp.c:1.19 src/sys/net/bridgestp.c:1.20
   --- src/sys/net/bridgestp.c:1.19Mon Feb 15 01:11:41 2016
   +++ src/sys/net/bridgestp.c Mon Apr 11 02:04:14 2016
   @@ -341,7 +341,8 @@ bstp_config_bpdu_generation(struct bridg
{
   struct bridge_iflist *bif;

   -   LIST_FOREACH(bif, >sc_iflist, bif_next) {
   +   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
   +   bif_next) {

I'm pretty sure everything in bridgestp.c runs under a writer lock, so
you should use PSLIST_WRITER_FOREACH here, not PSLIST_READER_FOREACH.

(PSLIST_WRITER_FOREACH is faster on alpha -- but more important is the
statement of intent that this is in an exclusive write section, not a
shared read section.)

   @@ -828,7 +833,8 @@ bstp_initialization(struct bridge_softc 

   BRIDGE_LOCK(sc);

   -   LIST_FOREACH(bif, >sc_iflist, bif_next) {
   +   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
   +   bif_next) {

For example, this one is definitely done under a writer lock!

   Index: src/sys/net/if_bridge.c
   diff -u src/sys/net/if_bridge.c:1.111 src/sys/net/if_bridge.c:1.112
   --- src/sys/net/if_bridge.c:1.111   Mon Mar 28 04:38:04 2016
   +++ src/sys/net/if_bridge.c Mon Apr 11 02:04:14 2016
   @@ -447,8 +447,14 @@ bridge_clone_destroy(struct ifnet *ifp)
   bridge_stop(ifp, 1);

   BRIDGE_LOCK(sc);
   -   while ((bif = LIST_FIRST(>sc_iflist)) != NULL)
   +   for (;;) {
   +   bif = PSLIST_WRITER_FIRST(>sc_iflist, struct 
bridge_iflist,
   +   bif_next);
   +   if (bif == NULL)
   +   break;
   bridge_delete_member(sc, bif);
   +   }
   +   PSLIST_DESTROY(>sc_iflist);

Any particular reason you switched from `while ((bif = ...) != NULL)'
to `for (;;) { bif = ...; if (bif == NULL) break;'?   I find the while
construct clearer, myself.

   @@ -705,7 +712,8 @@ bridge_delete_member(struct bridge_softc
   ifs->if_bridge = NULL;
   ifs->if_bridgeif = NULL;

   -   LIST_REMOVE(bif, bif_next);
   +   PSLIST_WRITER_REMOVE(bif, bif_next);
   +   PSLIST_ENTRY_DESTROY(bif, bif_next);

   BRIDGE_PSZ_PERFORM(sc);

This order is incorrect.  You cannot destroy the entry until you have
confirmed all readers are done with it.  You must pserialize_perform
before doing PSLIST_ENTRY_DESTROY.  See the note in the pslist(9) man
page about this:

`Either _element_ must never have been inserted into a list, or it
must have been inserted and removed, and the caller must have waited
for all parallel readers to finish reading it first.'

The reason is that PSLIST_WRITER_REMOVE only changes the next pointer
of the *previous* element so that no new readers can discover the
current element, but leaves the next pointer of the *current* element
intact so that readers that are still active can get to the next
element.  Then PSLIST_ENTRY_DESTROY nullifies the next pointer of the
current element and kasserts that you removed the entry.

Maybe PSLIST_ENTRY_DESTROY should really set it to some non-null
poison pointer like QUEUEDEBUG does, so that anyone trying to use it
afterward will actually crash instead of just thinking they hit the
end of the list.

   @@ -939,7 +949,8 @@ bridge_ioctl_gifs(struct bridge_softc *s
retry:
   BRIDGE_LOCK(sc);
   count = 0;
   -   LIST_FOREACH(bif, >sc_iflist, bif_next)
   +   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
   +   bif_next)

Use PSLIST_WRITER_FOREACH here too.

   @@ -959,7 +970,8 @@ retry:
   BRIDGE_LOCK(sc);

   i = 0;
   -   LIST_FOREACH(bif, >sc_iflist, bif_next)
   +   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
   +   bif_next)

PSLIST_WRITER_FOREACH

   i++;
   if (i > count) {
   /*
   @@ -972,7 +984,8 @@ retry:
   }

   i = 0;
   -   LIST_FOREACH(bif, >sc_iflist, bif_next) {
   +   PSLIST_READER_FOREACH(bif, >sc_iflist, struct bridge_iflist,
   +   bif_next) {

PSLIST_WRITER_FOREACH