| Message ID | 1600085217-26245-7-git-send-email-tanhuazhong@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: hns3: updates for -next | expand |
On Mon, 2020-09-14 at 20:06 +0800, Huazhong Tan wrote: > From: Yunsheng Lin <linyunsheng@huawei.com> > > Use napi_consume_skb() to batch consuming skb when cleaning > tx desc in NAPI polling. > > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com> > --- > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 27 +++++++++++- > ---------- > drivers/net/ethernet/hisilicon/hns3/hns3_enet.h | 2 +- > drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 4 ++-- > 3 files changed, 17 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > index 4a49a76..feeaf75 100644 > --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > @@ -2333,10 +2333,10 @@ static int hns3_alloc_buffer(struct > hns3_enet_ring *ring, > } > > static void hns3_free_buffer(struct hns3_enet_ring *ring, > - struct hns3_desc_cb *cb) > + struct hns3_desc_cb *cb, int budget) > { > if (cb->type == DESC_TYPE_SKB) > - dev_kfree_skb_any((struct sk_buff *)cb->priv); > + napi_consume_skb(cb->priv, budget); This code can be reached from hns3_lb_clear_tx_ring() below which is your loopback test and called with non-zero budget, I am not sure you are allowed to call napi_consume_skb() with non-zero budget outside napi context, perhaps the cb->type for loopback test is different in lb test case ? Idk.. , please double check other code paths. [...] > static void hns3_lb_clear_tx_ring(struct hns3_nic_priv *priv, u32 > start_ringid, > - u32 end_ringid, u32 budget) > + u32 end_ringid, int budget) > { > u32 i; > > for (i = start_ringid; i <= end_ringid; i++) { > struct hns3_enet_ring *ring = &priv->ring[i]; > > - hns3_clean_tx_ring(ring); > + hns3_clean_tx_ring(ring, budget); > } > } >
On 2020/9/15 13:09, Saeed Mahameed wrote: > On Mon, 2020-09-14 at 20:06 +0800, Huazhong Tan wrote: >> From: Yunsheng Lin <linyunsheng@huawei.com> >> >> Use napi_consume_skb() to batch consuming skb when cleaning >> tx desc in NAPI polling. >> >> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> >> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com> >> --- >> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 27 +++++++++++- >> ---------- >> drivers/net/ethernet/hisilicon/hns3/hns3_enet.h | 2 +- >> drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 4 ++-- >> 3 files changed, 17 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >> index 4a49a76..feeaf75 100644 >> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >> @@ -2333,10 +2333,10 @@ static int hns3_alloc_buffer(struct >> hns3_enet_ring *ring, >> } >> >> static void hns3_free_buffer(struct hns3_enet_ring *ring, >> - struct hns3_desc_cb *cb) >> + struct hns3_desc_cb *cb, int budget) >> { >> if (cb->type == DESC_TYPE_SKB) >> - dev_kfree_skb_any((struct sk_buff *)cb->priv); >> + napi_consume_skb(cb->priv, budget); > > This code can be reached from hns3_lb_clear_tx_ring() below which is > your loopback test and called with non-zero budget, I am not sure you > are allowed to call napi_consume_skb() with non-zero budget outside > napi context, perhaps the cb->type for loopback test is different in lb > test case ? Idk.. , please double check other code paths. Yes, loopback test may call napi_consume_skb() with non-zero budget outside napi context. Thanks for pointing out this case. How about add the below WARN_ONCE() in napi_consume_skb() to catch this kind of error? WARN_ONCE(!in_serving_softirq(), "napi_consume_skb() is called with non-zero budget outside napi context"); > > [...] > >> static void hns3_lb_clear_tx_ring(struct hns3_nic_priv *priv, u32 >> start_ringid, >> - u32 end_ringid, u32 budget) >> + u32 end_ringid, int budget) >> { >> u32 i; >> >> for (i = start_ringid; i <= end_ringid; i++) { >> struct hns3_enet_ring *ring = &priv->ring[i]; >> >> - hns3_clean_tx_ring(ring); >> + hns3_clean_tx_ring(ring, budget); >> } >> } >>
On Tue, 2020-09-15 at 15:04 +0800, Yunsheng Lin wrote: > On 2020/9/15 13:09, Saeed Mahameed wrote: > > On Mon, 2020-09-14 at 20:06 +0800, Huazhong Tan wrote: > > > From: Yunsheng Lin <linyunsheng@huawei.com> > > > > > > Use napi_consume_skb() to batch consuming skb when cleaning > > > tx desc in NAPI polling. > > > > > > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > > > Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com> > > > --- > > > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 27 > > > +++++++++++- > > > ---------- > > > drivers/net/ethernet/hisilicon/hns3/hns3_enet.h | 2 +- > > > drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 4 ++-- > > > 3 files changed, 17 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > index 4a49a76..feeaf75 100644 > > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > @@ -2333,10 +2333,10 @@ static int hns3_alloc_buffer(struct > > > hns3_enet_ring *ring, > > > } > > > > > > static void hns3_free_buffer(struct hns3_enet_ring *ring, > > > - struct hns3_desc_cb *cb) > > > + struct hns3_desc_cb *cb, int budget) > > > { > > > if (cb->type == DESC_TYPE_SKB) > > > - dev_kfree_skb_any((struct sk_buff *)cb->priv); > > > + napi_consume_skb(cb->priv, budget); > > > > This code can be reached from hns3_lb_clear_tx_ring() below which > > is > > your loopback test and called with non-zero budget, I am not sure > > you > > are allowed to call napi_consume_skb() with non-zero budget outside > > napi context, perhaps the cb->type for loopback test is different > > in lb > > test case ? Idk.. , please double check other code paths. > > Yes, loopback test may call napi_consume_skb() with non-zero budget > outside > napi context. Thanks for pointing out this case. > > How about add the below WARN_ONCE() in napi_consume_skb() to catch > this > kind of error? > > WARN_ONCE(!in_serving_softirq(), "napi_consume_skb() is called with > non-zero budget outside napi context"); > Cc: Eric I don't know, need to check performance impact. And in_serving_softirq() doesn't necessarily mean in napi but looking at _kfree_skb_defer(), i think it shouldn't care if napi or not as long as it runs in soft irq it will push the skb to that particular cpu napi_alloc_cache, which should be fine. Maybe instead of the WARN_ONCE just remove the budget condition and replace it with if (!in_serving_softirq()) dev_consume_skb_any(skb);
On Wed, Sep 16, 2020 at 10:33 AM Saeed Mahameed <saeed@kernel.org> wrote: > > On Tue, 2020-09-15 at 15:04 +0800, Yunsheng Lin wrote: > > On 2020/9/15 13:09, Saeed Mahameed wrote: > > > On Mon, 2020-09-14 at 20:06 +0800, Huazhong Tan wrote: > > > > From: Yunsheng Lin <linyunsheng@huawei.com> > > > > > > > > Use napi_consume_skb() to batch consuming skb when cleaning > > > > tx desc in NAPI polling. > > > > > > > > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > > > > Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com> > > > > --- > > > > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 27 > > > > +++++++++++- > > > > ---------- > > > > drivers/net/ethernet/hisilicon/hns3/hns3_enet.h | 2 +- > > > > drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 4 ++-- > > > > 3 files changed, 17 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > > b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > > index 4a49a76..feeaf75 100644 > > > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > > @@ -2333,10 +2333,10 @@ static int hns3_alloc_buffer(struct > > > > hns3_enet_ring *ring, > > > > } > > > > > > > > static void hns3_free_buffer(struct hns3_enet_ring *ring, > > > > - struct hns3_desc_cb *cb) > > > > + struct hns3_desc_cb *cb, int budget) > > > > { > > > > if (cb->type == DESC_TYPE_SKB) > > > > - dev_kfree_skb_any((struct sk_buff *)cb->priv); > > > > + napi_consume_skb(cb->priv, budget); > > > > > > This code can be reached from hns3_lb_clear_tx_ring() below which > > > is > > > your loopback test and called with non-zero budget, I am not sure > > > you > > > are allowed to call napi_consume_skb() with non-zero budget outside > > > napi context, perhaps the cb->type for loopback test is different > > > in lb > > > test case ? Idk.. , please double check other code paths. > > > > Yes, loopback test may call napi_consume_skb() with non-zero budget > > outside > > napi context. Thanks for pointing out this case. > > > > How about add the below WARN_ONCE() in napi_consume_skb() to catch > > this > > kind of error? > > > > WARN_ONCE(!in_serving_softirq(), "napi_consume_skb() is called with > > non-zero budget outside napi context"); > > > > Cc: Eric > > I don't know, need to check performance impact. > And in_serving_softirq() doesn't necessarily mean in napi > but looking at _kfree_skb_defer(), i think it shouldn't care if napi or > not as long as it runs in soft irq it will push the skb to that > particular cpu napi_alloc_cache, which should be fine. > > Maybe instead of the WARN_ONCE just remove the budget condition and > replace it with > > if (!in_serving_softirq()) > dev_consume_skb_any(skb); > I think we need to keep costs small. So lets add a CONFIG_DEBUG_NET option so that developers can add various DEBUG_NET() clauses.
On 2020/9/16 16:38, Eric Dumazet wrote: > On Wed, Sep 16, 2020 at 10:33 AM Saeed Mahameed <saeed@kernel.org> wrote: >> >> On Tue, 2020-09-15 at 15:04 +0800, Yunsheng Lin wrote: >>> On 2020/9/15 13:09, Saeed Mahameed wrote: >>>> On Mon, 2020-09-14 at 20:06 +0800, Huazhong Tan wrote: >>>>> From: Yunsheng Lin <linyunsheng@huawei.com> >>>>> >>>>> Use napi_consume_skb() to batch consuming skb when cleaning >>>>> tx desc in NAPI polling. >>>>> >>>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> >>>>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com> >>>>> --- >>>>> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 27 >>>>> +++++++++++- >>>>> ---------- >>>>> drivers/net/ethernet/hisilicon/hns3/hns3_enet.h | 2 +- >>>>> drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 4 ++-- >>>>> 3 files changed, 17 insertions(+), 16 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >>>>> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >>>>> index 4a49a76..feeaf75 100644 >>>>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >>>>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >>>>> @@ -2333,10 +2333,10 @@ static int hns3_alloc_buffer(struct >>>>> hns3_enet_ring *ring, >>>>> } >>>>> >>>>> static void hns3_free_buffer(struct hns3_enet_ring *ring, >>>>> - struct hns3_desc_cb *cb) >>>>> + struct hns3_desc_cb *cb, int budget) >>>>> { >>>>> if (cb->type == DESC_TYPE_SKB) >>>>> - dev_kfree_skb_any((struct sk_buff *)cb->priv); >>>>> + napi_consume_skb(cb->priv, budget); >>>> >>>> This code can be reached from hns3_lb_clear_tx_ring() below which >>>> is >>>> your loopback test and called with non-zero budget, I am not sure >>>> you >>>> are allowed to call napi_consume_skb() with non-zero budget outside >>>> napi context, perhaps the cb->type for loopback test is different >>>> in lb >>>> test case ? Idk.. , please double check other code paths. >>> >>> Yes, loopback test may call napi_consume_skb() with non-zero budget >>> outside >>> napi context. Thanks for pointing out this case. >>> >>> How about add the below WARN_ONCE() in napi_consume_skb() to catch >>> this >>> kind of error? >>> >>> WARN_ONCE(!in_serving_softirq(), "napi_consume_skb() is called with >>> non-zero budget outside napi context"); >>> >> >> Cc: Eric >> >> I don't know, need to check performance impact. >> And in_serving_softirq() doesn't necessarily mean in napi >> but looking at _kfree_skb_defer(), i think it shouldn't care if napi or >> not as long as it runs in soft irq it will push the skb to that >> particular cpu napi_alloc_cache, which should be fine. Yes, we only need to ensure _kfree_skb_defer() runs with automic context. And it seems NAPI polling can be in thread context with BH disabled in below patch, so in_softirq() checking should be more future-proof? * in_softirq() - We have BH disabled, or are processing softirqs net: add support for threaded NAPI polling https://www.mail-archive.com/netdev@vger.kernel.org/msg348491.html >> >> Maybe instead of the WARN_ONCE just remove the budget condition and >> replace it with >> >> if (!in_serving_softirq()) >> dev_consume_skb_any(skb); Yes, that is good idea, _kfree_skb_defer() is only called in softirq or BH disabled context, dev_consume_skb_any(skb) is called in other context, so driver author do not need to worry about the calling context of the napi_consume_skb(). >> > > I think we need to keep costs small. > > So lets add a CONFIG_DEBUG_NET option so that developers can add > various DEBUG_NET() clauses. Do you means something like below: diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 157e024..61a6a62 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -5104,6 +5104,15 @@ do { \ }) #endif +#if defined(CONFIG_DEBUG_NET) +#define DEBUG_NET_WARN(condition, format...) \ + do { \ + WARN(condition, ##__VA_ARGS__); + } while (0) +#else +#define DEBUG_NET_WARN(condition, format...) +#endif + /* * The list of packet types we will receive (as opposed to discard) * and the routines to invoke. diff --git a/net/Kconfig b/net/Kconfig index 3831206..f59ea4b 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -473,3 +473,9 @@ config HAVE_CBPF_JIT # Extended BPF JIT (eBPF) config HAVE_EBPF_JIT bool + +config DEBUG_NET + bool + depends on DEBUG_KERNEL + help + Say Y here to add some extra checks and diagnostics to networking. diff --git a/net/core/skbuff.c b/net/core/skbuff.c index bfd7483..10547db 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -904,6 +904,9 @@ void napi_consume_skb(struct sk_buff *skb, int budget) return; } + DEBUG_NET_WARN(!in_serving_softirq(), + "napi_consume_skb() is called with non-zero budget outside softirq context"); + if (!skb_unref(skb)) return; > . >
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c index 4a49a76..feeaf75 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c @@ -2333,10 +2333,10 @@ static int hns3_alloc_buffer(struct hns3_enet_ring *ring, } static void hns3_free_buffer(struct hns3_enet_ring *ring, - struct hns3_desc_cb *cb) + struct hns3_desc_cb *cb, int budget) { if (cb->type == DESC_TYPE_SKB) - dev_kfree_skb_any((struct sk_buff *)cb->priv); + napi_consume_skb(cb->priv, budget); else if (!HNAE3_IS_TX_RING(ring) && cb->pagecnt_bias) __page_frag_cache_drain(cb->priv, cb->pagecnt_bias); memset(cb, 0, sizeof(*cb)); @@ -2370,7 +2370,8 @@ static void hns3_buffer_detach(struct hns3_enet_ring *ring, int i) ring->desc[i].addr = 0; } -static void hns3_free_buffer_detach(struct hns3_enet_ring *ring, int i) +static void hns3_free_buffer_detach(struct hns3_enet_ring *ring, int i, + int budget) { struct hns3_desc_cb *cb = &ring->desc_cb[i]; @@ -2378,7 +2379,7 @@ static void hns3_free_buffer_detach(struct hns3_enet_ring *ring, int i) return; hns3_buffer_detach(ring, i); - hns3_free_buffer(ring, cb); + hns3_free_buffer(ring, cb, budget); } static void hns3_free_buffers(struct hns3_enet_ring *ring) @@ -2386,7 +2387,7 @@ static void hns3_free_buffers(struct hns3_enet_ring *ring) int i; for (i = 0; i < ring->desc_num; i++) - hns3_free_buffer_detach(ring, i); + hns3_free_buffer_detach(ring, i, 0); } /* free desc along with its attached buffer */ @@ -2431,7 +2432,7 @@ static int hns3_alloc_and_map_buffer(struct hns3_enet_ring *ring, return 0; out_with_buf: - hns3_free_buffer(ring, cb); + hns3_free_buffer(ring, cb, 0); out: return ret; } @@ -2463,7 +2464,7 @@ static int hns3_alloc_ring_buffers(struct hns3_enet_ring *ring) out_buffer_fail: for (j = i - 1; j >= 0; j--) - hns3_free_buffer_detach(ring, j); + hns3_free_buffer_detach(ring, j, 0); return ret; } @@ -2491,7 +2492,7 @@ static void hns3_reuse_buffer(struct hns3_enet_ring *ring, int i) } static bool hns3_nic_reclaim_desc(struct hns3_enet_ring *ring, - int *bytes, int *pkts) + int *bytes, int *pkts, int budget) { /* pair with ring->last_to_use update in hns3_tx_doorbell(), * smp_store_release() is not used in hns3_tx_doorbell() because @@ -2514,7 +2515,7 @@ static bool hns3_nic_reclaim_desc(struct hns3_enet_ring *ring, (*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); + hns3_free_buffer_detach(ring, ntc, budget); if (++ntc == ring->desc_num) ntc = 0; @@ -2534,7 +2535,7 @@ static bool hns3_nic_reclaim_desc(struct hns3_enet_ring *ring, return true; } -void hns3_clean_tx_ring(struct hns3_enet_ring *ring) +void hns3_clean_tx_ring(struct hns3_enet_ring *ring, int budget) { struct net_device *netdev = ring_to_netdev(ring); struct hns3_nic_priv *priv = netdev_priv(netdev); @@ -2544,7 +2545,7 @@ void hns3_clean_tx_ring(struct hns3_enet_ring *ring) bytes = 0; pkts = 0; - if (unlikely(!hns3_nic_reclaim_desc(ring, &bytes, &pkts))) + if (unlikely(!hns3_nic_reclaim_desc(ring, &bytes, &pkts, budget))) return; ring->tqp_vector->tx_group.total_bytes += bytes; @@ -3351,7 +3352,7 @@ static int hns3_nic_common_poll(struct napi_struct *napi, int budget) * budget and be more aggressive about cleaning up the Tx descriptors. */ hns3_for_each_ring(ring, tqp_vector->tx_group) - hns3_clean_tx_ring(ring); + hns3_clean_tx_ring(ring, budget); /* make sure rx ring budget not smaller than 1 */ if (tqp_vector->num_tqps > 1) @@ -4195,7 +4196,7 @@ static void hns3_clear_tx_ring(struct hns3_enet_ring *ring) { while (ring->next_to_clean != ring->next_to_use) { ring->desc[ring->next_to_clean].tx.bdtp_fe_sc_vld_ra_ri = 0; - hns3_free_buffer_detach(ring, ring->next_to_clean); + hns3_free_buffer_detach(ring, ring->next_to_clean, 0); ring_ptr_move_fw(ring, next_to_clean); } diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h index 20e62ce..8a6c8ba 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h @@ -576,7 +576,7 @@ void hns3_ethtool_set_ops(struct net_device *netdev); int hns3_set_channels(struct net_device *netdev, struct ethtool_channels *ch); -void hns3_clean_tx_ring(struct hns3_enet_ring *ring); +void hns3_clean_tx_ring(struct hns3_enet_ring *ring, int budget); int hns3_init_all_ring(struct hns3_nic_priv *priv); int hns3_uninit_all_ring(struct hns3_nic_priv *priv); int hns3_nic_reset_all_ring(struct hnae3_handle *h); diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c index f402c39..afa1656 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c @@ -223,14 +223,14 @@ static u32 hns3_lb_check_rx_ring(struct hns3_nic_priv *priv, u32 budget) } static void hns3_lb_clear_tx_ring(struct hns3_nic_priv *priv, u32 start_ringid, - u32 end_ringid, u32 budget) + u32 end_ringid, int budget) { u32 i; for (i = start_ringid; i <= end_ringid; i++) { struct hns3_enet_ring *ring = &priv->ring[i]; - hns3_clean_tx_ring(ring); + hns3_clean_tx_ring(ring, budget); } }