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:     Jesper Dangaard Brouer <jbrouer@redhat.com>
CC:     "brouer@redhat.com" <brouer@redhat.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        Andrew Lunn <andrew@lunn.ch>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        Shenwei Wang <shenwei.wang@nxp.com>,
        Clark Wang <xiaoning.wang@nxp.com>,
        "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "hawk@kernel.org" <hawk@kernel.org>,
        "john.fastabend@gmail.com" <john.fastabend@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: RE: [PATCH V3 net-next] net: fec: add XDP_TX feature support
Thread-Topic: [PATCH V3 net-next] net: fec: add XDP_TX feature support
Thread-Index: AQHZw3VFF5NSBizeREOxLxVc18wksa/WwA6AgAAZpQA=
Date:   Wed, 2 Aug 2023 12:33:34 +0000
Message-ID: <AM5PR04MB3139E5A9A0B407922A33BF99880BA@AM5PR04MB3139.eurprd04.prod.outlook.com>
References: <20230731060025.3117343-1-wei.fang@nxp.com>
 <3d0a8536-6a22-22e5-41c0-98c13dd7b802@redhat.com>
In-Reply-To: <3d0a8536-6a22-22e5-41c0-98c13dd7b802@redhat.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?ca4CNxQFUjPHpwcEyNn96s7YkS64hwGjAB7GJJjj/VgTq+PAhpvmaFceIpJH?=
 =?us-ascii?Q?7MXp/olXHBe6OC0BkAuS7OmkwIK1UFgzGNBm5LxH6GnxFfR1D8w4FvtdFESY?=
 =?us-ascii?Q?7hqGCNjHJ+MJBMmnlOI6JdP8Jm8F8/aJXRHijUqLm0Ksz6PMUMJUpKGeN1Vz?=
 =?us-ascii?Q?e3Da6wZrC5EIVFtl1rwa2dlkVKbgwFStu+uR91E1ufjeLzaKGXM+eSRIwWBX?=
 =?us-ascii?Q?qmPlfFVsyoQUbfpAaQwDWWW8vTwiuMeFNG27KPXVefVfZMZtHwn9dWlqirQa?=
 =?us-ascii?Q?z1V63MdjMWNotfXHk59HaVVJ6oW0/JpKiVRuofIbPrV0Sl6KCz8V9wxGbRd8?=
 =?us-ascii?Q?pKlyRTa2sS1nnU8Lbz7+lZTDK03t/SCt31y9orvhTAg+ZZHifV6nkaYoQicw?=
 =?us-ascii?Q?xk973UjjoOrYCOOiQonY9io7wy2+wMBgxHjCrLfyhvDjx5E1+mO1j8UclfuL?=
 =?us-ascii?Q?IkT0amuAIKqAbuX04LWqLB2vS1XfRx9iiv+3B0bHkdvp1vAB7J6w7fvSaCYu?=
 =?us-ascii?Q?l8EMOQwzX0tJjgYqruo73aKnVXvut/c6xchWdc8CxGKZ1zWtxx9cvDdVblXt?=
 =?us-ascii?Q?o0a5KliaSOjPiQVeGj2gQV2/j24bX+pDy3LaMm1UikCk61+jZJ2Y/uFmInuR?=
 =?us-ascii?Q?+uJQPfor5lsDGxeLuSuk9vq9d8xhc/uHuKGQ0pGKKfWBnVoGhb5dR/j/7aso?=
 =?us-ascii?Q?rUgpOvBQpvwT6qHpEkkh4gqMRrBJGWtB8b1h2r0hnLor58NtD3ZLFbWECIDf?=
 =?us-ascii?Q?EI2H9NmMdCTpZTkL+NoNgZ0WbnM8KtsQfmcwQ2DNYgi2dZSFE1FBYZsXbW6W?=
 =?us-ascii?Q?wwgOOOU4f3sQZb2CUvIgleRFiswUpocfjsSeTq079RWSvjZcmnSHXya+nSvk?=
 =?us-ascii?Q?P4aLGxJhXK+tSukSUQKPGeAsDQCraG8Wra8PtDT4nCglcMCbpnngRKynkO9e?=
 =?us-ascii?Q?PRhJ5vPpGOUYsbtPJLASVAkPIv3+/Zu9FPB8sSZybcWG6doeA4xSkF1Ss4V7?=
 =?us-ascii?Q?W7gI8uz4dsf8MiUymahk72sz1vn8F46FfMnKAw5p/yzq8Q5ryaFaPMVLF2ur?=
 =?us-ascii?Q?1y2yTa1N5nyhdtfexPQlhLGCsglmyzcMzN+yn97ogYEqPxxJv+mAYRxqEM/Z?=
 =?us-ascii?Q?RzVHli6tZgRdmfeZBiOAt8GN/wb/j6TrKezJQmBKYRj1IMRRItxaSCOLurbS?=
 =?us-ascii?Q?7azsys53ufX0u42AvZ1H84GBYG/6hocElOY1/fa+ugm2OpdY19Py0C751H92?=
 =?us-ascii?Q?HzmgPrfe9aTN8pYxInhUBe0PPPQUxiEdiL+sXAX+DPlUla/BG+DxgOGxU5hO?=
 =?us-ascii?Q?kK3Szu0zA2zW5nmcCdrEN0eOBCr3/bSUfqcov+Sa7Fe71E8DhlJNyO/Gy6NK?=
 =?us-ascii?Q?fFZWqV40vJi3fJZlFXAGozQoHV07ovQnd4V1JUuRZKPJS9fD9+aVx27qVzib?=
 =?us-ascii?Q?/gXX1rcrqR46A2UVc4w+XEzkbvehphTUG6iTTbWrd3/0a6hY/hl2fYHlZ4hZ?=
 =?us-ascii?Q?P8A+ZFCT3bNzoP4lTauuqRC/OBhK30PXWOWz6iGsFGHB17HlSCRgIQWpXgKh?=
 =?us-ascii?Q?O+Tzs/ItKlVJB+9oTMU=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: 0a22e14e-c85c-44d9-59fa-08db9354b019
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Aug 2023 12:33:34.3878
 (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: VfPjXrVsaIZUbaWtewvmkD+vo8CrIjE/VSZY8XeHOpHfmSmOoszSYeBMnhDVbi9XWlMs9eEqR7aL4rYmcrIFdQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB8740
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_BLOCKED,
        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

Sorry, I missed some comments before.

> > @@ -1482,7 +1486,7 @@ fec_enet_tx_queue(struct net_device *ndev, u16
> queue_id, int budget)
> >   			/* Free the sk buffer associated with this last transmit */
> >   			dev_kfree_skb_any(skb);
> >   		} else {
> > -			xdp_return_frame(xdpf);
> > +			xdp_return_frame_rx_napi(xdpf);
> >
> >   			txq->tx_buf[index].xdp =3D NULL;
> >   			/* restore default tx buffer type: FEC_TXBUF_T_SKB */
> > @@ -1573,11 +1577,18 @@ fec_enet_run_xdp(struct fec_enet_private
> *fep, struct bpf_prog *prog,
> >   		}
> >   		break;
> >
> > -	default:
> > -		bpf_warn_invalid_xdp_action(fep->netdev, prog, act);
> > -		fallthrough;
> > -
> >   	case XDP_TX:
> > +		err =3D fec_enet_xdp_tx_xmit(fep->netdev, xdp);
>=20
> You should pass along the "sync" length value to fec_enet_xdp_tx_xmit().
> Because we know DMA comes from same device (it is already DMA mapped
> to), then we can do a DMA sync "to_device" with only the sync length.
>=20
I think it's okay if the frame length does not change, but if we increase t=
he
length of the received frame, such as add a VLAN header into the frame, I
think the "sync" length value is not correct.

> > +		if (err) {
>=20
> Add an unlikely(err) or do like above case XDP_REDIRECT, where it takes
> the likely case "if (!err)" first.
Yes, you are right, I will improve it.

>=20
> > +			ret =3D FEC_ENET_XDP_CONSUMED;
> > +			page =3D virt_to_head_page(xdp->data);
> > +			page_pool_put_page(rxq->page_pool, page, sync, true);
> > +		} else {
> > +			ret =3D FEC_ENET_XDP_TX;
> > +		}
> > +		break;
> > +
> > +	default:
> >   		bpf_warn_invalid_xdp_action(fep->netdev, prog, act);
> >   		fallthrough;
> >
> > @@ -3793,7 +3804,8 @@ fec_enet_xdp_get_tx_queue(struct
> fec_enet_private *fep, int index)
> >
> >   static int fec_enet_txq_xmit_frame(struct fec_enet_private *fep,
> >   				   struct fec_enet_priv_tx_q *txq,
> > -				   struct xdp_frame *frame)
> > +				   struct xdp_frame *frame,
> > +				   bool ndo_xmit)
>=20
> E.g add parameter dma_sync_len.
>=20
> >   {
> >   	unsigned int index, status, estatus;
> >   	struct bufdesc *bdp;
> > @@ -3813,10 +3825,24 @@ static int fec_enet_txq_xmit_frame(struct
> fec_enet_private *fep,
> >
> >   	index =3D fec_enet_get_bd_index(bdp, &txq->bd);
> >
> > -	dma_addr =3D dma_map_single(&fep->pdev->dev, frame->data,
> > -				  frame->len, DMA_TO_DEVICE);
> > -	if (dma_mapping_error(&fep->pdev->dev, dma_addr))
> > -		return -ENOMEM;
> > +	if (ndo_xmit) {
> > +		dma_addr =3D dma_map_single(&fep->pdev->dev, frame->data,
> > +					  frame->len, DMA_TO_DEVICE);
> > +		if (dma_mapping_error(&fep->pdev->dev, dma_addr))
> > +			return -ENOMEM;
> > +
> > +		txq->tx_buf[index].type =3D FEC_TXBUF_T_XDP_NDO;
> > +	} else {
> > +		struct page *page =3D virt_to_page(frame->data);
> > +
> > +		dma_addr =3D page_pool_get_dma_addr(page) + sizeof(*frame) +
> > +			   frame->headroom;
> > +		dma_sync_single_for_device(&fep->pdev->dev, dma_addr,
> > +					   frame->len, DMA_BIDIRECTIONAL);
>=20
> Optimization: use dma_sync_len here instead of frame->len.
>=20
> > +		txq->tx_buf[index].type =3D FEC_TXBUF_T_XDP_TX;
> > +	}
> > +
> > +	txq->tx_buf[index].xdp =3D frame;
> >
> >   	status |=3D (BD_ENET_TX_INTR | BD_ENET_TX_LAST);
> >   	if (fep->bufdesc_ex)
> > @@ -3835,9 +3861,6 @@ static int fec_enet_txq_xmit_frame(struct
> fec_enet_private *fep,
> >   		ebdp->cbd_esc =3D cpu_to_fec32(estatus);
> >   	}
> >
> > -	txq->tx_buf[index].type =3D FEC_TXBUF_T_XDP_NDO;
> > -	txq->tx_buf[index].xdp =3D frame;
> > -
> >   	/* Make sure the updates to rest of the descriptor are performed
> before
> >   	 * transferring ownership.
> >   	 */
> > @@ -3863,6 +3886,31 @@ static int fec_enet_txq_xmit_frame(struct
> fec_enet_private *fep,
> >   	return 0;
> >   }
> >
> > +static int fec_enet_xdp_tx_xmit(struct net_device *ndev,
> > +				struct xdp_buff *xdp)
> > +{
>=20
> E.g add parameter dma_sync_len.
>=20
> > +	struct xdp_frame *xdpf =3D xdp_convert_buff_to_frame(xdp);
>=20
> XDP_TX can avoid this conversion to xdp_frame.
> It would requires some refactor of fec_enet_txq_xmit_frame().
>=20
Yes, but I'm not intend to change it, using the existing interface is enoug=
h.

> > +	struct fec_enet_private *fep =3D netdev_priv(ndev);
> > +	struct fec_enet_priv_tx_q *txq;
> > +	int cpu =3D smp_processor_id();
> > +	struct netdev_queue *nq;
> > +	int queue, ret;
> > +
> > +	queue =3D fec_enet_xdp_get_tx_queue(fep, cpu);
> > +	txq =3D fep->tx_queue[queue];
> > +	nq =3D netdev_get_tx_queue(fep->netdev, queue);
> > +
> > +	__netif_tx_lock(nq, cpu);
>=20
> It is sad that XDP_TX takes a lock for each frame.
>=20
Yes, but the XDP path share the queue with the kernel network stack, so
we need a lock here, unless there is a dedicated queue for XDP path. Do
you have a better solution?

> > +
> > +	/* Avoid tx timeout as XDP shares the queue with kernel stack */
> > +	txq_trans_cond_update(nq);
> > +	ret =3D fec_enet_txq_xmit_frame(fep, txq, xdpf, false);
>=20
> Add/pass parameter dma_sync_len to fec_enet_txq_xmit_frame().
>=20
>=20
> > +
> > +	__netif_tx_unlock(nq);
> > +
> > +	return ret;
> > +}
> > +
> >   static int fec_enet_xdp_xmit(struct net_device *dev,
> >   			     int num_frames,
> >   			     struct xdp_frame **frames,
> > @@ -3885,7 +3933,7 @@ static int fec_enet_xdp_xmit(struct net_device
> *dev,
> >   	/* Avoid tx timeout as XDP shares the queue with kernel stack */
> >   	txq_trans_cond_update(nq);
> >   	for (i =3D 0; i < num_frames; i++) {
> > -		if (fec_enet_txq_xmit_frame(fep, txq, frames[i]) < 0)
> > +		if (fec_enet_txq_xmit_frame(fep, txq, frames[i], true) < 0)
> >   			break;
> >   		sent_frames++;
> >   	}