| Message ID | 20240812063539.575865-8-justinlai0215@realtek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add Realtek automotive PCIe driver | expand |
On Mon, 12 Aug 2024 14:35:33 +0800 Justin Lai wrote: > + if (!delta && workdone) > + netdev_info(dev, "no Rx buffer allocated\n"); > + > + ring->dirty_idx += delta; > + > + if ((ring->dirty_idx + RTASE_NUM_DESC) == ring->cur_idx) > + netdev_emerg(dev, "Rx buffers exhausted\n"); Memory allocation failures happen, we shouldn't risk spamming the logs. I mean these two messages and the one in rtase_alloc_rx_data_buf(), the should be removed. There is a alloc_fail statistic defined in include/net/netdev_queues.h that's the correct way to report buffer allocation failures. And you should have a periodic service task / work which checks for buffers being exhausted, and if they are schedule NAPI so that it tries to allocate.
> On Mon, 12 Aug 2024 14:35:33 +0800 Justin Lai wrote: > > + if (!delta && workdone) > > + netdev_info(dev, "no Rx buffer allocated\n"); > > + > > + ring->dirty_idx += delta; > > + > > + if ((ring->dirty_idx + RTASE_NUM_DESC) == ring->cur_idx) > > + netdev_emerg(dev, "Rx buffers exhausted\n"); > > Memory allocation failures happen, we shouldn't risk spamming the logs. > I mean these two messages and the one in rtase_alloc_rx_data_buf(), the > should be removed. > > There is a alloc_fail statistic defined in include/net/netdev_queues.h that's the > correct way to report buffer allocation failures. > And you should have a periodic service task / work which checks for buffers > being exhausted, and if they are schedule NAPI so that it tries to allocate. Hi Jakub, Thank you for the comments you provided. I will modify it. Thanks Justin
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, August 16, 2024 9:55 AM > To: Justin Lai <justinlai0215@realtek.com> > Cc: davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; > linux-kernel@vger.kernel.org; netdev@vger.kernel.org; andrew@lunn.ch; > jiri@resnulli.us; horms@kernel.org; rkannoth@marvell.com; > jdamato@fastly.com; Ping-Ke Shih <pkshih@realtek.com>; Larry Chiu > <larry.chiu@realtek.com> > Subject: Re: [PATCH net-next v27 07/13] rtase: Implement a function to > receive packets > > > External mail. > > > > On Mon, 12 Aug 2024 14:35:33 +0800 Justin Lai wrote: > > + if (!delta && workdone) > > + netdev_info(dev, "no Rx buffer allocated\n"); > > + > > + ring->dirty_idx += delta; > > + > > + if ((ring->dirty_idx + RTASE_NUM_DESC) == ring->cur_idx) > > + netdev_emerg(dev, "Rx buffers exhausted\n"); > > Memory allocation failures happen, we shouldn't risk spamming the logs. > I mean these two messages and the one in rtase_alloc_rx_data_buf(), > the should be removed. > > There is a alloc_fail statistic defined in include/net/netdev_queues.h > that's the correct way to report buffer allocation failures. Hi, Jakub, Can we just count the rx_alloc_fail here? If we implement the "netdev_stat_ops", we can report this counter. > And you should have a periodic service task / work which checks for > buffers being exhausted, and if they are schedule NAPI so that it tries > to allocate. We will redefine the rtase_rx_ring_fill() to check the buffers and try to get page from the pool. Should we return the budget to schedule this NAPI if there are some empty buffers? Thanks, Larry
On Tue, 20 Aug 2024 05:13:32 +0000 Larry Chiu wrote: > > Memory allocation failures happen, we shouldn't risk spamming the logs. > > I mean these two messages and the one in rtase_alloc_rx_data_buf(), > > the should be removed. > > > > There is a alloc_fail statistic defined in include/net/netdev_queues.h > > that's the correct way to report buffer allocation failures. > > Hi, Jakub, > Can we just count the rx_alloc_fail here? > If we implement the "netdev_stat_ops", we can report this counter. I think so. > > And you should have a periodic service task / work which checks for > > buffers being exhausted, and if they are schedule NAPI so that it tries > > to allocate. > > We will redefine the rtase_rx_ring_fill() to check the buffers and > try to get page from the pool. > Should we return the budget to schedule this NAPI if there are some > empty buffers? I wouldn't recommend that. If system is under memory stress we shouldn't be adding extra load by rescheduling NAPI.
> > > And you should have a periodic service task / work which checks for > > > buffers being exhausted, and if they are schedule NAPI so that it tries > > > to allocate. > > > > We will redefine the rtase_rx_ring_fill() to check the buffers and > > try to get page from the pool. > > Should we return the budget to schedule this NAPI if there are some > > empty buffers? > > I wouldn't recommend that. If system is under memory stress > we shouldn't be adding extra load by rescheduling NAPI. Okay, I get it, but there's a problem. If all buffers are empty, it indicates that the memory allocation failed multiple times. Should we keep trying to allocate or just log an error message and stop it?
On Wed, 21 Aug 2024 09:02:41 +0000 Larry Chiu wrote: > If all buffers are empty, it indicates that the memory allocation failed > multiple times. Should we keep trying to allocate or just log an error > message and stop it? Yes, you can keep trying to refill every time the NAPI loop exits. That will be at most once per packet (assuming NAPI loop collected just a single packet). I thought you wanted to return "busy" from NAPI until memory appears. That'd be "busy polling for memory"..
diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c index 70f52149c2ab..561fcaeb2b2b 100644 --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c @@ -435,6 +435,159 @@ static void rtase_rx_ring_clear(struct page_pool *page_pool, } } +static int rtase_fragmented_frame(u32 status) +{ + return (status & (RTASE_RX_FIRST_FRAG | RTASE_RX_LAST_FRAG)) != + (RTASE_RX_FIRST_FRAG | RTASE_RX_LAST_FRAG); +} + +static void rtase_rx_csum(const struct rtase_private *tp, struct sk_buff *skb, + const union rtase_rx_desc *desc) +{ + u32 opts2 = le32_to_cpu(desc->desc_status.opts2); + + /* rx csum offload */ + if (((opts2 & RTASE_RX_V4F) && !(opts2 & RTASE_RX_IPF)) || + (opts2 & RTASE_RX_V6F)) { + if (((opts2 & RTASE_RX_TCPT) && !(opts2 & RTASE_RX_TCPF)) || + ((opts2 & RTASE_RX_UDPT) && !(opts2 & RTASE_RX_UDPF))) + skb->ip_summed = CHECKSUM_UNNECESSARY; + else + skb->ip_summed = CHECKSUM_NONE; + } else { + skb->ip_summed = CHECKSUM_NONE; + } +} + +static void rtase_rx_vlan_skb(union rtase_rx_desc *desc, struct sk_buff *skb) +{ + u32 opts2 = le32_to_cpu(desc->desc_status.opts2); + + if (!(opts2 & RTASE_RX_VLAN_TAG)) + return; + + __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), + swab16(opts2 & RTASE_VLAN_TAG_MASK)); +} + +static void rtase_rx_skb(const struct rtase_ring *ring, struct sk_buff *skb) +{ + struct rtase_int_vector *ivec = ring->ivec; + + napi_gro_receive(&ivec->napi, skb); +} + +static int rx_handler(struct rtase_ring *ring, int budget) +{ + union rtase_rx_desc *desc_base = ring->desc; + u32 pkt_size, cur_rx, delta, entry, status; + struct rtase_private *tp = ring->ivec->tp; + struct net_device *dev = tp->dev; + union rtase_rx_desc *desc; + struct sk_buff *skb; + int workdone = 0; + + cur_rx = ring->cur_idx; + entry = cur_rx % RTASE_NUM_DESC; + desc = &desc_base[entry]; + + do { + status = le32_to_cpu(desc->desc_status.opts1); + + if (status & RTASE_DESC_OWN) + break; + + /* This barrier is needed to keep us from reading + * any other fields out of the rx descriptor until + * we know the status of RTASE_DESC_OWN + */ + dma_rmb(); + + if (unlikely(status & RTASE_RX_RES)) { + if (net_ratelimit()) + netdev_warn(dev, "Rx ERROR. status = %08x\n", + status); + + tp->stats.rx_errors++; + + if (status & (RTASE_RX_RWT | RTASE_RX_RUNT)) + tp->stats.rx_length_errors++; + + if (status & RTASE_RX_CRC) + tp->stats.rx_crc_errors++; + + if (dev->features & NETIF_F_RXALL) + goto process_pkt; + + rtase_mark_to_asic(desc, tp->rx_buf_sz); + goto skip_process_pkt; + } + +process_pkt: + pkt_size = status & RTASE_RX_PKT_SIZE_MASK; + if (likely(!(dev->features & NETIF_F_RXFCS))) + pkt_size -= ETH_FCS_LEN; + + /* The driver does not support incoming fragmented frames. + * They are seen as a symptom of over-mtu sized frames. + */ + if (unlikely(rtase_fragmented_frame(status))) { + tp->stats.rx_dropped++; + tp->stats.rx_length_errors++; + rtase_mark_to_asic(desc, tp->rx_buf_sz); + goto skip_process_pkt; + } + + dma_sync_single_for_cpu(&tp->pdev->dev, + ring->mis.data_phy_addr[entry], + tp->rx_buf_sz, DMA_FROM_DEVICE); + + skb = build_skb(ring->data_buf[entry], PAGE_SIZE); + if (!skb) { + netdev_err(dev, "skb build failed\n"); + tp->stats.rx_dropped++; + rtase_mark_to_asic(desc, tp->rx_buf_sz); + goto skip_process_pkt; + } + ring->data_buf[entry] = NULL; + + if (dev->features & NETIF_F_RXCSUM) + rtase_rx_csum(tp, skb, desc); + + skb->dev = dev; + skb_put(skb, pkt_size); + skb_mark_for_recycle(skb); + skb->protocol = eth_type_trans(skb, dev); + + if (skb->pkt_type == PACKET_MULTICAST) + tp->stats.multicast++; + + rtase_rx_vlan_skb(desc, skb); + rtase_rx_skb(ring, skb); + + dev_sw_netstats_rx_add(dev, pkt_size); + +skip_process_pkt: + workdone++; + cur_rx++; + entry = cur_rx % RTASE_NUM_DESC; + desc = ring->desc + sizeof(union rtase_rx_desc) * entry; + } while (workdone != budget); + + ring->cur_idx = cur_rx; + delta = rtase_rx_ring_fill(ring, ring->dirty_idx, ring->cur_idx); + + if (!delta && workdone) + netdev_info(dev, "no Rx buffer allocated\n"); + + ring->dirty_idx += delta; + + if ((ring->dirty_idx + RTASE_NUM_DESC) == ring->cur_idx) + netdev_emerg(dev, "Rx buffers exhausted\n"); + + return workdone; +} + static void rtase_rx_desc_init(struct rtase_private *tp, u16 idx) { struct rtase_ring *ring = &tp->rx_ring[idx];
Implement rx_handler to read the information of the rx descriptor, thereby checking the packet accordingly and storing the packet in the socket buffer to complete the reception of the packet. Signed-off-by: Justin Lai <justinlai0215@realtek.com> --- .../net/ethernet/realtek/rtase/rtase_main.c | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+)