| Message ID | 1654811828-25339-3-git-send-email-haiyangz@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mana: Add PF and XDP_REDIRECT support | expand |
On Thu, 2022-06-09 at 14:57 -0700, Haiyang Zhang wrote: > Support XDP_REDIRECT action > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> You really should expand the changelog a little bit... > --- > drivers/net/ethernet/microsoft/mana/mana.h | 6 ++ > .../net/ethernet/microsoft/mana/mana_bpf.c | 64 +++++++++++++++++++ > 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 > 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); > 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, > + 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 > 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) > ndev->stats.tx_dropped++; > } > > +static int mana_xdp_xmit_fm(struct net_device *ndev, struct xdp_frame *frame, > + 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. Cheers, Paolo
> -----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 <sthemmin@microsoft.com>; Paul Rosswurm > <paulros@microsoft.com>; Shachar Raindel <shacharr@microsoft.com>; > olaf@aepfle.de; vkuznets <vkuznets@redhat.com>; 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: > > Support XDP_REDIRECT action > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > > You really should expand the changelog a little bit... > > > --- > > drivers/net/ethernet/microsoft/mana/mana.h | 6 ++ > > .../net/ethernet/microsoft/mana/mana_bpf.c | 64 > +++++++++++++++++++ > > 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 > > 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); > > 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, > > + 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 > > 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) > > ndev->stats.tx_dropped++; > > } > > > > +static int mana_xdp_xmit_fm(struct net_device *ndev, struct xdp_frame > *frame, > > + 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
diff --git a/drivers/net/ethernet/microsoft/mana/mana.h b/drivers/net/ethernet/microsoft/mana/mana.h 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); 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, + 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 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) ndev->stats.tx_dropped++; } +static int mana_xdp_xmit_fm(struct net_device *ndev, struct xdp_frame *frame, + u16 q_idx) +{ + struct sk_buff *skb; + + skb = xdp_build_skb_from_frame(frame, ndev); + if (unlikely(!skb)) + return -ENOMEM; + + skb_set_queue_mapping(skb, q_idx); + + mana_xdp_tx(skb, ndev); + + return 0; +} + +int mana_xdp_xmit(struct net_device *ndev, int n, struct xdp_frame **frames, + u32 flags) +{ + struct mana_port_context *apc = netdev_priv(ndev); + struct mana_stats_tx *tx_stats; + int i, count = 0; + u16 q_idx; + + if (unlikely(!apc->port_is_up)) + return 0; + + q_idx = smp_processor_id() % ndev->real_num_tx_queues; + + for (i = 0; i < n; i++) { + if (mana_xdp_xmit_fm(ndev, frames[i], q_idx)) + break; + + count++; + } + + tx_stats = &apc->tx_qp[q_idx].txq.stats; + + u64_stats_update_begin(&tx_stats->syncp); + tx_stats->xdp_xmit += count; + u64_stats_update_end(&tx_stats->syncp); + + return count; +} + u32 mana_run_xdp(struct net_device *ndev, struct mana_rxq *rxq, struct xdp_buff *xdp, void *buf_va, uint pkt_len) { + struct mana_stats_rx *rx_stats; struct bpf_prog *prog; u32 act = XDP_PASS; @@ -49,12 +95,30 @@ u32 mana_run_xdp(struct net_device *ndev, struct mana_rxq *rxq, act = bpf_prog_run_xdp(prog, xdp); + rx_stats = &rxq->stats; + switch (act) { case XDP_PASS: case XDP_TX: case XDP_DROP: break; + case XDP_REDIRECT: + rxq->xdp_rc = xdp_do_redirect(ndev, xdp, prog); + if (!rxq->xdp_rc) { + rxq->xdp_flush = true; + + u64_stats_update_begin(&rx_stats->syncp); + rx_stats->packets++; + rx_stats->bytes += pkt_len; + rx_stats->xdp_redirect++; + u64_stats_update_end(&rx_stats->syncp); + + break; + } + + fallthrough; + case XDP_ABORTED: trace_xdp_exception(ndev, prog, act); break; diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c index 3ef09e0cdbaa..9259a74eca40 100644 --- a/drivers/net/ethernet/microsoft/mana/mana_en.c +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c @@ -6,6 +6,7 @@ #include <linux/inetdevice.h> #include <linux/etherdevice.h> #include <linux/ethtool.h> +#include <linux/filter.h> #include <linux/mm.h> #include <net/checksum.h> @@ -382,6 +383,7 @@ static const struct net_device_ops mana_devops = { .ndo_validate_addr = eth_validate_addr, .ndo_get_stats64 = mana_get_stats64, .ndo_bpf = mana_bpf, + .ndo_xdp_xmit = mana_xdp_xmit, }; static void mana_cleanup_port_context(struct mana_port_context *apc) @@ -1120,6 +1122,9 @@ static void mana_rx_skb(void *buf_va, struct mana_rxcomp_oob *cqe, act = mana_run_xdp(ndev, rxq, &xdp, buf_va, pkt_len); + if (act == XDP_REDIRECT && !rxq->xdp_rc) + return; + if (act != XDP_PASS && act != XDP_TX) goto drop_xdp; @@ -1275,11 +1280,14 @@ static void mana_process_rx_cqe(struct mana_rxq *rxq, struct mana_cq *cq, static void mana_poll_rx_cq(struct mana_cq *cq) { struct gdma_comp *comp = cq->gdma_comp_buf; + struct mana_rxq *rxq = cq->rxq; int comp_read, i; comp_read = mana_gd_poll_cq(cq->gdma_cq, comp, CQE_POLLING_BUFFER); WARN_ON_ONCE(comp_read > CQE_POLLING_BUFFER); + rxq->xdp_flush = false; + for (i = 0; i < comp_read; i++) { if (WARN_ON_ONCE(comp[i].is_sq)) return; @@ -1288,8 +1296,11 @@ static void mana_poll_rx_cq(struct mana_cq *cq) if (WARN_ON_ONCE(comp[i].wq_num != cq->rxq->gdma_id)) return; - mana_process_rx_cqe(cq->rxq, cq, &comp[i]); + mana_process_rx_cqe(rxq, cq, &comp[i]); } + + if (rxq->xdp_flush) + xdp_do_flush(); } static void mana_cq_handler(void *context, struct gdma_queue *gdma_queue) diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c index e13f2453eabb..c530db76880f 100644 --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c @@ -23,7 +23,7 @@ static int mana_get_sset_count(struct net_device *ndev, int stringset) if (stringset != ETH_SS_STATS) return -EINVAL; - return ARRAY_SIZE(mana_eth_stats) + num_queues * 6; + return ARRAY_SIZE(mana_eth_stats) + num_queues * 8; } static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data) @@ -50,6 +50,8 @@ static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data) p += ETH_GSTRING_LEN; sprintf(p, "rx_%d_xdp_tx", i); p += ETH_GSTRING_LEN; + sprintf(p, "rx_%d_xdp_redirect", i); + p += ETH_GSTRING_LEN; } for (i = 0; i < num_queues; i++) { @@ -57,6 +59,8 @@ static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data) p += ETH_GSTRING_LEN; sprintf(p, "tx_%d_bytes", i); p += ETH_GSTRING_LEN; + sprintf(p, "tx_%d_xdp_xmit", i); + p += ETH_GSTRING_LEN; } } @@ -70,6 +74,8 @@ static void mana_get_ethtool_stats(struct net_device *ndev, struct mana_stats_tx *tx_stats; unsigned int start; u64 packets, bytes; + u64 xdp_redirect; + u64 xdp_xmit; u64 xdp_drop; u64 xdp_tx; int q, i = 0; @@ -89,12 +95,14 @@ static void mana_get_ethtool_stats(struct net_device *ndev, bytes = rx_stats->bytes; xdp_drop = rx_stats->xdp_drop; xdp_tx = rx_stats->xdp_tx; + xdp_redirect = rx_stats->xdp_redirect; } while (u64_stats_fetch_retry_irq(&rx_stats->syncp, start)); data[i++] = packets; data[i++] = bytes; data[i++] = xdp_drop; data[i++] = xdp_tx; + data[i++] = xdp_redirect; } for (q = 0; q < num_queues; q++) { @@ -104,10 +112,12 @@ static void mana_get_ethtool_stats(struct net_device *ndev, start = u64_stats_fetch_begin_irq(&tx_stats->syncp); packets = tx_stats->packets; bytes = tx_stats->bytes; + xdp_xmit = tx_stats->xdp_xmit; } while (u64_stats_fetch_retry_irq(&tx_stats->syncp, start)); data[i++] = packets; data[i++] = bytes; + data[i++] = xdp_xmit; } }
Support XDP_REDIRECT action Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> --- drivers/net/ethernet/microsoft/mana/mana.h | 6 ++ .../net/ethernet/microsoft/mana/mana_bpf.c | 64 +++++++++++++++++++ drivers/net/ethernet/microsoft/mana/mana_en.c | 13 +++- .../ethernet/microsoft/mana/mana_ethtool.c | 12 +++- 4 files changed, 93 insertions(+), 2 deletions(-)