Messages in this thread | | | Date | Mon, 10 Jul 2023 14:58:51 +0200 | Subject | Re: [PATCH V2 net 2/4] net: fec: recycle pages for transmitted XDP frames | From | Alexander Lobakin <> |
| |
From: Wei Fang <wei.fang@nxp.com> Date: Fri, 7 Jul 2023 01:54:04 +0000
>> -----Original Message----- >> From: Alexander Lobakin <aleksander.lobakin@intel.com> >> Sent: 2023年7月6日 19:59 >> To: Wei Fang <wei.fang@nxp.com> >> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; >> pabeni@redhat.com; ast@kernel.org; daniel@iogearbox.net; >> hawk@kernel.org; john.fastabend@gmail.com; Shenwei Wang >> <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; >> netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; >> linux-kernel@vger.kernel.org; bpf@vger.kernel.org >> Subject: Re: [PATCH V2 net 2/4] net: fec: recycle pages for transmitted XDP >> frames >> >> From: Wei Fang <wei.fang@nxp.com> >> Date: Thu, 6 Jul 2023 16:10:10 +0800 >> >>> From: Wei Fang <wei.fang@nxp.com> >>> >>> Once the XDP frames have been successfully transmitted through the >>> ndo_xdp_xmit() interface, it's the driver responsibility to free the >>> frames so that the page_pool can recycle the pages and reuse them. >>> However, this action is not implemented in the fec driver. >>> This leads to a user-visible problem that the console will print the >>> following warning log. >> >> [...] >> >>> + if (txq->tx_buf[i].xdp) { >>> + xdp_return_frame(txq->tx_buf[i].xdp); >>> + txq->tx_buf[i].xdp = NULL; >>> + } >>> + >>> + /* restore default tx buffer type: FEC_TXBUF_T_SKB */ >>> + txq->tx_buf[i].type = FEC_TXBUF_T_SKB; >> >> Here and in the related places below: maybe set ::type dynamically when >> sending to either SKB or XDP instead of setting it only for XDP and then >> restoring each time? > I also considered this method. but when the skb has frags or needs to be TSO, > only the last tx_buf of the skb needs to store the skb pointer, but all the tx_buf > of the skb needs to set the type explicitly, I think it is a bit mess and not concise. > So I restore the type to default when recycle the BDs. Anyway, it;s just a difference > in implement, if you guys insist it's better to set the type explicitly, I will modify > the patch. Thanks!
Just more of personal preference, no problems. Moreover, your explanation makes sense to me.
>> >>> } >>> + >>> bdp->cbd_bufaddr = cpu_to_fec32(0); >>> bdp = fec_enet_get_nextdesc(bdp, &txq->bd); >>> } >> [...] >> >> Thanks, >> Olek
Thanks, Olek
| |