Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Wei Fang <wei.fang@nxp.com>
To:     Alexander H Duyck <alexander.duyck@gmail.com>
CC:     dl-linux-imx <linux-imx@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        Shenwei Wang <shenwei.wang@nxp.com>,
        Clark Wang <xiaoning.wang@nxp.com>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: RE: [PATCH net] net: fec: tx processing does not call XDP APIs if
 budget is 0
Thread-Topic: [PATCH net] net: fec: tx processing does not call XDP APIs if
 budget is 0
Thread-Index: AQHZvsxwawoZvXvGy0ONq5DghKmkjq/KsvwAgACtTTA=
Date:   Wed, 26 Jul 2023 03:40:54 +0000
Message-ID: <AM5PR04MB31390FCD7DB9F905FCB599108800A@AM5PR04MB3139.eurprd04.prod.outlook.com>
References: <20230725074148.2936402-1-wei.fang@nxp.com>
 <70b71e7bb8a7dff2dacab99b0746e7bf2bee9344.camel@gmail.com>
In-Reply-To: <70b71e7bb8a7dff2dacab99b0746e7bf2bee9344.camel@gmail.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?6XfGLqPrISQ/QSjP3S0Eoi3OxKpQOi6ww/9AdwKXG63xg0yY7jzM8SsOKchi?=
 =?us-ascii?Q?nf+LGu5pF0xKXSiyZzlFnE+x76F+8b6QKcJVLEJhJKC0lHseGJmIyBv+Cbgh?=
 =?us-ascii?Q?Ki3O9wTNUxozqxUPLCCMVjaP+43JK0YGW9Y1KJqE32Il8Dv4uBG+/db9R8pb?=
 =?us-ascii?Q?iI/oM0X29e1Q3qVhfYaXFr+z3bOjTxBwElSc56ddbziTSOMTdI+rQrmKC8MW?=
 =?us-ascii?Q?Xj8jAFVpW6EKbT2rZlk6uGqmgVxFHOUzu8XyjDUMmE+0ZLJS+b4BijcGT1TJ?=
 =?us-ascii?Q?CWVAo1/qHJ1h6qgXZAeRAFD2gKJwm+gL5ugoNqz1VEDcEMFcmis4wQq5YbJE?=
 =?us-ascii?Q?2MPoWuBqQYqXaXBwqWZ6zaj2A+Fs/1whmmDen8tw7sGl76e2RcW6ez9vbBEh?=
 =?us-ascii?Q?G5X8FuOSm+h0gkZBrAVZs54otoaE0N5Z21GVHlmkTAEIB/vWVhHVQg8xsQYj?=
 =?us-ascii?Q?YV2XRSdheEmFamux3ku06TTqLSJEvvFWoA4LrgOD9bLcd5mwy5LOvptmTxwY?=
 =?us-ascii?Q?msAHhKiIhFqO4TKfZs2GSBRCTJ1I3eZ5ts8785VVxisTIutKeJFdQ9rq7c3i?=
 =?us-ascii?Q?AKB//QIPd45p3riihYcYif2JHxw+ouf96qWn3Zx4hk4wMv3lYr3AVYf9Ogkv?=
 =?us-ascii?Q?QAd2rVS5TjOwhns0B/P9eLbTuxRcuxTIo+cSMy9CzRLX6il6fePpv6tU7WjM?=
 =?us-ascii?Q?NqMp4slXzYA7odqAII9yUyNMTDj5zojfIalMYdC4LHT3rok9R4bNa9WdBh65?=
 =?us-ascii?Q?AIpgawdgUkXs4beuKborBlwV/V+L6uQh/owx19QSm/suFfj46Mvlp5Bo6u6A?=
 =?us-ascii?Q?lQ17TQgwIhb2G/R094IXdSJmF4hw0bIyZSeA7RHM6r2QsbjE/anD4hSSQun0?=
 =?us-ascii?Q?69WLd98Ez/479Q3wvydHfDzUBTn/xA6y2gFIBlB3FsJtdlZyflQJvW7ROpkX?=
 =?us-ascii?Q?JgBkyi2oegYkpdjVXj8ZzgEDw/1kK+UOLT385bXYj+YaLS6URLs2+X/uUxQ2?=
 =?us-ascii?Q?cWBOA+zizKrVhK0I4HWBZNLbNQHa6qi2cr2KSsqFmwVMs5T7k77+AMnq/BTF?=
 =?us-ascii?Q?5xvq6geroqJ09Z+lJVKq3HPhKjvZoKF0MsMYDe4bSEx8S9jJ1i7j5kuUbG2y?=
 =?us-ascii?Q?KosV3891Unz3wWOxLH6pxZIlJRFOcCJJD0mFlBRN/oi4kCXKSuR7/RhF3Q/S?=
 =?us-ascii?Q?hmS2MIe29Lrpe176IdKyw4kjRDQT9oNEJiG5MX2Ox+Egsrz7E3SId0d4Yaow?=
 =?us-ascii?Q?3AsH0HKnsd+glDdLWjfW9OTMFhWFrnpwAUZTlFZJNxgRIwV99ag4vcARsbst?=
 =?us-ascii?Q?8Da+bd8PTzRK3lbw7UJJTAsfb7n2sA5r+9DUNZPEX3AsBcooBEInnEKz0xqy?=
 =?us-ascii?Q?QmJwB5yadVS8Jrgthl8tdXJsU++ko9B3DsSF+wFtOoxR12obo56vboKR5+AU?=
 =?us-ascii?Q?A3GVgnrl/Gr+kyEqYeGHhNoqwTZRYGfWxWLBwlZqa45FD0lXvjf/lbJNlHk/?=
 =?us-ascii?Q?iBpOsy1hJK7KYWI/3TTbEL6ybUSVjiEHyfi1sO7Mo2KCuQ5Z8dEY55KtZPHU?=
 =?us-ascii?Q?9n28ZmopnwpVxt5xt6I=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM5PR04MB3139.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 674237d7-684a-4232-3f0a-08db8d8a1dd2
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jul 2023 03:40:54.8253
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 72ltYTxMomlHTEJvrl0Eh8I4LcFvmx9MQfTtEo8Xt/iz4iMhABpyW7IXUnldiLsEmOjiPnfBoicFlOCFRd8IAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR04MB9843
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Alexander,

> > @@ -1416,6 +1416,14 @@ fec_enet_tx_queue(struct net_device *ndev,
> u16 queue_id)
> >  			if (!skb)
> >  				goto tx_buf_done;
> >  		} else {
> > +			/* Tx processing cannot call any XDP (or page pool) APIs if
> > +			 * the "budget" is 0. Because NAPI is called with budget of
> > +			 * 0 (such as netpoll) indicates we may be in an IRQ context,
> > +			 * however, we can't use the page pool from IRQ context.
> > +			 */
> > +			if (unlikely(!budget))
> > +				break;
> > +
> >  			xdpf =3D txq->tx_buf[index].xdp;
> >  			if (bdp->cbd_bufaddr)
> >  				dma_unmap_single(&fep->pdev->dev,
>=20
> This statement isn't correct. There are napi enabled and non-napi
> versions of these calls. This is the reason for things like the
> "allow_direct" parameter in page_pool_put_full_page and the
> "napi_direct" parameter in __xdp_return.
>=20
> By blocking on these cases you can end up hanging the Tx queue which is
> going to break netpoll as you are going to stall the ring on XDP
> packets if they are already in the queue.
>=20
> From what I can tell your driver is using xdp_return_frame in the case
> of an XDP frame which doesn't make use of the NAPI optimizations in
> freeing from what I can tell. The NAPI optimized version is
> xdp_return_frame_rx.
>=20
So you mean it is safe to use xdp_return_frame no matter in NAPI context
or non-NAPI context? And xdp_return_frame_rx_napi must be used in NAPI
context? If so, I think I must have misunderstood, then this patch is not n=
ecessary.

> > @@ -1508,14 +1516,14 @@ fec_enet_tx_queue(struct net_device *ndev,
> u16 queue_id)
> >  		writel(0, txq->bd.reg_desc_active);
> >  }
> >
> > -static void fec_enet_tx(struct net_device *ndev)
> > +static void fec_enet_tx(struct net_device *ndev, int budget)
> >  {
> >  	struct fec_enet_private *fep =3D netdev_priv(ndev);
> >  	int i;
> >
> >  	/* Make sure that AVB queues are processed first. */
> >  	for (i =3D fep->num_tx_queues - 1; i >=3D 0; i--)
> > -		fec_enet_tx_queue(ndev, i);
> > +		fec_enet_tx_queue(ndev, i, budget);
> >  }
> >
> >  static void fec_enet_update_cbd(struct fec_enet_priv_rx_q *rxq,
> > @@ -1858,7 +1866,7 @@ static int fec_enet_rx_napi(struct napi_struct
> *napi, int budget)
> >
> >  	do {
> >  		done +=3D fec_enet_rx(ndev, budget - done);
> > -		fec_enet_tx(ndev);
> > +		fec_enet_tx(ndev, budget);
> >  	} while ((done < budget) && fec_enet_collect_events(fep));
> >
> >  	if (done < budget) {
>=20
> Since you are passing budget, one optimization you could make use of
> would be napi_consume_skb in your Tx path instead of dev_kfree_skb_any.
That's good suggestion, I think I can add this optimization in my XDP_TX su=
pport
patch. Thanks!