* [PATCH net 0/3] fixes the memory barrier for SCRQ/CRQ entry
@ 2021-01-21 6:17 Lijun Pan
2021-01-21 6:17 ` [PATCH net 1/3] ibmvnic: rework to ensure SCRQ entry reads are properly ordered Lijun Pan
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Lijun Pan @ 2021-01-21 6:17 UTC (permalink / raw)
To: netdev; +Cc: Lijun Pan
This series rework/fix the memory barrier for SCRQ (Sub-Command-Response
Queue) and CRQ (Command-Response Queue) entries.
This series does not have merge conflict with Suka's
https://lists.openwall.net/netdev/2021/01/08/89
Lijun Pan (3):
ibmvnic: rework to ensure SCRQ entry reads are properly ordered
ibmvnic: remove unnecessary rmb() inside ibmvnic_poll
ibmvnic: Ensure that CRQ entry read/write are correctly ordered
drivers/net/ethernet/ibm/ibmvnic.c | 54 ++++++++++++++++++++----------
1 file changed, 36 insertions(+), 18 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH net 1/3] ibmvnic: rework to ensure SCRQ entry reads are properly ordered 2021-01-21 6:17 [PATCH net 0/3] fixes the memory barrier for SCRQ/CRQ entry Lijun Pan @ 2021-01-21 6:17 ` Lijun Pan 2021-01-24 5:08 ` Jakub Kicinski 2021-01-21 6:17 ` [PATCH net 2/3] ibmvnic: remove unnecessary rmb() inside ibmvnic_poll Lijun Pan 2021-01-21 6:17 ` [PATCH net 3/3] ibmvnic: Ensure that CRQ entry read/write are correctly ordered Lijun Pan 2 siblings, 1 reply; 9+ messages in thread From: Lijun Pan @ 2021-01-21 6:17 UTC (permalink / raw) To: netdev; +Cc: Lijun Pan Move the dma_rmb() between pending_scrq() and ibmvnic_next_scrq() into the end of pending_scrq(), and explain why. Explain in detail why the dma_rmb() is placed at the end of ibmvnic_next_scrq(). Fixes: b71ec9522346 ("ibmvnic: Ensure that SCRQ entry reads are correctly ordered") Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 41 +++++++++++++++++------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 9778c83150f1..8e043683610f 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -2511,12 +2511,6 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) if (!pending_scrq(adapter, rx_scrq)) break; - /* The queue entry at the current index is peeked at above - * to determine that there is a valid descriptor awaiting - * processing. We want to be sure that the current slot - * holds a valid descriptor before reading its contents. - */ - dma_rmb(); next = ibmvnic_next_scrq(adapter, rx_scrq); rx_buff = (struct ibmvnic_rx_buff *)be64_to_cpu(next-> @@ -3256,13 +3250,6 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, int total_bytes = 0; int num_packets = 0; - /* The queue entry at the current index is peeked at above - * to determine that there is a valid descriptor awaiting - * processing. We want to be sure that the current slot - * holds a valid descriptor before reading its contents. - */ - dma_rmb(); - next = ibmvnic_next_scrq(adapter, scrq); for (i = 0; i < next->tx_comp.num_comps; i++) { if (next->tx_comp.rcs[i]) @@ -3636,11 +3623,25 @@ static int pending_scrq(struct ibmvnic_adapter *adapter, struct ibmvnic_sub_crq_queue *scrq) { union sub_crq *entry = &scrq->msgs[scrq->cur]; + int rc; if (entry->generic.first & IBMVNIC_CRQ_CMD_RSP) - return 1; + rc = 1; else - return 0; + rc = 0; + + /* Ensure that the entire SCRQ descriptor scrq->msgs + * has been loaded before reading its contents. + * This barrier makes sure this function's entry, esp. + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP + * 1. is loaded before ibmvnic_next_scrq()'s + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP; + * 2. OR is loaded before ibmvnic_poll()'s + * disable_scrq_irq()'s scrq->hw_irq. + */ + dma_rmb(); + + return rc; } static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, @@ -3659,8 +3660,14 @@ static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, } spin_unlock_irqrestore(&scrq->lock, flags); - /* Ensure that the entire buffer descriptor has been - * loaded before reading its contents + /* Ensure that the entire SCRQ descriptor scrq->msgs + * has been loaded before reading its contents. + * This barrier makes sure this function's entry, esp. + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP + * 1. is loaded before ibmvnic_poll()'s + * be64_to_cpu(next->rx_comp.correlator); + * 2. OR is loaded before ibmvnic_complet_tx()'s + * be32_to_cpu(next->tx_comp.correlators[i]). */ dma_rmb(); -- 2.23.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/3] ibmvnic: rework to ensure SCRQ entry reads are properly ordered 2021-01-21 6:17 ` [PATCH net 1/3] ibmvnic: rework to ensure SCRQ entry reads are properly ordered Lijun Pan @ 2021-01-24 5:08 ` Jakub Kicinski 2021-01-25 4:34 ` Lijun Pan 0 siblings, 1 reply; 9+ messages in thread From: Jakub Kicinski @ 2021-01-24 5:08 UTC (permalink / raw) To: Lijun Pan; +Cc: netdev On Thu, 21 Jan 2021 00:17:08 -0600 Lijun Pan wrote: > Move the dma_rmb() between pending_scrq() and ibmvnic_next_scrq() > into the end of pending_scrq(), and explain why. > Explain in detail why the dma_rmb() is placed at the end of > ibmvnic_next_scrq(). > > Fixes: b71ec9522346 ("ibmvnic: Ensure that SCRQ entry reads are correctly ordered") > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> I fail to see why this is a fix. You move the barrier from caller to callee but there are no new barriers here. Did I miss some? > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > index 9778c83150f1..8e043683610f 100644 > --- a/drivers/net/ethernet/ibm/ibmvnic.c > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > @@ -2511,12 +2511,6 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) > > if (!pending_scrq(adapter, rx_scrq)) > break; > - /* The queue entry at the current index is peeked at above > - * to determine that there is a valid descriptor awaiting > - * processing. We want to be sure that the current slot > - * holds a valid descriptor before reading its contents. > - */ > - dma_rmb(); > next = ibmvnic_next_scrq(adapter, rx_scrq); > rx_buff = > (struct ibmvnic_rx_buff *)be64_to_cpu(next-> > @@ -3256,13 +3250,6 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, > int total_bytes = 0; > int num_packets = 0; > > - /* The queue entry at the current index is peeked at above > - * to determine that there is a valid descriptor awaiting > - * processing. We want to be sure that the current slot > - * holds a valid descriptor before reading its contents. > - */ > - dma_rmb(); > - > next = ibmvnic_next_scrq(adapter, scrq); > for (i = 0; i < next->tx_comp.num_comps; i++) { > if (next->tx_comp.rcs[i]) > @@ -3636,11 +3623,25 @@ static int pending_scrq(struct ibmvnic_adapter *adapter, > struct ibmvnic_sub_crq_queue *scrq) > { > union sub_crq *entry = &scrq->msgs[scrq->cur]; > + int rc; > > if (entry->generic.first & IBMVNIC_CRQ_CMD_RSP) > - return 1; > + rc = 1; > else > - return 0; > + rc = 0; > + > + /* Ensure that the entire SCRQ descriptor scrq->msgs > + * has been loaded before reading its contents. This comment is confusing. IMHO the comments you're removing are much clearer. > + * This barrier makes sure this function's entry, esp. > + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP > + * 1. is loaded before ibmvnic_next_scrq()'s > + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP; > + * 2. OR is loaded before ibmvnic_poll()'s > + * disable_scrq_irq()'s scrq->hw_irq. > + */ > + dma_rmb(); > + > + return rc; > } > > static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/3] ibmvnic: rework to ensure SCRQ entry reads are properly ordered 2021-01-24 5:08 ` Jakub Kicinski @ 2021-01-25 4:34 ` Lijun Pan 0 siblings, 0 replies; 9+ messages in thread From: Lijun Pan @ 2021-01-25 4:34 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Lijun Pan, netdev On Sat, Jan 23, 2021 at 11:10 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 21 Jan 2021 00:17:08 -0600 Lijun Pan wrote: > > Move the dma_rmb() between pending_scrq() and ibmvnic_next_scrq() > > into the end of pending_scrq(), and explain why. > > Explain in detail why the dma_rmb() is placed at the end of > > ibmvnic_next_scrq(). > > > > Fixes: b71ec9522346 ("ibmvnic: Ensure that SCRQ entry reads are correctly ordered") > > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> > > I fail to see why this is a fix. You move the barrier from caller to > callee but there are no new barriers here. Did I miss some? I want to save the code since it is used more than twice. Maybe I should send to net-next. > > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > > index 9778c83150f1..8e043683610f 100644 > > --- a/drivers/net/ethernet/ibm/ibmvnic.c > > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > > @@ -2511,12 +2511,6 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) > > > > if (!pending_scrq(adapter, rx_scrq)) > > break; > > - /* The queue entry at the current index is peeked at above > > - * to determine that there is a valid descriptor awaiting > > - * processing. We want to be sure that the current slot > > - * holds a valid descriptor before reading its contents. > > - */ > > - dma_rmb(); > > next = ibmvnic_next_scrq(adapter, rx_scrq); > > rx_buff = > > (struct ibmvnic_rx_buff *)be64_to_cpu(next-> > > @@ -3256,13 +3250,6 @@ static int ibmvnic_complete_tx(struct ibmvnic_adapter *adapter, > > int total_bytes = 0; > > int num_packets = 0; > > > > - /* The queue entry at the current index is peeked at above > > - * to determine that there is a valid descriptor awaiting > > - * processing. We want to be sure that the current slot > > - * holds a valid descriptor before reading its contents. > > - */ > > - dma_rmb(); > > - > > next = ibmvnic_next_scrq(adapter, scrq); > > for (i = 0; i < next->tx_comp.num_comps; i++) { > > if (next->tx_comp.rcs[i]) > > @@ -3636,11 +3623,25 @@ static int pending_scrq(struct ibmvnic_adapter *adapter, > > struct ibmvnic_sub_crq_queue *scrq) > > { > > union sub_crq *entry = &scrq->msgs[scrq->cur]; > > + int rc; > > > > if (entry->generic.first & IBMVNIC_CRQ_CMD_RSP) > > - return 1; > > + rc = 1; > > else > > - return 0; > > + rc = 0; > > + > > + /* Ensure that the entire SCRQ descriptor scrq->msgs > > + * has been loaded before reading its contents. > > This comment is confusing. IMHO the comments you're removing are much > clearer. I did not quite get what fields in the data structure are supposed to be guarded from the old comment. Then I asked around and figured it out. So I updated it with the new comment, which tells specifically what the barrier protects against. > > > + * This barrier makes sure this function's entry, esp. > > + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP > > + * 1. is loaded before ibmvnic_next_scrq()'s > > + * entry->generic.first & IBMVNIC_CRQ_CMD_RSP; > > + * 2. OR is loaded before ibmvnic_poll()'s > > + * disable_scrq_irq()'s scrq->hw_irq. > > + */ > > + dma_rmb(); > > + > > + return rc; > > } > > > > static union sub_crq *ibmvnic_next_scrq(struct ibmvnic_adapter *adapter, > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net 2/3] ibmvnic: remove unnecessary rmb() inside ibmvnic_poll 2021-01-21 6:17 [PATCH net 0/3] fixes the memory barrier for SCRQ/CRQ entry Lijun Pan 2021-01-21 6:17 ` [PATCH net 1/3] ibmvnic: rework to ensure SCRQ entry reads are properly ordered Lijun Pan @ 2021-01-21 6:17 ` Lijun Pan 2021-01-24 5:09 ` Jakub Kicinski 2021-01-21 6:17 ` [PATCH net 3/3] ibmvnic: Ensure that CRQ entry read/write are correctly ordered Lijun Pan 2 siblings, 1 reply; 9+ messages in thread From: Lijun Pan @ 2021-01-21 6:17 UTC (permalink / raw) To: netdev; +Cc: Lijun Pan rmb() was introduced to load rx_scrq->msgs after calling pending_scrq(). Now since pending_scrq() itself already has dma_rmb() at the end of the function, rmb() is duplicated and can be removed. Fixes: ec20f36bb41a ("ibmvnic: Correctly re-enable interrupts in NAPI polling routine") Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 8e043683610f..933e8fb71a8b 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -2577,7 +2577,6 @@ static int ibmvnic_poll(struct napi_struct *napi, int budget) if (napi_complete_done(napi, frames_processed)) { enable_scrq_irq(adapter, rx_scrq); if (pending_scrq(adapter, rx_scrq)) { - rmb(); if (napi_reschedule(napi)) { disable_scrq_irq(adapter, rx_scrq); goto restart_poll; -- 2.23.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/3] ibmvnic: remove unnecessary rmb() inside ibmvnic_poll 2021-01-21 6:17 ` [PATCH net 2/3] ibmvnic: remove unnecessary rmb() inside ibmvnic_poll Lijun Pan @ 2021-01-24 5:09 ` Jakub Kicinski 2021-01-25 4:38 ` Lijun Pan 0 siblings, 1 reply; 9+ messages in thread From: Jakub Kicinski @ 2021-01-24 5:09 UTC (permalink / raw) To: Lijun Pan; +Cc: netdev On Thu, 21 Jan 2021 00:17:09 -0600 Lijun Pan wrote: > rmb() was introduced to load rx_scrq->msgs after calling > pending_scrq(). Now since pending_scrq() itself already > has dma_rmb() at the end of the function, rmb() is > duplicated and can be removed. > > Fixes: ec20f36bb41a ("ibmvnic: Correctly re-enable interrupts in NAPI polling routine") > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> rmb() is a stronger barrier than dma_rmb() also again, I don't see how this fixes any bugs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/3] ibmvnic: remove unnecessary rmb() inside ibmvnic_poll 2021-01-24 5:09 ` Jakub Kicinski @ 2021-01-25 4:38 ` Lijun Pan 2021-01-25 20:35 ` Jakub Kicinski 0 siblings, 1 reply; 9+ messages in thread From: Lijun Pan @ 2021-01-25 4:38 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Lijun Pan, netdev On Sat, Jan 23, 2021 at 11:11 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 21 Jan 2021 00:17:09 -0600 Lijun Pan wrote: > > rmb() was introduced to load rx_scrq->msgs after calling > > pending_scrq(). Now since pending_scrq() itself already > > has dma_rmb() at the end of the function, rmb() is > > duplicated and can be removed. > > > > Fixes: ec20f36bb41a ("ibmvnic: Correctly re-enable interrupts in NAPI polling routine") > > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> > > rmb() is a stronger barrier than dma_rmb() Yes. I think the weaker dma_rmb() here is enough. And I let it reuse the dma_rmb() in the pending_scrq(). > > also again, I don't see how this fixes any bugs I will send to net-next if you are ok with it. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/3] ibmvnic: remove unnecessary rmb() inside ibmvnic_poll 2021-01-25 4:38 ` Lijun Pan @ 2021-01-25 20:35 ` Jakub Kicinski 0 siblings, 0 replies; 9+ messages in thread From: Jakub Kicinski @ 2021-01-25 20:35 UTC (permalink / raw) To: Lijun Pan; +Cc: Lijun Pan, netdev On Sun, 24 Jan 2021 22:38:02 -0600 Lijun Pan wrote: > On Sat, Jan 23, 2021 at 11:11 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 21 Jan 2021 00:17:09 -0600 Lijun Pan wrote: > > > rmb() was introduced to load rx_scrq->msgs after calling > > > pending_scrq(). Now since pending_scrq() itself already > > > has dma_rmb() at the end of the function, rmb() is > > > duplicated and can be removed. > > > > > > Fixes: ec20f36bb41a ("ibmvnic: Correctly re-enable interrupts in NAPI polling routine") > > > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> > > > > rmb() is a stronger barrier than dma_rmb() > > Yes. I think the weaker dma_rmb() here is enough. > And I let it reuse the dma_rmb() in the pending_scrq(). > > > > > also again, I don't see how this fixes any bugs > > I will send to net-next if you are ok with it. If there is consensus at IBM that the first 2 changes are an improvement you can drop the Fixes tags and resubmit to net-next. In patch 3 it looks like the dma_rmb() may indeed be missing so that one could go to net, but I don't think the dma_wmb() is needed, especially not where you put it. I think dma_wmb() is only needed before the device is notified that new buffer was posted, not on completion. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net 3/3] ibmvnic: Ensure that CRQ entry read/write are correctly ordered 2021-01-21 6:17 [PATCH net 0/3] fixes the memory barrier for SCRQ/CRQ entry Lijun Pan 2021-01-21 6:17 ` [PATCH net 1/3] ibmvnic: rework to ensure SCRQ entry reads are properly ordered Lijun Pan 2021-01-21 6:17 ` [PATCH net 2/3] ibmvnic: remove unnecessary rmb() inside ibmvnic_poll Lijun Pan @ 2021-01-21 6:17 ` Lijun Pan 2 siblings, 0 replies; 9+ messages in thread From: Lijun Pan @ 2021-01-21 6:17 UTC (permalink / raw) To: netdev; +Cc: Lijun Pan Ensure that received Command-Response Queue (CRQ) entries are properly read/written in order by the driver. dma_rmb barrier has been added before accessing the CRQ descriptor to ensure the entire descriptor is read before processing. dma_wmb barrier is also added to ensure the entire descriptor is written before future processing. Fixes: 032c5e82847a ("Driver for IBM System i/p VNIC protocol") Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 933e8fb71a8b..e947eb068163 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -5090,8 +5090,20 @@ static void ibmvnic_tasklet(struct tasklet_struct *t) while (!done) { /* Pull all the valid messages off the CRQ */ while ((crq = ibmvnic_next_crq(adapter)) != NULL) { + /* Ensure that the entire CRQ descriptor queue->msgs + * has been loaded before reading its contents. + * This barrier makes sure ibmvnic_next_crq()'s + * crq->generic.first & IBMVNIC_CRQ_CMD_RSP is loaded + * before ibmvnic_handle_crq()'s + * switch(gen_crq->first) and switch(gen_crq->cmd). + */ + dma_rmb(); ibmvnic_handle_crq(crq, adapter); crq->generic.first = 0; + /* Ensure the entire CRQ descriptor is written before + * future writing. + */ + dma_wmb(); } /* remain in tasklet until all -- 2.23.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-25 20:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-21 6:17 [PATCH net 0/3] fixes the memory barrier for SCRQ/CRQ entry Lijun Pan 2021-01-21 6:17 ` [PATCH net 1/3] ibmvnic: rework to ensure SCRQ entry reads are properly ordered Lijun Pan 2021-01-24 5:08 ` Jakub Kicinski 2021-01-25 4:34 ` Lijun Pan 2021-01-21 6:17 ` [PATCH net 2/3] ibmvnic: remove unnecessary rmb() inside ibmvnic_poll Lijun Pan 2021-01-24 5:09 ` Jakub Kicinski 2021-01-25 4:38 ` Lijun Pan 2021-01-25 20:35 ` Jakub Kicinski 2021-01-21 6:17 ` [PATCH net 3/3] ibmvnic: Ensure that CRQ entry read/write are correctly ordered Lijun Pan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).