Messages in this thread | | | From | Wei Fang <> | Subject | RE: [PATCH V2 net 2/4] net: fec: recycle pages for transmitted XDP frames | 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! > > > } > > + > > bdp->cbd_bufaddr = cpu_to_fec32(0); > > bdp = fec_enet_get_nextdesc(bdp, &txq->bd); > > } > [...] > > Thanks, > Olek
| |