| Message ID | 20250305162132.1106080-6-aleksander.lobakin@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | idpf: add XDP support | expand |
On Wed, Mar 05, 2025 at 05:21:21PM +0100, Alexander Lobakin wrote: > No idea what the current barrier position was meant for. At that point, > nothing is read from the descriptor, only the pointer to the actual one > is fetched. > The correct barrier usage here is after the generation check, so that > only the first qword is read if the descriptor is not yet ready and we > need to stop polling. Debatable on coherent DMA as the Rx descriptor > size is <= cacheline size, but anyway, the current barrier position > only makes the codegen worse. Makes sense: Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> But you know the process... :P fixes should go to -net. > > Fixes: 3a8845af66ed ("idpf: add RX splitq napi poll support") > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> > --- > drivers/net/ethernet/intel/idpf/idpf_txrx.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c > index 6254806c2072..c15833928ea1 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c > @@ -3232,18 +3232,14 @@ static int idpf_rx_splitq_clean(struct idpf_rx_queue *rxq, int budget) > /* get the Rx desc from Rx queue based on 'next_to_clean' */ > rx_desc = &rxq->rx[ntc].flex_adv_nic_3_wb; > > - /* This memory barrier is needed to keep us from reading > - * any other fields out of the rx_desc > - */ > - dma_rmb(); > - > /* if the descriptor isn't done, no work yet to do */ > gen_id = le16_get_bits(rx_desc->pktlen_gen_bufq_id, > VIRTCHNL2_RX_FLEX_DESC_ADV_GEN_M); > - > if (idpf_queue_has(GEN_CHK, rxq) != gen_id) > break; > > + dma_rmb(); > + > rxdid = FIELD_GET(VIRTCHNL2_RX_FLEX_DESC_ADV_RXDID_M, > rx_desc->rxdid_ucast); > if (rxdid != VIRTCHNL2_RXDID_2_FLEX_SPLITQ) { > -- > 2.48.1 >
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Date: Fri, 7 Mar 2025 11:17:11 +0100 > On Wed, Mar 05, 2025 at 05:21:21PM +0100, Alexander Lobakin wrote: >> No idea what the current barrier position was meant for. At that point, >> nothing is read from the descriptor, only the pointer to the actual one >> is fetched. >> The correct barrier usage here is after the generation check, so that >> only the first qword is read if the descriptor is not yet ready and we >> need to stop polling. Debatable on coherent DMA as the Rx descriptor >> size is <= cacheline size, but anyway, the current barrier position >> only makes the codegen worse. > > Makes sense: > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > But you know the process... :P fixes should go to -net. The thing's that it makes no difference for regular skb Rx, but with ret != XDP_PASS it starts making issues. So yes, this is a fix, but I don't think it should go separately. > >> >> Fixes: 3a8845af66ed ("idpf: add RX splitq napi poll support") >> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> >> --- >> drivers/net/ethernet/intel/idpf/idpf_txrx.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) Thanks, Olek
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c index 6254806c2072..c15833928ea1 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c @@ -3232,18 +3232,14 @@ static int idpf_rx_splitq_clean(struct idpf_rx_queue *rxq, int budget) /* get the Rx desc from Rx queue based on 'next_to_clean' */ rx_desc = &rxq->rx[ntc].flex_adv_nic_3_wb; - /* This memory barrier is needed to keep us from reading - * any other fields out of the rx_desc - */ - dma_rmb(); - /* if the descriptor isn't done, no work yet to do */ gen_id = le16_get_bits(rx_desc->pktlen_gen_bufq_id, VIRTCHNL2_RX_FLEX_DESC_ADV_GEN_M); - if (idpf_queue_has(GEN_CHK, rxq) != gen_id) break; + dma_rmb(); + rxdid = FIELD_GET(VIRTCHNL2_RX_FLEX_DESC_ADV_RXDID_M, rx_desc->rxdid_ucast); if (rxdid != VIRTCHNL2_RXDID_2_FLEX_SPLITQ) {
No idea what the current barrier position was meant for. At that point, nothing is read from the descriptor, only the pointer to the actual one is fetched. The correct barrier usage here is after the generation check, so that only the first qword is read if the descriptor is not yet ready and we need to stop polling. Debatable on coherent DMA as the Rx descriptor size is <= cacheline size, but anyway, the current barrier position only makes the codegen worse. Fixes: 3a8845af66ed ("idpf: add RX splitq napi poll support") Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> --- drivers/net/ethernet/intel/idpf/idpf_txrx.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)