[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <16369c7d-af15-4959-9e84-8a495b6b5035@habana.ai>
Date: Tue, 18 Jun 2024 19:37:36 +0000
From: Omer Shpigelman <oshpigelman@...ana.ai>
To: Joe Damato <jdamato@...tly.com>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"linux-rdma@...r.kernel.org"
<linux-rdma@...r.kernel.org>,
"netdev@...r.kernel.org"
<netdev@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org"
<dri-devel@...ts.freedesktop.org>,
"ogabbay@...nel.org" <ogabbay@...nel.org>,
Zvika Yehudai <zyehudai@...ana.ai>
Subject: Re: [PATCH 09/15] net: hbl_en: add habanalabs Ethernet driver
On 6/15/24 01:48, Joe Damato wrote:
> [You don't often get email from jdamato@...tly.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On Thu, Jun 13, 2024 at 11:22:02AM +0300, Omer Shpigelman wrote:
>> This ethernet driver is initialized via auxiliary bus by the hbl_cn
>> driver.
>> It serves mainly for control operations that are needed for AI scaling.
>>
>> Signed-off-by: Omer Shpigelman <oshpigelman@...ana.ai>
>> Co-developed-by: Abhilash K V <kvabhilash@...ana.ai>
>> Signed-off-by: Abhilash K V <kvabhilash@...ana.ai>
>> Co-developed-by: Andrey Agranovich <aagranovich@...ana.ai>
>> Signed-off-by: Andrey Agranovich <aagranovich@...ana.ai>
>> Co-developed-by: Bharat Jauhari <bjauhari@...ana.ai>
>> Signed-off-by: Bharat Jauhari <bjauhari@...ana.ai>
>> Co-developed-by: David Meriin <dmeriin@...ana.ai>
>> Signed-off-by: David Meriin <dmeriin@...ana.ai>
>> Co-developed-by: Sagiv Ozeri <sozeri@...ana.ai>
>> Signed-off-by: Sagiv Ozeri <sozeri@...ana.ai>
>> Co-developed-by: Zvika Yehudai <zyehudai@...ana.ai>
>> Signed-off-by: Zvika Yehudai <zyehudai@...ana.ai>
<...>
>> + if (hdev->poll_enable)
>> + skb = __netdev_alloc_skb_ip_align(ndev, pkt_size, GFP_KERNEL);
>> + else
>> + skb = napi_alloc_skb(&port->napi, pkt_size);
>> +
>> + if (!skb) {
>> + atomic64_inc(&port->net_stats.rx_dropped);
>
> It seems like buffer exhaustion (!skb) would be rx_missed_errors?
>
> The documentation in include/uapi/linux/if_link.h:
>
> * @rx_dropped: Number of packets received but not processed,
> * e.g. due to lack of resources or unsupported protocol.
> * For hardware interfaces this counter may include packets discarded
> * due to L2 address filtering but should not include packets dropped
> * by the device due to buffer exhaustion which are counted separately in
> * @rx_missed_errors (since procfs folds those two counters together).
>
> But, I don't know much about your hardware so I could be wrong.
>
Per my understanding rx_dropped should be used here. According the doc you
posted, rx_dropped should be used in case of dropped packets due to lack
of resources, while rx_missed_errors should be used for packets that were
dropped by the device due to buffer exhaustion, not by the driver.
Please correct me if I misunderstood something.
>> + break;
>> + }
>> +
>> + skb_copy_to_linear_data(skb, pkt_addr, pkt_size);
>> + skb_put(skb, pkt_size);
>> +
>> + if (is_pkt_swap_enabled(hdev))
>> + dump_swap_pkt(port, skb);
>> +
>> + skb->protocol = eth_type_trans(skb, ndev);
>> +
>> + /* Zero the packet buffer memory to avoid leak in case of wrong
>> + * size is used when next packet populates the same memory
>> + */
>> + memset(pkt_addr, 0, pkt_size);
>> +
>> + /* polling is done in thread context and hence BH should be disabled */
>> + if (hdev->poll_enable)
>> + local_bh_disable();
>> +
>> + rc = netif_receive_skb(skb);
>
> Is there any reason in particular to call netif_receive_skb instead of
> napi_gro_receive ?
>
As you can see, we also support polling mode which is a non-NAPI flow.
We could use napi_gro_receive() for NAPI flow and netif_receive_skb() for
polling mode but we don't support RX checksum offload anyway.
>> +
>> + if (hdev->poll_enable)
>> + local_bh_enable();
<...>
>> + pkt_count = hbl_en_handle_rx(port, budget);
>> +
>> + /* If budget not fully consumed, exit the polling mode */
>> + if (pkt_count < budget) {
>> + napi_complete_done(napi, pkt_count);
>
> I believe this code might be incorrect and that it should be:
>
> if (napi_complete_done(napi, pkt_done))
> hdev->asic_funcs.reenable_rx_irq(port);
>
Thanks, I'll add the condition.
Powered by blists - more mailing lists