| Message ID | 20200810210832.34699-3-andre.guedes@intel.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/3] igc: Clean RX descriptor error flags | expand |
On Mon, Aug 10, 2020 at 2:08 PM Andre Guedes <andre.guedes@intel.com> wrote: > > SRRCTL register is set with 'one buffer descriptor' option (see DESCTYPE > setting a few lines below) so setting BSIZEHEADER bits is pointless. > They should be zero. Also, since there is no header buffer we should set > the header buffer address field from the receive descriptor to zero for > the sake of consistency. > > Signed-off-by: Andre Guedes <andre.guedes@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 0c481dc906ad..a5d825d44002 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -531,14 +531,11 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter, > ring->next_to_clean = 0; > ring->next_to_use = 0; > > - /* set descriptor configuration */ > - srrctl = IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT; > if (ring_uses_large_buffer(ring)) > srrctl |= IGC_RXBUFFER_3072 >> IGC_SRRCTL_BSIZEPKT_SHIFT; > else > srrctl |= IGC_RXBUFFER_2048 >> IGC_SRRCTL_BSIZEPKT_SHIFT; > srrctl |= IGC_SRRCTL_DESCTYPE_ADV_ONEBUF; > - > wr32(IGC_SRRCTL(reg_idx), srrctl); > > rxdctl |= IGC_RX_PTHRESH; Some of this was left in place to leave parity with the ixgbe driver which was required to populate that field in order to enable RSC/LRO. > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count) > * because each write-back erases this info. > */ > rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset); > + rx_desc->read.hdr_addr = 0; > > rx_desc++; > bi++; If you are going to do this it would be better to replace the line that is setting the length to zero instead of just adding this line. That way you can avoid having to rewrite it. I only had bothered with clearing the length field as it was a 32b field, however if you are wanting to flush the full 64b then I would recommend doing it there rather than here.
Quoting Alexander Duyck (2020-08-10 15:56:31) > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count) > > * because each write-back erases this info. > > */ > > rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset); > > + rx_desc->read.hdr_addr = 0; > > > > rx_desc++; > > bi++; > > If you are going to do this it would be better to replace the line > that is setting the length to zero instead of just adding this line. > That way you can avoid having to rewrite it. I only had bothered with > clearing the length field as it was a 32b field, however if you are > wanting to flush the full 64b then I would recommend doing it there > rather than here. Just to make sure I'm on the same page, do you mean to move this line to patch 2/3, right?
On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <andre.guedes@intel.com> wrote: > > Quoting Alexander Duyck (2020-08-10 15:56:31) > > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count) > > > * because each write-back erases this info. > > > */ > > > rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset); > > > + rx_desc->read.hdr_addr = 0; > > > > > > rx_desc++; > > > bi++; > > > > If you are going to do this it would be better to replace the line > > that is setting the length to zero instead of just adding this line. > > That way you can avoid having to rewrite it. I only had bothered with > > clearing the length field as it was a 32b field, however if you are > > wanting to flush the full 64b then I would recommend doing it there > > rather than here. > > Just to make sure I'm on the same page, do you mean to move this line to > patch 2/3, right? No, I hadn't had a chance to take a look at patch 2 yet. I think patch 2 is ill advised as the patch is currently broken, and even if done correctly you don't get any benefit out of it that I can see. From what I can tell this patch was likely covering up some of the errors introduced in patch 2. Now that I see this in conjunction with patch 2 I would say you should probably just drop patch 2 and this one as well since they aren't adding any real value other than removing a redundant write which was just there to keep this mostly in sync with how we did it for ixgbe. The reason the driver path was coded the way it is in order to get away from having to clear the entire descriptor after processing it and to avoid having to explicitly track next_to_use and next_to_clean. Instead we leave the descriptor as mostly read-only until we reallocate the buffer and give it back to the device. All we have to do is clear the length field of the next_to_use descriptor when we are done allocating so that we do not overrun the descriptors when cleaning. It makes it much easier to debug when the descriptors are left in place as long as possible since we can then come back and look at the memory and generally I have found performance is improved as we are not having to dirty cachelines prematurely.
Quoting Alexander Duyck (2020-08-10 18:41:41) > On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <andre.guedes@intel.com> wrote: > > > > Quoting Alexander Duyck (2020-08-10 15:56:31) > > > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count) > > > > * because each write-back erases this info. > > > > */ > > > > rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset); > > > > + rx_desc->read.hdr_addr = 0; > > > > > > > > rx_desc++; > > > > bi++; > > > > > > If you are going to do this it would be better to replace the line > > > that is setting the length to zero instead of just adding this line. > > > That way you can avoid having to rewrite it. I only had bothered with > > > clearing the length field as it was a 32b field, however if you are > > > wanting to flush the full 64b then I would recommend doing it there > > > rather than here. > > > > Just to make sure I'm on the same page, do you mean to move this line to > > patch 2/3, right? > > No, I hadn't had a chance to take a look at patch 2 yet. I think patch > 2 is ill advised as the patch is currently broken, and even if done > correctly you don't get any benefit out of it that I can see. From > what I can tell this patch was likely covering up some of the errors > introduced in patch 2. Now that I see this in conjunction with patch 2 > I would say you should probably just drop patch 2 and this one as well > since they aren't adding any real value other than removing a > redundant write which was just there to keep this mostly in sync with > how we did it for ixgbe. What about not setting BSIZEHEADER bits since 'one buffer descriptor' option is used in SRRCTL? > The reason the driver path was coded the way it is in order to get > away from having to clear the entire descriptor after processing it > and to avoid having to explicitly track next_to_use and next_to_clean. > Instead we leave the descriptor as mostly read-only until we > reallocate the buffer and give it back to the device. All we have to > do is clear the length field of the next_to_use descriptor when we are > done allocating so that we do not overrun the descriptors when > cleaning. It makes it much easier to debug when the descriptors are > left in place as long as possible since we can then come back and look > at the memory and generally I have found performance is improved as we > are not having to dirty cachelines prematurely. Thanks for the context. - Andre
On Tue, Aug 11, 2020 at 10:00 AM Andre Guedes <andre.guedes@intel.com> wrote: > > Quoting Alexander Duyck (2020-08-10 18:41:41) > > On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <andre.guedes@intel.com> wrote: > > > > > > Quoting Alexander Duyck (2020-08-10 15:56:31) > > > > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count) > > > > > * because each write-back erases this info. > > > > > */ > > > > > rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset); > > > > > + rx_desc->read.hdr_addr = 0; > > > > > > > > > > rx_desc++; > > > > > bi++; > > > > > > > > If you are going to do this it would be better to replace the line > > > > that is setting the length to zero instead of just adding this line. > > > > That way you can avoid having to rewrite it. I only had bothered with > > > > clearing the length field as it was a 32b field, however if you are > > > > wanting to flush the full 64b then I would recommend doing it there > > > > rather than here. > > > > > > Just to make sure I'm on the same page, do you mean to move this line to > > > patch 2/3, right? > > > > No, I hadn't had a chance to take a look at patch 2 yet. I think patch > > 2 is ill advised as the patch is currently broken, and even if done > > correctly you don't get any benefit out of it that I can see. From > > what I can tell this patch was likely covering up some of the errors > > introduced in patch 2. Now that I see this in conjunction with patch 2 > > I would say you should probably just drop patch 2 and this one as well > > since they aren't adding any real value other than removing a > > redundant write which was just there to keep this mostly in sync with > > how we did it for ixgbe. > > What about not setting BSIZEHEADER bits since 'one buffer descriptor' option > is used in SRRCTL? Does it cause some sort of problem to be populating it? If not I would say leave it. It isn't too different from just populating the field with 0 and should have no effect since the field is unused when in one buffer mode. > > The reason the driver path was coded the way it is in order to get > > away from having to clear the entire descriptor after processing it > > and to avoid having to explicitly track next_to_use and next_to_clean. > > Instead we leave the descriptor as mostly read-only until we > > reallocate the buffer and give it back to the device. All we have to > > do is clear the length field of the next_to_use descriptor when we are > > done allocating so that we do not overrun the descriptors when > > cleaning. It makes it much easier to debug when the descriptors are > > left in place as long as possible since we can then come back and look > > at the memory and generally I have found performance is improved as we > > are not having to dirty cachelines prematurely. > > Thanks for the context. > > - Andre
Quoting Alexander Duyck (2020-08-11 11:04:52) > On Tue, Aug 11, 2020 at 10:00 AM Andre Guedes <andre.guedes@intel.com> wrote: > > > > Quoting Alexander Duyck (2020-08-10 18:41:41) > > > On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <andre.guedes@intel.com> wrote: > > > > > > > > Quoting Alexander Duyck (2020-08-10 15:56:31) > > > > > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count) > > > > > > * because each write-back erases this info. > > > > > > */ > > > > > > rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset); > > > > > > + rx_desc->read.hdr_addr = 0; > > > > > > > > > > > > rx_desc++; > > > > > > bi++; > > > > > > > > > > If you are going to do this it would be better to replace the line > > > > > that is setting the length to zero instead of just adding this line. > > > > > That way you can avoid having to rewrite it. I only had bothered with > > > > > clearing the length field as it was a 32b field, however if you are > > > > > wanting to flush the full 64b then I would recommend doing it there > > > > > rather than here. > > > > > > > > Just to make sure I'm on the same page, do you mean to move this line to > > > > patch 2/3, right? > > > > > > No, I hadn't had a chance to take a look at patch 2 yet. I think patch > > > 2 is ill advised as the patch is currently broken, and even if done > > > correctly you don't get any benefit out of it that I can see. From > > > what I can tell this patch was likely covering up some of the errors > > > introduced in patch 2. Now that I see this in conjunction with patch 2 > > > I would say you should probably just drop patch 2 and this one as well > > > since they aren't adding any real value other than removing a > > > redundant write which was just there to keep this mostly in sync with > > > how we did it for ixgbe. > > > > What about not setting BSIZEHEADER bits since 'one buffer descriptor' option > > is used in SRRCTL? > > Does it cause some sort of problem to be populating it? If not I would > say leave it. It isn't too different from just populating the field > with 0 and should have no effect since the field is unused when in one > buffer mode. No problem that I'm aware of. When I first came across that code I found it a bit misleading since no header buffer is actually allocated by the driver. That was the only motivation for this patch. So Jeff/Tony please disregard patches 2 and 3 from this series. Let's move forward with patch 1 only. Thanks for the review, Alexander. - Andre
On Tue, 2020-08-11 at 11:38 -0700, Andre Guedes wrote: > Quoting Alexander Duyck (2020-08-11 11:04:52) > > On Tue, Aug 11, 2020 at 10:00 AM Andre Guedes < > > andre.guedes@intel.com> wrote: > > > > > > Quoting Alexander Duyck (2020-08-10 18:41:41) > > > > On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes < > > > > andre.guedes@intel.com> wrote: > > > > > > > > > > Quoting Alexander Duyck (2020-08-10 15:56:31) > > > > > > > @@ -1869,6 +1866,7 @@ static void > > > > > > > igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 > > > > > > > cleaned_count) > > > > > > > * because each write-back erases this > > > > > > > info. > > > > > > > */ > > > > > > > rx_desc->read.pkt_addr = cpu_to_le64(bi- > > > > > > > >dma + bi->page_offset); > > > > > > > + rx_desc->read.hdr_addr = 0; > > > > > > > > > > > > > > rx_desc++; > > > > > > > bi++; > > > > > > > > > > > > If you are going to do this it would be better to replace > > > > > > the line > > > > > > that is setting the length to zero instead of just adding > > > > > > this line. > > > > > > That way you can avoid having to rewrite it. I only had > > > > > > bothered with > > > > > > clearing the length field as it was a 32b field, however if > > > > > > you are > > > > > > wanting to flush the full 64b then I would recommend doing > > > > > > it there > > > > > > rather than here. > > > > > > > > > > Just to make sure I'm on the same page, do you mean to move > > > > > this line to > > > > > patch 2/3, right? > > > > > > > > No, I hadn't had a chance to take a look at patch 2 yet. I > > > > think patch > > > > 2 is ill advised as the patch is currently broken, and even if > > > > done > > > > correctly you don't get any benefit out of it that I can see. > > > > From > > > > what I can tell this patch was likely covering up some of the > > > > errors > > > > introduced in patch 2. Now that I see this in conjunction with > > > > patch 2 > > > > I would say you should probably just drop patch 2 and this one > > > > as well > > > > since they aren't adding any real value other than removing a > > > > redundant write which was just there to keep this mostly in > > > > sync with > > > > how we did it for ixgbe. > > > > > > What about not setting BSIZEHEADER bits since 'one buffer > > > descriptor' option > > > is used in SRRCTL? > > > > Does it cause some sort of problem to be populating it? If not I > > would > > say leave it. It isn't too different from just populating the field > > with 0 and should have no effect since the field is unused when in > > one > > buffer mode. > > No problem that I'm aware of. When I first came across that code I > found it a > bit misleading since no header buffer is actually allocated by the > driver. That > was the only motivation for this patch. > > So Jeff/Tony please disregard patches 2 and 3 from this series. Let's > move forward with > patch 1 only. I will apply patch 1 and drop the other 2. Thanks, Tony
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 0c481dc906ad..a5d825d44002 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -531,14 +531,11 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter, ring->next_to_clean = 0; ring->next_to_use = 0; - /* set descriptor configuration */ - srrctl = IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT; if (ring_uses_large_buffer(ring)) srrctl |= IGC_RXBUFFER_3072 >> IGC_SRRCTL_BSIZEPKT_SHIFT; else srrctl |= IGC_RXBUFFER_2048 >> IGC_SRRCTL_BSIZEPKT_SHIFT; srrctl |= IGC_SRRCTL_DESCTYPE_ADV_ONEBUF; - wr32(IGC_SRRCTL(reg_idx), srrctl); rxdctl |= IGC_RX_PTHRESH; @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count) * because each write-back erases this info. */ rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset); + rx_desc->read.hdr_addr = 0; rx_desc++; bi++;
SRRCTL register is set with 'one buffer descriptor' option (see DESCTYPE setting a few lines below) so setting BSIZEHEADER bits is pointless. They should be zero. Also, since there is no header buffer we should set the header buffer address field from the receive descriptor to zero for the sake of consistency. Signed-off-by: Andre Guedes <andre.guedes@intel.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)