[PATCH 0/3][SRU Bionic] hns3: avoid corruption due to ring buffer race

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3][SRU Bionic] hns3: avoid corruption due to ring buffer race

dann frazier-4
BugLink: https://bugs.launchpad.net/bugs/1840717

All clean cherry picks. Patch #1 is a small cleanup that allows Patch #2
to apply cleanly. Patch #3 improves the performance of Patch #2. All 3
patches were introduced in v5.2, so they've had plenty of "bake time"
together.


Yunsheng Lin (3):
  net: hns3: minor optimization for ring_space
  net: hns3: fix data race between ring->next_to_clean
  net: hns3: optimize the barrier using when cleaning TX BD

 .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 35 ++++++++++++-------
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   | 18 +++++-----
 2 files changed, 30 insertions(+), 23 deletions(-)

--
2.23.0.rc1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3][SRU Bionic] net: hns3: minor optimization for ring_space

dann frazier-4
From: Yunsheng Lin <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1840717

This patch optimizes the ring_space by calculating the
ring space without calling ring_dist.

Also ring_dist is only used by ring_space, so this patch
removes it when it is no longer used.

Signed-off-by: Yunsheng Lin <[hidden email]>
Signed-off-by: Peng Li <[hidden email]>
Signed-off-by: Huazhong Tan <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(cherry picked from commit 0aa3d88a9197fd7176dbaf5db769837be6afdf46)
Signed-off-by: dann frazier <[hidden email]>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.h | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
index e55995e93bb08..77f6e6cf3fead 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
@@ -570,18 +570,13 @@ union l4_hdr_info {
  unsigned char *hdr;
 };
 
-/* the distance between [begin, end) in a ring buffer
- * note: there is a unuse slot between the begin and the end
- */
-static inline int ring_dist(struct hns3_enet_ring *ring, int begin, int end)
-{
- return (end - begin + ring->desc_num) % ring->desc_num;
-}
-
 static inline int ring_space(struct hns3_enet_ring *ring)
 {
- return ring->desc_num -
- ring_dist(ring, ring->next_to_clean, ring->next_to_use) - 1;
+ int begin = ring->next_to_clean;
+ int end = ring->next_to_use;
+
+ return ((end >= begin) ? (ring->desc_num - end + begin) :
+ (begin - end)) - 1;
 }
 
 static inline int is_ring_empty(struct hns3_enet_ring *ring)
--
2.23.0.rc1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3][SRU Bionic] net: hns3: fix data race between ring->next_to_clean

dann frazier-4
In reply to this post by dann frazier-4
From: Yunsheng Lin <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1840717

hns3_clean_tx_ring calls hns3_nic_reclaim_one_desc to clean
buffers and set ring->next_to_clean, then hns3_nic_net_xmit
reuses the cleaned buffers. But there are no memory barriers
when buffers gets recycled, so the recycled buffers can be
corrupted.

This patch uses smp_store_release to update ring->next_to_clean
and smp_load_acquire to read ring->next_to_clean to properly
hand off buffers from hns3_clean_tx_ring to hns3_nic_net_xmit.

Fixes: 76ad4f0ee747 ("net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC")
Signed-off-by: Yunsheng Lin <[hidden email]>
Signed-off-by: Peng Li <[hidden email]>
Signed-off-by: Huazhong Tan <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(cherry picked from commit 26cda2f1613878d9bde11325559f4fca92fff395)
Signed-off-by: dann frazier <[hidden email]>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 14 +++++++++++---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.h |  7 +++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 08718f9093dc5..99815c45c9a8f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2112,14 +2112,22 @@ static void hns3_reuse_buffer(struct hns3_enet_ring *ring, int i)
 static void hns3_nic_reclaim_one_desc(struct hns3_enet_ring *ring, int *bytes,
       int *pkts)
 {
- struct hns3_desc_cb *desc_cb = &ring->desc_cb[ring->next_to_clean];
+ int ntc = ring->next_to_clean;
+ struct hns3_desc_cb *desc_cb;
 
+ desc_cb = &ring->desc_cb[ntc];
  (*pkts) += (desc_cb->type == DESC_TYPE_SKB);
  (*bytes) += desc_cb->length;
  /* desc_cb will be cleaned, after hnae3_free_buffer_detach*/
- hns3_free_buffer_detach(ring, ring->next_to_clean);
+ hns3_free_buffer_detach(ring, ntc);
 
- ring_ptr_move_fw(ring, next_to_clean);
+ if (++ntc == ring->desc_num)
+ ntc = 0;
+
+ /* This smp_store_release() pairs with smp_load_acquire() in
+ * ring_space called by hns3_nic_net_xmit.
+ */
+ smp_store_release(&ring->next_to_clean, ntc);
 }
 
 static int is_valid_clean_head(struct hns3_enet_ring *ring, int h)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
index 77f6e6cf3fead..0553022ad6db4 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
@@ -572,8 +572,11 @@ union l4_hdr_info {
 
 static inline int ring_space(struct hns3_enet_ring *ring)
 {
- int begin = ring->next_to_clean;
- int end = ring->next_to_use;
+ /* This smp_load_acquire() pairs with smp_store_release() in
+ * hns3_nic_reclaim_one_desc called by hns3_clean_tx_ring.
+ */
+ int begin = smp_load_acquire(&ring->next_to_clean);
+ int end = READ_ONCE(ring->next_to_use);
 
  return ((end >= begin) ? (ring->desc_num - end + begin) :
  (begin - end)) - 1;
--
2.23.0.rc1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3][SRU Bionic] net: hns3: optimize the barrier using when cleaning TX BD

dann frazier-4
In reply to this post by dann frazier-4
From: Yunsheng Lin <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1840717

Currently, a barrier is used when cleaning each TX BD, which may
cause performance degradation.

This patch optimizes it to use one barrier when cleaning TX BD
each round.

Signed-off-by: Yunsheng Lin <[hidden email]>
Signed-off-by: Huazhong Tan <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(cherry picked from commit ce74370c2ce9a90c16167131f837e14b5e3c57ed)
Signed-off-by: dann frazier <[hidden email]>
---
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 99815c45c9a8f..a7fe58f020d12 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2109,20 +2109,25 @@ static void hns3_reuse_buffer(struct hns3_enet_ring *ring, int i)
  ring->desc[i].rx.bd_base_info = 0;
 }
 
-static void hns3_nic_reclaim_one_desc(struct hns3_enet_ring *ring, int *bytes,
-      int *pkts)
+static void hns3_nic_reclaim_desc(struct hns3_enet_ring *ring, int head,
+  int *bytes, int *pkts)
 {
  int ntc = ring->next_to_clean;
  struct hns3_desc_cb *desc_cb;
 
- desc_cb = &ring->desc_cb[ntc];
- (*pkts) += (desc_cb->type == DESC_TYPE_SKB);
- (*bytes) += desc_cb->length;
- /* desc_cb will be cleaned, after hnae3_free_buffer_detach*/
- hns3_free_buffer_detach(ring, ntc);
+ while (head != ntc) {
+ desc_cb = &ring->desc_cb[ntc];
+ (*pkts) += (desc_cb->type == DESC_TYPE_SKB);
+ (*bytes) += desc_cb->length;
+ /* desc_cb will be cleaned, after hnae3_free_buffer_detach */
+ hns3_free_buffer_detach(ring, ntc);
 
- if (++ntc == ring->desc_num)
- ntc = 0;
+ if (++ntc == ring->desc_num)
+ ntc = 0;
+
+ /* Issue prefetch for next Tx descriptor */
+ prefetch(&ring->desc_cb[ntc]);
+ }
 
  /* This smp_store_release() pairs with smp_load_acquire() in
  * ring_space called by hns3_nic_net_xmit.
@@ -2167,11 +2172,7 @@ void hns3_clean_tx_ring(struct hns3_enet_ring *ring)
 
  bytes = 0;
  pkts = 0;
- while (head != ring->next_to_clean) {
- hns3_nic_reclaim_one_desc(ring, &bytes, &pkts);
- /* Issue prefetch for next Tx descriptor */
- prefetch(&ring->desc_cb[ring->next_to_clean]);
- }
+ hns3_nic_reclaim_desc(ring, head, &bytes, &pkts);
 
  ring->tqp_vector->tx_group.total_bytes += bytes;
  ring->tqp_vector->tx_group.total_packets += pkts;
--
2.23.0.rc1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/3][SRU Bionic] hns3: avoid corruption due to ring buffer race

Stefan Bader-2
In reply to this post by dann frazier-4
On 20.08.19 00:11, dann frazier wrote:

> BugLink: https://bugs.launchpad.net/bugs/1840717
>
> All clean cherry picks. Patch #1 is a small cleanup that allows Patch #2
> to apply cleanly. Patch #3 improves the performance of Patch #2. All 3
> patches were introduced in v5.2, so they've had plenty of "bake time"
> together.
>
>
> Yunsheng Lin (3):
>   net: hns3: minor optimization for ring_space
>   net: hns3: fix data race between ring->next_to_clean
>   net: hns3: optimize the barrier using when cleaning TX BD
>
>  .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 35 ++++++++++++-------
>  .../net/ethernet/hisilicon/hns3/hns3_enet.h   | 18 +++++-----
>  2 files changed, 30 insertions(+), 23 deletions(-)
>
Acked-by: Stefan Bader <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/3][SRU Bionic] hns3: avoid corruption due to ring buffer race

Connor Kuehl
In reply to this post by dann frazier-4
On 8/19/19 3:11 PM, dann frazier wrote:

> BugLink: https://bugs.launchpad.net/bugs/1840717
>
> All clean cherry picks. Patch #1 is a small cleanup that allows Patch #2
> to apply cleanly. Patch #3 improves the performance of Patch #2. All 3
> patches were introduced in v5.2, so they've had plenty of "bake time"
> together.
>
>
> Yunsheng Lin (3):
>    net: hns3: minor optimization for ring_space
>    net: hns3: fix data race between ring->next_to_clean
>    net: hns3: optimize the barrier using when cleaning TX BD
>
>   .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 35 ++++++++++++-------
>   .../net/ethernet/hisilicon/hns3/hns3_enet.h   | 18 +++++-----
>   2 files changed, 30 insertions(+), 23 deletions(-)
>

Acked-by: Connor Kuehl <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [PATCH 0/3][SRU Bionic] hns3: avoid corruption due to ring buffer race

Kleber Souza
In reply to this post by dann frazier-4
On 8/20/19 12:11 AM, dann frazier wrote:

> BugLink: https://bugs.launchpad.net/bugs/1840717
>
> All clean cherry picks. Patch #1 is a small cleanup that allows Patch #2
> to apply cleanly. Patch #3 improves the performance of Patch #2. All 3
> patches were introduced in v5.2, so they've had plenty of "bake time"
> together.
>
>
> Yunsheng Lin (3):
>   net: hns3: minor optimization for ring_space
>   net: hns3: fix data race between ring->next_to_clean
>   net: hns3: optimize the barrier using when cleaning TX BD
>
>  .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 35 ++++++++++++-------
>  .../net/ethernet/hisilicon/hns3/hns3_enet.h   | 18 +++++-----
>  2 files changed, 30 insertions(+), 23 deletions(-)
>

Applied to bionic/master-next branch.

Thanks,
Kleber

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team