Message ID | CAE4R7bD1EmYm4TNrWWskTeKA6NfNsasVRTP4KxhOYvZt_9n3Ow@mail.gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
Hi Scott, > Simon, just focusing on this rocker->neigh_tbl_next_index++ issue at > the moment, I think the change below would make _rocker_neigh_add() > safe to call without needing to put neigh_update into process context. > It works because entry is stored between PREPARE and COMMIT, so once > entry->index is assigned in PREPARE (under neigh_tbl_lock), it'll be > re-used in COMMIT, even if the NONE thread also claims it's > entry->index (also under neigh_tbl_lock). > > I know there are other issues you and Toshiaki Makita have pointed > out, but let's see if we can peel the onion one layer at a time. > > --- a/drivers/net/ethernet/rocker/rocker.c > +++ b/drivers/net/ethernet/rocker/rocker.c > @@ -2904,10 +2904,10 @@ static void _rocker_neigh_add(struct rocker *rocker, > enum switchdev_trans trans, > struct rocker_neigh_tbl_entry *entry) > { > - entry->index = rocker->neigh_tbl_next_index; > + if (trans != SWITCHDEV_TRANS_COMMIT) > + entry->index = rocker->neigh_tbl_next_index++; > if (trans == SWITCHDEV_TRANS_PREPARE) > return; > - rocker->neigh_tbl_next_index++; > entry->ref_count++; > hash_add(rocker->neigh_tbl, &entry->entry, > be32_to_cpu(entry->ip_addr)); I agree that should work. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 4, 2015 at 3:54 PM, Simon Horman <horms@verge.net.au> wrote: > Hi Scott, > >> Simon, just focusing on this rocker->neigh_tbl_next_index++ issue at >> the moment, I think the change below would make _rocker_neigh_add() >> safe to call without needing to put neigh_update into process context. >> It works because entry is stored between PREPARE and COMMIT, so once >> entry->index is assigned in PREPARE (under neigh_tbl_lock), it'll be >> re-used in COMMIT, even if the NONE thread also claims it's >> entry->index (also under neigh_tbl_lock). >> >> I know there are other issues you and Toshiaki Makita have pointed >> out, but let's see if we can peel the onion one layer at a time. >> >> --- a/drivers/net/ethernet/rocker/rocker.c >> +++ b/drivers/net/ethernet/rocker/rocker.c >> @@ -2904,10 +2904,10 @@ static void _rocker_neigh_add(struct rocker *rocker, >> enum switchdev_trans trans, >> struct rocker_neigh_tbl_entry *entry) >> { >> - entry->index = rocker->neigh_tbl_next_index; >> + if (trans != SWITCHDEV_TRANS_COMMIT) >> + entry->index = rocker->neigh_tbl_next_index++; >> if (trans == SWITCHDEV_TRANS_PREPARE) >> return; >> - rocker->neigh_tbl_next_index++; >> entry->ref_count++; >> hash_add(rocker->neigh_tbl, &entry->entry, >> be32_to_cpu(entry->ip_addr)); > > I agree that should work. I've been running it today with no problems. So I'm preparing a new patch that changes direction here, to address David's concern about switching neigh event context. Details: 1) Above little patch addresses the race issue with neigh_tbl_next_index 2) Keep neigh update event in event context and bring back a couple of items I removed in the Spring Cleanup series: a) use ROCKER_OP_FLAG_NOWAIT to mark contexts that can't sleep b) if nowait, mem allocations use GFP_ATOMIC c) if nowait, cmd to device is issued, but we will not sleep to wait for response from device; just hit-and-run I was probably too aggressive with Spring Cleanup thinking we could swing all device accesses to process context. Thanks to all for the testing/review/feedback/patience. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/net/ethernet/rocker/rocker.c +++ b/drivers/net/ethernet/rocker/rocker.c @@ -2904,10 +2904,10 @@ static void _rocker_neigh_add(struct rocker *rocker, enum switchdev_trans trans, struct rocker_neigh_tbl_entry *entry) { - entry->index = rocker->neigh_tbl_next_index; + if (trans != SWITCHDEV_TRANS_COMMIT) + entry->index = rocker->neigh_tbl_next_index++; if (trans == SWITCHDEV_TRANS_PREPARE) return; - rocker->neigh_tbl_next_index++; entry->ref_count++; hash_add(rocker->neigh_tbl, &entry->entry, be32_to_cpu(entry->ip_addr));