-----Original Message-----
From: Paolo Abeni <pabeni@redhat.com>
Sent: Tuesday, June 14, 2022 3:56 AM
To: Haiyang Zhang <haiyangz@microsoft.com>; linux-hyperv@vger.kernel.org;
netdev@vger.kernel.org
Cc: Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
Stephen Hemminger [off-list ref]; Paul Rosswurm
[off-list ref]; Shachar Raindel [off-list ref];
olaf@aepfle.de; vkuznets [off-list ref]; davem@davemloft.net;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next,2/2] net: mana: Add support of XDP_REDIRECT
action

On Thu, 2022-06-09 at 14:57 -0700, Haiyang Zhang wrote:

quoted

Support XDP_REDIRECT action

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

You really should expand the changelog a little bit...

quoted

---
 drivers/net/ethernet/microsoft/mana/mana.h    |  6 ++
 .../net/ethernet/microsoft/mana/mana_bpf.c    | 64

+++++++++++++++++++

quoted

 drivers/net/ethernet/microsoft/mana/mana_en.c | 13 +++-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 12 +++-
 4 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana.h

b/drivers/net/ethernet/microsoft/mana/mana.h

quoted

index f198b34c232f..d58be64374c8 100644

--- a/drivers/net/ethernet/microsoft/mana/mana.h
+++ b/drivers/net/ethernet/microsoft/mana/mana.h

@@ -53,12 +53,14 @@ struct mana_stats_rx {
 	u64 bytes;
 	u64 xdp_drop;
 	u64 xdp_tx;
+	u64 xdp_redirect;
 	struct u64_stats_sync syncp;
 };

 struct mana_stats_tx {
 	u64 packets;
 	u64 bytes;
+	u64 xdp_xmit;
 	struct u64_stats_sync syncp;
 };

@@ -311,6 +313,8 @@ struct mana_rxq {
 	struct bpf_prog __rcu *bpf_prog;
 	struct xdp_rxq_info xdp_rxq;
 	struct page *xdp_save_page;
+	bool xdp_flush;
+	int xdp_rc; /* XDP redirect return code */

 	/* MUST BE THE LAST MEMBER:
 	 * Each receive buffer has an associated mana_recv_buf_oob.

@@ -396,6 +400,8 @@ int mana_probe(struct gdma_dev *gd, bool

resuming);

quoted

 void mana_remove(struct gdma_dev *gd, bool suspending);

 void mana_xdp_tx(struct sk_buff *skb, struct net_device *ndev);
+int mana_xdp_xmit(struct net_device *ndev, int n, struct xdp_frame

**frames,

quoted

+		  u32 flags);
 u32 mana_run_xdp(struct net_device *ndev, struct mana_rxq *rxq,
 		 struct xdp_buff *xdp, void *buf_va, uint pkt_len);
 struct bpf_prog *mana_xdp_get(struct mana_port_context *apc);

diff --git a/drivers/net/ethernet/microsoft/mana/mana_bpf.c

b/drivers/net/ethernet/microsoft/mana/mana_bpf.c

quoted

index 1d2f948b5c00..421fd39ff3a8 100644

--- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c

@@ -32,9 +32,55 @@ void mana_xdp_tx(struct sk_buff *skb, struct

net_device *ndev)

quoted

 	ndev->stats.tx_dropped++;
 }

+static int mana_xdp_xmit_fm(struct net_device *ndev, struct xdp_frame

*frame,

quoted

+			    u16 q_idx)
+{
+	struct sk_buff *skb;
+
+	skb = xdp_build_skb_from_frame(frame, ndev);
+	if (unlikely(!skb))
+		return -ENOMEM;

... especially considering this implementation choice: converting the
xdp frame to an skb in very bad for performances.

You could implement a mana xmit helper working on top of the xdp_frame
struct, and use it here.

Additionally you could consider revisiting the XDP_TX path: currently
it builds a skb from the xdp_buff to xmit it locally, while it could
resort to a much cheaper xdp_buff to xdp_frame conversion.

The traditional way to handle all the above is keep all the
XDP_TX/XDP_REDIRECT bits in the device-specific _run_xdp helper, that
will additional avoid several conditionals in mana_rx_skb().

The above refactoring would probably require a bit of work, but it will
pay-off for sure and will become more costily with time. Your choice ;)

But at the very least we need a better changelog here.

Hi Paolo,

Thank you for the review.
Sure, I will put more details into the change log.

Other suggestions on removing the SKB conversion, etc., I will work on
them later.

Thanks,
- Haiyang