Extend the current support of XDP in lan966x with the action XDP_TX and
XDP_REDIRECT.
The first patches just prepare the things such that it would be easier
to add XDP_TX and XDP_REDIRECT actions. Like adding XDP_PACKET_HEADROOM,
introduce helper functions, use the correct dma_dir for the page pool
The last 2 patches introduce the XDP actions XDP_TX and XDP_REDIRECT.
v2->v3:
- make sure to update rxq memory model
- update the page pool direction if there is any xdp program
- in case of action XDP_TX give back to reuse the page
- in case of action XDP_REDIRECT, remap the frame and make sure to
unmap it when is transmitted.
v1->v2:
- use skb_reserve of using skb_put and skb_pull
- make sure that data_len doesn't include XDP_PACKET_HEADROOM
Horatiu Vultur (7):
net: lan966x: Add XDP_PACKET_HEADROOM
net: lan966x: Introduce helper functions
net: lan966x: Add len field to lan966x_tx_dcb_buf
net: lan966x: Update rxq memory model
net: lan966x: Update dma_dir of page_pool_params
net: lan966x: Add support for XDP_TX
net: lan966x: Add support for XDP_REDIRECT
.../ethernet/microchip/lan966x/lan966x_fdma.c | 266 +++++++++++++++---
.../ethernet/microchip/lan966x/lan966x_main.c | 5 +-
.../ethernet/microchip/lan966x/lan966x_main.h | 19 ++
.../ethernet/microchip/lan966x/lan966x_xdp.c | 70 ++++-
4 files changed, 312 insertions(+), 48 deletions(-)
--
2.38.0
Currently when a frame was transmitted, it is required to unamp the
frame that was transmitted. The length of the frame was taken from the
transmitted skb. In the future we might not have an skb, therefore store
the length skb directly in the lan966x_tx_dcb_buf and use this one to
unamp the frame.
Signed-off-by: Horatiu Vultur <[email protected]>
---
drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 5 +++--
drivers/net/ethernet/microchip/lan966x/lan966x_main.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index 94c720e59caee..384ed34197d58 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -391,12 +391,12 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
continue;
dcb_buf->dev->stats.tx_packets++;
- dcb_buf->dev->stats.tx_bytes += dcb_buf->skb->len;
+ dcb_buf->dev->stats.tx_bytes += dcb_buf->len;
dcb_buf->used = false;
dma_unmap_single(lan966x->dev,
dcb_buf->dma_addr,
- dcb_buf->skb->len,
+ dcb_buf->len,
DMA_TO_DEVICE);
if (!dcb_buf->ptp)
dev_kfree_skb_any(dcb_buf->skb);
@@ -709,6 +709,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
/* Fill up the buffer */
next_dcb_buf = &tx->dcbs_buf[next_to_use];
next_dcb_buf->skb = skb;
+ next_dcb_buf->len = skb->len;
next_dcb_buf->dma_addr = dma_addr;
next_dcb_buf->used = true;
next_dcb_buf->ptp = false;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index bc93051aa0798..7bb9098496f60 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -175,6 +175,7 @@ struct lan966x_rx {
struct lan966x_tx_dcb_buf {
struct net_device *dev;
struct sk_buff *skb;
+ int len;
dma_addr_t dma_addr;
bool used;
bool ptp;
--
2.38.0
Extend lan966x XDP support with the action XDP_TX. In this case when the
received buffer needs to execute XDP_TX, the buffer will be moved to the
TX buffers. So a new RX buffer will be allocated.
When the TX finish with the frame, it would give back the buffer to the
page pool.
Signed-off-by: Horatiu Vultur <[email protected]>
---
.../ethernet/microchip/lan966x/lan966x_fdma.c | 78 +++++++++++++++++--
.../ethernet/microchip/lan966x/lan966x_main.c | 4 +-
.../ethernet/microchip/lan966x/lan966x_main.h | 8 ++
.../ethernet/microchip/lan966x/lan966x_xdp.c | 8 ++
4 files changed, 90 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index f8287a6a86ed5..b14fdb8e15e22 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -411,12 +411,18 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
dcb_buf->dev->stats.tx_bytes += dcb_buf->len;
dcb_buf->used = false;
- dma_unmap_single(lan966x->dev,
- dcb_buf->dma_addr,
- dcb_buf->len,
- DMA_TO_DEVICE);
- if (!dcb_buf->ptp)
- dev_kfree_skb_any(dcb_buf->skb);
+ if (dcb_buf->skb) {
+ dma_unmap_single(lan966x->dev,
+ dcb_buf->dma_addr,
+ dcb_buf->len,
+ DMA_TO_DEVICE);
+
+ if (!dcb_buf->ptp)
+ dev_kfree_skb_any(dcb_buf->skb);
+ }
+
+ if (dcb_buf->xdpf)
+ xdp_return_frame_rx_napi(dcb_buf->xdpf);
clear = true;
}
@@ -549,6 +555,9 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
lan966x_fdma_rx_free_page(rx);
lan966x_fdma_rx_advance_dcb(rx);
goto allocate_new;
+ case FDMA_TX:
+ lan966x_fdma_rx_advance_dcb(rx);
+ continue;
case FDMA_DROP:
lan966x_fdma_rx_free_page(rx);
lan966x_fdma_rx_advance_dcb(rx);
@@ -670,6 +679,62 @@ static void lan966x_fdma_tx_start(struct lan966x_tx *tx, int next_to_use)
tx->last_in_use = next_to_use;
}
+int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
+ struct xdp_frame *xdpf,
+ struct page *page)
+{
+ struct lan966x *lan966x = port->lan966x;
+ struct lan966x_tx_dcb_buf *next_dcb_buf;
+ struct lan966x_tx *tx = &lan966x->tx;
+ dma_addr_t dma_addr;
+ int next_to_use;
+ __be32 *ifh;
+ int ret = 0;
+
+ spin_lock(&lan966x->tx_lock);
+
+ /* Get next index */
+ next_to_use = lan966x_fdma_get_next_dcb(tx);
+ if (next_to_use < 0) {
+ netif_stop_queue(port->dev);
+ ret = NETDEV_TX_BUSY;
+ goto out;
+ }
+
+ /* Generate new IFH */
+ ifh = page_address(page) + XDP_PACKET_HEADROOM;
+ memset(ifh, 0x0, sizeof(__be32) * IFH_LEN);
+ lan966x_ifh_set_bypass(ifh, 1);
+ lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
+
+ dma_addr = page_pool_get_dma_addr(page);
+ dma_sync_single_for_device(lan966x->dev, dma_addr + XDP_PACKET_HEADROOM,
+ xdpf->len + IFH_LEN_BYTES,
+ DMA_TO_DEVICE);
+
+ /* Setup next dcb */
+ lan966x_fdma_tx_setup_dcb(tx, next_to_use, xdpf->len + IFH_LEN_BYTES,
+ dma_addr + XDP_PACKET_HEADROOM);
+
+ /* Fill up the buffer */
+ next_dcb_buf = &tx->dcbs_buf[next_to_use];
+ next_dcb_buf->skb = NULL;
+ next_dcb_buf->xdpf = xdpf;
+ next_dcb_buf->len = xdpf->len + IFH_LEN_BYTES;
+ next_dcb_buf->dma_addr = dma_addr;
+ next_dcb_buf->used = true;
+ next_dcb_buf->ptp = false;
+ next_dcb_buf->dev = port->dev;
+
+ /* Start the transmission */
+ lan966x_fdma_tx_start(tx, next_to_use);
+
+out:
+ spin_unlock(&lan966x->tx_lock);
+
+ return ret;
+}
+
int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
{
struct lan966x_port *port = netdev_priv(dev);
@@ -726,6 +791,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
/* Fill up the buffer */
next_dcb_buf = &tx->dcbs_buf[next_to_use];
next_dcb_buf->skb = skb;
+ next_dcb_buf->xdpf = NULL;
next_dcb_buf->len = skb->len;
next_dcb_buf->dma_addr = dma_addr;
next_dcb_buf->used = true;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 42be5d0f1f015..0b7707306da26 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -302,13 +302,13 @@ static int lan966x_port_ifh_xmit(struct sk_buff *skb,
return NETDEV_TX_BUSY;
}
-static void lan966x_ifh_set_bypass(void *ifh, u64 bypass)
+void lan966x_ifh_set_bypass(void *ifh, u64 bypass)
{
packing(ifh, &bypass, IFH_POS_BYPASS + IFH_WID_BYPASS - 1,
IFH_POS_BYPASS, IFH_LEN * 4, PACK, 0);
}
-static void lan966x_ifh_set_port(void *ifh, u64 bypass)
+void lan966x_ifh_set_port(void *ifh, u64 bypass)
{
packing(ifh, &bypass, IFH_POS_DSTS + IFH_WID_DSTS - 1,
IFH_POS_DSTS, IFH_LEN * 4, PACK, 0);
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 7f1231b31c924..e303a12daf88a 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -105,11 +105,13 @@ enum macaccess_entry_type {
* FDMA_PASS, frame is valid and can be used
* FDMA_ERROR, something went wrong, stop getting more frames
* FDMA_DROP, frame is dropped, but continue to get more frames
+ * FDMA_TX, frame is given to TX, but continue to get more frames
*/
enum lan966x_fdma_action {
FDMA_PASS = 0,
FDMA_ERROR,
FDMA_DROP,
+ FDMA_TX,
};
struct lan966x_port;
@@ -175,6 +177,7 @@ struct lan966x_rx {
struct lan966x_tx_dcb_buf {
struct net_device *dev;
struct sk_buff *skb;
+ struct xdp_frame *xdpf;
int len;
dma_addr_t dma_addr;
bool used;
@@ -360,6 +363,8 @@ bool lan966x_hw_offload(struct lan966x *lan966x, u32 port, struct sk_buff *skb);
void lan966x_ifh_get_src_port(void *ifh, u64 *src_port);
void lan966x_ifh_get_timestamp(void *ifh, u64 *timestamp);
+void lan966x_ifh_set_bypass(void *ifh, u64 bypass);
+void lan966x_ifh_set_port(void *ifh, u64 bypass);
void lan966x_stats_get(struct net_device *dev,
struct rtnl_link_stats64 *stats);
@@ -460,6 +465,9 @@ u32 lan966x_ptp_get_period_ps(void);
int lan966x_ptp_gettime64(struct ptp_clock_info *ptp, struct timespec64 *ts);
int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev);
+int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
+ struct xdp_frame *frame,
+ struct page *page);
int lan966x_fdma_change_mtu(struct lan966x *lan966x);
void lan966x_fdma_netdev_init(struct lan966x *lan966x, struct net_device *dev);
void lan966x_fdma_netdev_deinit(struct lan966x *lan966x, struct net_device *dev);
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
index 05c5a28206558..5fd2f3b01e179 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
@@ -54,6 +54,7 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
{
struct bpf_prog *xdp_prog = port->xdp_prog;
struct lan966x *lan966x = port->lan966x;
+ struct xdp_frame *xdpf;
struct xdp_buff xdp;
u32 act;
@@ -66,6 +67,13 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
switch (act) {
case XDP_PASS:
return FDMA_PASS;
+ case XDP_TX:
+ xdpf = xdp_convert_buff_to_frame(&xdp);
+ if (!xdpf)
+ return FDMA_DROP;
+
+ return lan966x_fdma_xmit_xdpf(port, xdpf, page) ?
+ FDMA_DROP : FDMA_TX;
default:
bpf_warn_invalid_xdp_action(port->dev, xdp_prog, act);
fallthrough;
--
2.38.0
Extend lan966x XDP support with the action XDP_REDIRECT. This is similar
with the XDP_TX, so a lot of functionality can be reused.
Signed-off-by: Horatiu Vultur <[email protected]>
---
.../ethernet/microchip/lan966x/lan966x_fdma.c | 83 +++++++++++++++----
.../ethernet/microchip/lan966x/lan966x_main.c | 1 +
.../ethernet/microchip/lan966x/lan966x_main.h | 10 ++-
.../ethernet/microchip/lan966x/lan966x_xdp.c | 31 ++++++-
4 files changed, 109 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index b14fdb8e15e22..81dc27d8f8963 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0+
#include <linux/bpf.h>
+#include <linux/filter.h>
#include "lan966x_main.h"
@@ -391,11 +392,14 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
{
struct lan966x_tx *tx = &lan966x->tx;
struct lan966x_tx_dcb_buf *dcb_buf;
+ struct xdp_frame_bulk bq;
struct lan966x_db *db;
unsigned long flags;
bool clear = false;
int i;
+ xdp_frame_bulk_init(&bq);
+
spin_lock_irqsave(&lan966x->tx_lock, flags);
for (i = 0; i < FDMA_DCB_MAX; ++i) {
dcb_buf = &tx->dcbs_buf[i];
@@ -421,12 +425,24 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
dev_kfree_skb_any(dcb_buf->skb);
}
- if (dcb_buf->xdpf)
- xdp_return_frame_rx_napi(dcb_buf->xdpf);
+ if (dcb_buf->xdpf) {
+ if (dcb_buf->xdp_ndo)
+ dma_unmap_single(lan966x->dev,
+ dcb_buf->dma_addr,
+ dcb_buf->len,
+ DMA_TO_DEVICE);
+
+ if (dcb_buf->xdp_ndo)
+ xdp_return_frame_bulk(dcb_buf->xdpf, &bq);
+ else
+ xdp_return_frame_rx_napi(dcb_buf->xdpf);
+ }
clear = true;
}
+ xdp_flush_frame_bulk(&bq);
+
if (clear)
lan966x_fdma_wakeup_netdev(lan966x);
@@ -533,6 +549,7 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
int dcb_reload = rx->dcb_index;
struct lan966x_rx_dcb *old_dcb;
struct lan966x_db *db;
+ bool redirect = false;
struct sk_buff *skb;
struct page *page;
int counter = 0;
@@ -558,6 +575,10 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
case FDMA_TX:
lan966x_fdma_rx_advance_dcb(rx);
continue;
+ case FDMA_REDIRECT:
+ lan966x_fdma_rx_advance_dcb(rx);
+ redirect = true;
+ continue;
case FDMA_DROP:
lan966x_fdma_rx_free_page(rx);
lan966x_fdma_rx_advance_dcb(rx);
@@ -594,6 +615,9 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
if (counter < weight && napi_complete_done(napi, counter))
lan_wr(0xff, lan966x, FDMA_INTR_DB_ENA);
+ if (redirect)
+ xdp_do_flush();
+
return counter;
}
@@ -681,7 +705,8 @@ static void lan966x_fdma_tx_start(struct lan966x_tx *tx, int next_to_use)
int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
struct xdp_frame *xdpf,
- struct page *page)
+ struct page *page,
+ bool dma_map)
{
struct lan966x *lan966x = port->lan966x;
struct lan966x_tx_dcb_buf *next_dcb_buf;
@@ -702,24 +727,53 @@ int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
}
/* Generate new IFH */
- ifh = page_address(page) + XDP_PACKET_HEADROOM;
- memset(ifh, 0x0, sizeof(__be32) * IFH_LEN);
- lan966x_ifh_set_bypass(ifh, 1);
- lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
+ if (dma_map) {
+ if (xdpf->headroom < IFH_LEN_BYTES) {
+ ret = NETDEV_TX_OK;
+ goto out;
+ }
- dma_addr = page_pool_get_dma_addr(page);
- dma_sync_single_for_device(lan966x->dev, dma_addr + XDP_PACKET_HEADROOM,
- xdpf->len + IFH_LEN_BYTES,
- DMA_TO_DEVICE);
+ ifh = xdpf->data - IFH_LEN_BYTES;
+ memset(ifh, 0x0, sizeof(__be32) * IFH_LEN);
+ lan966x_ifh_set_bypass(ifh, 1);
+ lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
+
+ dma_addr = dma_map_single(lan966x->dev,
+ xdpf->data - IFH_LEN_BYTES,
+ xdpf->len + IFH_LEN_BYTES,
+ DMA_TO_DEVICE);
+ if (dma_mapping_error(lan966x->dev, dma_addr)) {
+ ret = NETDEV_TX_OK;
+ goto out;
+ }
- /* Setup next dcb */
- lan966x_fdma_tx_setup_dcb(tx, next_to_use, xdpf->len + IFH_LEN_BYTES,
- dma_addr + XDP_PACKET_HEADROOM);
+ /* Setup next dcb */
+ lan966x_fdma_tx_setup_dcb(tx, next_to_use,
+ xdpf->len + IFH_LEN_BYTES,
+ dma_addr);
+ } else {
+ ifh = page_address(page) + XDP_PACKET_HEADROOM;
+ memset(ifh, 0x0, sizeof(__be32) * IFH_LEN);
+ lan966x_ifh_set_bypass(ifh, 1);
+ lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
+
+ dma_addr = page_pool_get_dma_addr(page);
+ dma_sync_single_for_device(lan966x->dev,
+ dma_addr + XDP_PACKET_HEADROOM,
+ xdpf->len + IFH_LEN_BYTES,
+ DMA_TO_DEVICE);
+
+ /* Setup next dcb */
+ lan966x_fdma_tx_setup_dcb(tx, next_to_use,
+ xdpf->len + IFH_LEN_BYTES,
+ dma_addr + XDP_PACKET_HEADROOM);
+ }
/* Fill up the buffer */
next_dcb_buf = &tx->dcbs_buf[next_to_use];
next_dcb_buf->skb = NULL;
next_dcb_buf->xdpf = xdpf;
+ next_dcb_buf->xdp_ndo = dma_map;
next_dcb_buf->len = xdpf->len + IFH_LEN_BYTES;
next_dcb_buf->dma_addr = dma_addr;
next_dcb_buf->used = true;
@@ -792,6 +846,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
next_dcb_buf = &tx->dcbs_buf[next_to_use];
next_dcb_buf->skb = skb;
next_dcb_buf->xdpf = NULL;
+ next_dcb_buf->xdp_ndo = false;
next_dcb_buf->len = skb->len;
next_dcb_buf->dma_addr = dma_addr;
next_dcb_buf->used = true;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 0b7707306da26..0aed244826d39 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -469,6 +469,7 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
.ndo_eth_ioctl = lan966x_port_ioctl,
.ndo_setup_tc = lan966x_tc_setup,
.ndo_bpf = lan966x_xdp,
+ .ndo_xdp_xmit = lan966x_xdp_xmit,
};
bool lan966x_netdevice_check(const struct net_device *dev)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index e303a12daf88a..ed4adeae553d3 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -106,12 +106,14 @@ enum macaccess_entry_type {
* FDMA_ERROR, something went wrong, stop getting more frames
* FDMA_DROP, frame is dropped, but continue to get more frames
* FDMA_TX, frame is given to TX, but continue to get more frames
+ * FDMA_REDIRECT, frame is given to TX, but continue to get more frames
*/
enum lan966x_fdma_action {
FDMA_PASS = 0,
FDMA_ERROR,
FDMA_DROP,
FDMA_TX,
+ FDMA_REDIRECT,
};
struct lan966x_port;
@@ -178,6 +180,7 @@ struct lan966x_tx_dcb_buf {
struct net_device *dev;
struct sk_buff *skb;
struct xdp_frame *xdpf;
+ bool xdp_ndo;
int len;
dma_addr_t dma_addr;
bool used;
@@ -467,7 +470,8 @@ int lan966x_ptp_gettime64(struct ptp_clock_info *ptp, struct timespec64 *ts);
int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev);
int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
struct xdp_frame *frame,
- struct page *page);
+ struct page *page,
+ bool dma_map);
int lan966x_fdma_change_mtu(struct lan966x *lan966x);
void lan966x_fdma_netdev_init(struct lan966x *lan966x, struct net_device *dev);
void lan966x_fdma_netdev_deinit(struct lan966x *lan966x, struct net_device *dev);
@@ -565,6 +569,10 @@ int lan966x_xdp(struct net_device *dev, struct netdev_bpf *xdp);
int lan966x_xdp_run(struct lan966x_port *port,
struct page *page,
u32 data_len);
+int lan966x_xdp_xmit(struct net_device *dev,
+ int n,
+ struct xdp_frame **frames,
+ u32 flags);
bool lan966x_xdp_present(struct lan966x *lan966x);
static inline bool lan966x_xdp_port_present(struct lan966x_port *port)
{
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
index 5fd2f3b01e179..9f363316115ae 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
@@ -50,6 +50,30 @@ int lan966x_xdp(struct net_device *dev, struct netdev_bpf *xdp)
}
}
+int lan966x_xdp_xmit(struct net_device *dev,
+ int n,
+ struct xdp_frame **frames,
+ u32 flags)
+{
+ struct lan966x_port *port = netdev_priv(dev);
+ int i, nxmit = 0;
+
+ for (i = 0; i < n; ++i) {
+ struct xdp_frame *xdpf = frames[i];
+ int err;
+
+ err = lan966x_fdma_xmit_xdpf(port, xdpf,
+ virt_to_head_page(xdpf->data),
+ true);
+ if (err)
+ break;
+
+ nxmit++;
+ }
+
+ return nxmit;
+}
+
int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
{
struct bpf_prog *xdp_prog = port->xdp_prog;
@@ -72,8 +96,13 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
if (!xdpf)
return FDMA_DROP;
- return lan966x_fdma_xmit_xdpf(port, xdpf, page) ?
+ return lan966x_fdma_xmit_xdpf(port, xdpf, page, false) ?
FDMA_DROP : FDMA_TX;
+ case XDP_REDIRECT:
+ if (xdp_do_redirect(port->dev, &xdp, xdp_prog))
+ return FDMA_DROP;
+
+ return FDMA_REDIRECT;
default:
bpf_warn_invalid_xdp_action(port->dev, xdp_prog, act);
fallthrough;
--
2.38.0
To add support for XDP_TX it is required to be able to write to the DMA
area therefore it is required that the pages will be mapped using
DMA_BIDIRECTIONAL flag.
Therefore check if there are any xdp programs on the interfaces and in
that case set DMA_BIDRECTIONAL otherwise use DMA_FROM_DEVICE.
Therefore when a new XDP program is added it is required to redo the
page_pool.
Signed-off-by: Horatiu Vultur <[email protected]>
---
.../ethernet/microchip/lan966x/lan966x_fdma.c | 29 ++++++++++++++----
.../ethernet/microchip/lan966x/lan966x_main.h | 2 ++
.../ethernet/microchip/lan966x/lan966x_xdp.c | 30 +++++++++++++++++++
3 files changed, 55 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index 483d1470c8362..f8287a6a86ed5 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -81,6 +81,9 @@ static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
struct lan966x_port *port;
int i;
+ if (lan966x_xdp_present(lan966x))
+ pp_params.dma_dir = DMA_BIDIRECTIONAL;
+
rx->page_pool = page_pool_create(&pp_params);
for (i = 0; i < lan966x->num_phys_ports; ++i) {
@@ -827,16 +830,11 @@ static int lan966x_fdma_get_max_frame(struct lan966x *lan966x)
XDP_PACKET_HEADROOM;
}
-int lan966x_fdma_change_mtu(struct lan966x *lan966x)
+static int __lan966x_fdma_reload(struct lan966x *lan966x, int max_mtu)
{
- int max_mtu;
int err;
u32 val;
- max_mtu = lan966x_fdma_get_max_frame(lan966x);
- if (max_mtu == lan966x->rx.max_mtu)
- return 0;
-
/* Disable the CPU port */
lan_rmw(QSYS_SW_PORT_MODE_PORT_ENA_SET(0),
QSYS_SW_PORT_MODE_PORT_ENA,
@@ -862,6 +860,25 @@ int lan966x_fdma_change_mtu(struct lan966x *lan966x)
return err;
}
+int lan966x_fdma_change_mtu(struct lan966x *lan966x)
+{
+ int max_mtu;
+
+ max_mtu = lan966x_fdma_get_max_frame(lan966x);
+ if (max_mtu == lan966x->rx.max_mtu)
+ return 0;
+
+ return __lan966x_fdma_reload(lan966x, max_mtu);
+}
+
+int lan966x_fdma_reload_page_pool(struct lan966x *lan966x)
+{
+ int max_mtu;
+
+ max_mtu = lan966x_fdma_get_max_frame(lan966x);
+ return __lan966x_fdma_reload(lan966x, max_mtu);
+}
+
void lan966x_fdma_netdev_init(struct lan966x *lan966x, struct net_device *dev)
{
if (lan966x->fdma_ndev)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 7bb9098496f60..7f1231b31c924 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -466,6 +466,7 @@ void lan966x_fdma_netdev_deinit(struct lan966x *lan966x, struct net_device *dev)
int lan966x_fdma_init(struct lan966x *lan966x);
void lan966x_fdma_deinit(struct lan966x *lan966x);
irqreturn_t lan966x_fdma_irq_handler(int irq, void *args);
+int lan966x_fdma_reload_page_pool(struct lan966x *lan966x);
int lan966x_lag_port_join(struct lan966x_port *port,
struct net_device *brport_dev,
@@ -556,6 +557,7 @@ int lan966x_xdp(struct net_device *dev, struct netdev_bpf *xdp);
int lan966x_xdp_run(struct lan966x_port *port,
struct page *page,
u32 data_len);
+bool lan966x_xdp_present(struct lan966x *lan966x);
static inline bool lan966x_xdp_port_present(struct lan966x_port *port)
{
return !!port->xdp_prog;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
index 8ebde1eb6a09c..05c5a28206558 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
@@ -11,6 +11,8 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
struct lan966x_port *port = netdev_priv(dev);
struct lan966x *lan966x = port->lan966x;
struct bpf_prog *old_prog;
+ bool old_xdp, new_xdp;
+ int err;
if (!lan966x->fdma) {
NL_SET_ERR_MSG_MOD(xdp->extack,
@@ -18,7 +20,20 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
return -EOPNOTSUPP;
}
+ old_xdp = lan966x_xdp_present(lan966x);
old_prog = xchg(&port->xdp_prog, xdp->prog);
+ new_xdp = lan966x_xdp_present(lan966x);
+
+ if (old_xdp != new_xdp)
+ goto out;
+
+ err = lan966x_fdma_reload_page_pool(lan966x);
+ if (err) {
+ xchg(&port->xdp_prog, old_prog);
+ return err;
+ }
+
+out:
if (old_prog)
bpf_prog_put(old_prog);
@@ -62,6 +77,21 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
}
}
+bool lan966x_xdp_present(struct lan966x *lan966x)
+{
+ int p;
+
+ for (p = 0; p < lan966x->num_phys_ports; ++p) {
+ if (!lan966x->ports[p])
+ continue;
+
+ if (lan966x_xdp_port_present(lan966x->ports[p]))
+ return true;
+ }
+
+ return false;
+}
+
int lan966x_xdp_port_init(struct lan966x_port *port)
{
struct lan966x *lan966x = port->lan966x;
--
2.38.0
From: Horatiu Vultur <[email protected]>
Date: Mon, 21 Nov 2022 22:28:50 +0100
> Extend lan966x XDP support with the action XDP_REDIRECT. This is similar
> with the XDP_TX, so a lot of functionality can be reused.
>
> Signed-off-by: Horatiu Vultur <[email protected]>
> ---
> .../ethernet/microchip/lan966x/lan966x_fdma.c | 83 +++++++++++++++----
> .../ethernet/microchip/lan966x/lan966x_main.c | 1 +
> .../ethernet/microchip/lan966x/lan966x_main.h | 10 ++-
> .../ethernet/microchip/lan966x/lan966x_xdp.c | 31 ++++++-
> 4 files changed, 109 insertions(+), 16 deletions(-)
[...]
> @@ -558,6 +575,10 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
> case FDMA_TX:
> lan966x_fdma_rx_advance_dcb(rx);
> continue;
> + case FDMA_REDIRECT:
> + lan966x_fdma_rx_advance_dcb(rx);
> + redirect = true;
> + continue;
I think you can save a couple lines here and avoid small code dup:
+ case FDMA_REDIRECT:
+ redirect = true;
+ fallthrough;
case FDMA_TX:
lan966x_fdma_rx_advance_dcb(rx);
continue;
The logics stays the same.
> case FDMA_DROP:
> lan966x_fdma_rx_free_page(rx);
> lan966x_fdma_rx_advance_dcb(rx);
[...]
> @@ -178,6 +180,7 @@ struct lan966x_tx_dcb_buf {
> struct net_device *dev;
> struct sk_buff *skb;
> struct xdp_frame *xdpf;
> + bool xdp_ndo;
I suggest carefully inspecting this struct with pahole (or by just
printkaying its layout/sizes/offsets at runtime) and see if there's
any holes and how it could be optimized.
Also, it's just my personal preference, but it's not that unpopular:
I don't trust bools inside structures as they may surprise with
their sizes or alignment depending on the architercture. Considering
all the blah I wrote, I'd define it as:
struct lan966x_tx_dcb_buf {
dma_addr_t dma_addr; // can be 8 bytes on 32-bit plat
struct net_device *dev; // ensure natural alignment
struct sk_buff *skb;
struct xdp_frame *xdpf;
u32 len;
u32 xdp_ndo:1; // put all your booleans here in
u32 used:1; // one u32
...
};
BTW, we usually do union { skb, xdpf } since they're mutually
exclusive. And to distinguish between XDP and regular Tx you can use
one more bit/bool. This can also come handy later when you add XSk
support (you will be adding it, right? Please :P).
> int len;
> dma_addr_t dma_addr;
> bool used;
[...]
> --
> 2.38.0
Thanks,
Olek
From: Horatiu Vultur <[email protected]>
Date: Mon, 21 Nov 2022 22:28:48 +0100
> To add support for XDP_TX it is required to be able to write to the DMA
> area therefore it is required that the pages will be mapped using
> DMA_BIDIRECTIONAL flag.
> Therefore check if there are any xdp programs on the interfaces and in
> that case set DMA_BIDRECTIONAL otherwise use DMA_FROM_DEVICE.
> Therefore when a new XDP program is added it is required to redo the
> page_pool.
>
> Signed-off-by: Horatiu Vultur <[email protected]>
> ---
> .../ethernet/microchip/lan966x/lan966x_fdma.c | 29 ++++++++++++++----
> .../ethernet/microchip/lan966x/lan966x_main.h | 2 ++
> .../ethernet/microchip/lan966x/lan966x_xdp.c | 30 +++++++++++++++++++
> 3 files changed, 55 insertions(+), 6 deletions(-)
[...]
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> index 8ebde1eb6a09c..05c5a28206558 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> @@ -11,6 +11,8 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
> struct lan966x_port *port = netdev_priv(dev);
> struct lan966x *lan966x = port->lan966x;
> struct bpf_prog *old_prog;
> + bool old_xdp, new_xdp;
> + int err;
>
> if (!lan966x->fdma) {
> NL_SET_ERR_MSG_MOD(xdp->extack,
> @@ -18,7 +20,20 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
> return -EOPNOTSUPP;
> }
>
> + old_xdp = lan966x_xdp_present(lan966x);
> old_prog = xchg(&port->xdp_prog, xdp->prog);
> + new_xdp = lan966x_xdp_present(lan966x);
> +
> + if (old_xdp != new_xdp)
> + goto out;
Shouldn't it be the other way around? E.g. when there's no prog and
you're installing it or there is a prog and we're removing it from
the interface, DMA dir must be changed, so we reload the Pools, but
if `old_xdp == new_xdp` we should just hotswap them and goto out?
> +
> + err = lan966x_fdma_reload_page_pool(lan966x);
> + if (err) {
> + xchg(&port->xdp_prog, old_prog);
> + return err;
> + }
> +
> +out:
> if (old_prog)
> bpf_prog_put(old_prog);
>
[...]
> --
> 2.38.0
Thanks,
Olek
From: Horatiu Vultur <[email protected]>
Date: Mon, 21 Nov 2022 22:28:46 +0100
> Currently when a frame was transmitted, it is required to unamp the
> frame that was transmitted. The length of the frame was taken from the
> transmitted skb. In the future we might not have an skb, therefore store
> the length skb directly in the lan966x_tx_dcb_buf and use this one to
> unamp the frame.
>
> Signed-off-by: Horatiu Vultur <[email protected]>
> ---
> drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 5 +++--
> drivers/net/ethernet/microchip/lan966x/lan966x_main.h | 1 +
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index 94c720e59caee..384ed34197d58 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> @@ -391,12 +391,12 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> continue;
>
> dcb_buf->dev->stats.tx_packets++;
> - dcb_buf->dev->stats.tx_bytes += dcb_buf->skb->len;
> + dcb_buf->dev->stats.tx_bytes += dcb_buf->len;
>
> dcb_buf->used = false;
> dma_unmap_single(lan966x->dev,
> dcb_buf->dma_addr,
> - dcb_buf->skb->len,
> + dcb_buf->len,
> DMA_TO_DEVICE);
> if (!dcb_buf->ptp)
> dev_kfree_skb_any(dcb_buf->skb);
> @@ -709,6 +709,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
> /* Fill up the buffer */
> next_dcb_buf = &tx->dcbs_buf[next_to_use];
> next_dcb_buf->skb = skb;
> + next_dcb_buf->len = skb->len;
> next_dcb_buf->dma_addr = dma_addr;
> next_dcb_buf->used = true;
> next_dcb_buf->ptp = false;
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index bc93051aa0798..7bb9098496f60 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -175,6 +175,7 @@ struct lan966x_rx {
> struct lan966x_tx_dcb_buf {
> struct net_device *dev;
> struct sk_buff *skb;
> + int len;
Nit: perhaps you can define it as `u32` since fram length can't be
negative?
> dma_addr_t dma_addr;
Oh, also, on platforms with 64-bit addressing, `int len` placed in
between ::skb and ::dma_addr would create a 4-byte hole in the
structure. Placing `len` after ::dma_addr would make it holeless on
any architercture.
> bool used;
> bool ptp;
> --
> 2.38.0
Thanks,
Olek
From: Horatiu Vultur <[email protected]>
Date: Mon, 21 Nov 2022 22:28:49 +0100
> Extend lan966x XDP support with the action XDP_TX. In this case when the
> received buffer needs to execute XDP_TX, the buffer will be moved to the
> TX buffers. So a new RX buffer will be allocated.
> When the TX finish with the frame, it would give back the buffer to the
> page pool.
>
> Signed-off-by: Horatiu Vultur <[email protected]>
> ---
> .../ethernet/microchip/lan966x/lan966x_fdma.c | 78 +++++++++++++++++--
> .../ethernet/microchip/lan966x/lan966x_main.c | 4 +-
> .../ethernet/microchip/lan966x/lan966x_main.h | 8 ++
> .../ethernet/microchip/lan966x/lan966x_xdp.c | 8 ++
> 4 files changed, 90 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index f8287a6a86ed5..b14fdb8e15e22 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> @@ -411,12 +411,18 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> dcb_buf->dev->stats.tx_bytes += dcb_buf->len;
>
> dcb_buf->used = false;
> - dma_unmap_single(lan966x->dev,
> - dcb_buf->dma_addr,
> - dcb_buf->len,
> - DMA_TO_DEVICE);
> - if (!dcb_buf->ptp)
> - dev_kfree_skb_any(dcb_buf->skb);
> + if (dcb_buf->skb) {
> + dma_unmap_single(lan966x->dev,
> + dcb_buf->dma_addr,
> + dcb_buf->len,
> + DMA_TO_DEVICE);
> +
> + if (!dcb_buf->ptp)
> + dev_kfree_skb_any(dcb_buf->skb);
Damn, forgot to remind you you wanted to switch to
napi_consume_skb() :s
> + }
> +
> + if (dcb_buf->xdpf)
> + xdp_return_frame_rx_napi(dcb_buf->xdpf);
>
> clear = true;
> }
[...]
> --
> 2.38.0
Thanks,
Olek
The 11/22/2022 17:56, Alexander Lobakin wrote:
>
> From: Horatiu Vultur <[email protected]>
> Date: Mon, 21 Nov 2022 22:28:49 +0100
>
> > Extend lan966x XDP support with the action XDP_TX. In this case when the
> > received buffer needs to execute XDP_TX, the buffer will be moved to the
> > TX buffers. So a new RX buffer will be allocated.
> > When the TX finish with the frame, it would give back the buffer to the
> > page pool.
> >
> > Signed-off-by: Horatiu Vultur <[email protected]>
> > ---
> > .../ethernet/microchip/lan966x/lan966x_fdma.c | 78 +++++++++++++++++--
> > .../ethernet/microchip/lan966x/lan966x_main.c | 4 +-
> > .../ethernet/microchip/lan966x/lan966x_main.h | 8 ++
> > .../ethernet/microchip/lan966x/lan966x_xdp.c | 8 ++
> > 4 files changed, 90 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > index f8287a6a86ed5..b14fdb8e15e22 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > @@ -411,12 +411,18 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> > dcb_buf->dev->stats.tx_bytes += dcb_buf->len;
> >
> > dcb_buf->used = false;
> > - dma_unmap_single(lan966x->dev,
> > - dcb_buf->dma_addr,
> > - dcb_buf->len,
> > - DMA_TO_DEVICE);
> > - if (!dcb_buf->ptp)
> > - dev_kfree_skb_any(dcb_buf->skb);
> > + if (dcb_buf->skb) {
> > + dma_unmap_single(lan966x->dev,
> > + dcb_buf->dma_addr,
> > + dcb_buf->len,
> > + DMA_TO_DEVICE);
> > +
> > + if (!dcb_buf->ptp)
> > + dev_kfree_skb_any(dcb_buf->skb);
>
> Damn, forgot to remind you you wanted to switch to
> napi_consume_skb() :s
Correct, I forgot to update this. Will do in the next series.
>
> > + }
> > +
> > + if (dcb_buf->xdpf)
> > + xdp_return_frame_rx_napi(dcb_buf->xdpf);
> >
> > clear = true;
> > }
>
> [...]
>
> > --
> > 2.38.0
>
> Thanks,
> Olek
--
/Horatiu
The 11/22/2022 12:30, Alexander Lobakin wrote:
>
> From: Horatiu Vultur <[email protected]>
> Date: Mon, 21 Nov 2022 22:28:46 +0100
>
> > Currently when a frame was transmitted, it is required to unamp the
> > frame that was transmitted. The length of the frame was taken from the
> > transmitted skb. In the future we might not have an skb, therefore store
> > the length skb directly in the lan966x_tx_dcb_buf and use this one to
> > unamp the frame.
> >
> > Signed-off-by: Horatiu Vultur <[email protected]>
> > ---
> > drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 5 +++--
> > drivers/net/ethernet/microchip/lan966x/lan966x_main.h | 1 +
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > index 94c720e59caee..384ed34197d58 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > @@ -391,12 +391,12 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> > continue;
> >
> > dcb_buf->dev->stats.tx_packets++;
> > - dcb_buf->dev->stats.tx_bytes += dcb_buf->skb->len;
> > + dcb_buf->dev->stats.tx_bytes += dcb_buf->len;
> >
> > dcb_buf->used = false;
> > dma_unmap_single(lan966x->dev,
> > dcb_buf->dma_addr,
> > - dcb_buf->skb->len,
> > + dcb_buf->len,
> > DMA_TO_DEVICE);
> > if (!dcb_buf->ptp)
> > dev_kfree_skb_any(dcb_buf->skb);
> > @@ -709,6 +709,7 @@ int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
> > /* Fill up the buffer */
> > next_dcb_buf = &tx->dcbs_buf[next_to_use];
> > next_dcb_buf->skb = skb;
> > + next_dcb_buf->len = skb->len;
> > next_dcb_buf->dma_addr = dma_addr;
> > next_dcb_buf->used = true;
> > next_dcb_buf->ptp = false;
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index bc93051aa0798..7bb9098496f60 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -175,6 +175,7 @@ struct lan966x_rx {
> > struct lan966x_tx_dcb_buf {
> > struct net_device *dev;
> > struct sk_buff *skb;
> > + int len;
>
> Nit: perhaps you can define it as `u32` since fram length can't be
> negative?
The length is always positive. I will update this in the next version.
>
> > dma_addr_t dma_addr;
>
> Oh, also, on platforms with 64-bit addressing, `int len` placed in
> between ::skb and ::dma_addr would create a 4-byte hole in the
> structure. Placing `len` after ::dma_addr would make it holeless on
> any architercture.
Thanks for the suggestion.
I will make sure that lan966x_tx_dcb_buf will not have any holes. In
this patch I will arrange the members and in the next patches where
I will modify the struct, I will add them at the right place.
>
> > bool used;
> > bool ptp;
> > --
> > 2.38.0
>
> Thanks,
> Olek
--
/Horatiu
The 11/22/2022 13:04, Alexander Lobakin wrote:
>
> From: Horatiu Vultur <[email protected]>
> Date: Mon, 21 Nov 2022 22:28:50 +0100
>
> > Extend lan966x XDP support with the action XDP_REDIRECT. This is similar
> > with the XDP_TX, so a lot of functionality can be reused.
> >
> > Signed-off-by: Horatiu Vultur <[email protected]>
> > ---
> > .../ethernet/microchip/lan966x/lan966x_fdma.c | 83 +++++++++++++++----
> > .../ethernet/microchip/lan966x/lan966x_main.c | 1 +
> > .../ethernet/microchip/lan966x/lan966x_main.h | 10 ++-
> > .../ethernet/microchip/lan966x/lan966x_xdp.c | 31 ++++++-
> > 4 files changed, 109 insertions(+), 16 deletions(-)
>
> [...]
>
> > @@ -558,6 +575,10 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
> > case FDMA_TX:
> > lan966x_fdma_rx_advance_dcb(rx);
> > continue;
> > + case FDMA_REDIRECT:
> > + lan966x_fdma_rx_advance_dcb(rx);
> > + redirect = true;
> > + continue;
>
> I think you can save a couple lines here and avoid small code dup:
>
> + case FDMA_REDIRECT:
> + redirect = true;
> + fallthrough;
> case FDMA_TX:
> lan966x_fdma_rx_advance_dcb(rx);
> continue;
I will save only a line but I will add this change in the next version
as I like it more than what I wrote.
>
> The logics stays the same.
>
> > case FDMA_DROP:
> > lan966x_fdma_rx_free_page(rx);
> > lan966x_fdma_rx_advance_dcb(rx);
>
> [...]
>
> > @@ -178,6 +180,7 @@ struct lan966x_tx_dcb_buf {
> > struct net_device *dev;
> > struct sk_buff *skb;
> > struct xdp_frame *xdpf;
> > + bool xdp_ndo;
>
> I suggest carefully inspecting this struct with pahole (or by just
> printkaying its layout/sizes/offsets at runtime) and see if there's
> any holes and how it could be optimized.
> Also, it's just my personal preference, but it's not that unpopular:
> I don't trust bools inside structures as they may surprise with
> their sizes or alignment depending on the architercture. Considering
> all the blah I wrote, I'd define it as:
>
> struct lan966x_tx_dcb_buf {
> dma_addr_t dma_addr; // can be 8 bytes on 32-bit plat
> struct net_device *dev; // ensure natural alignment
> struct sk_buff *skb;
> struct xdp_frame *xdpf;
> u32 len;
> u32 xdp_ndo:1; // put all your booleans here in
> u32 used:1; // one u32
> ...
> };
Thanks for the suggestion. I make sure not that this struct will not
have any holes.
Can it be a rule of thumb, that every time when a new member is added to
a struct, to make sure that it doesn't introduce any holes?
>
> BTW, we usually do union { skb, xdpf } since they're mutually
> exclusive. And to distinguish between XDP and regular Tx you can use
> one more bit/bool. This can also come handy later when you add XSk
> support (you will be adding it, right? Please :P).
I think I will take this battle at later point when I will add XSK :)
After I finish with this patch series, I will need to focus on some VCAP
support for lan966x.
And maybe after that I will be able to add XSK. Because I need to look
more at this XSK topic as I have looked too much on it before but I heard
a lot of great things about it :)
>
> > int len;
> > dma_addr_t dma_addr;
> > bool used;
>
> [...]
>
> > --
> > 2.38.0
>
> Thanks,
> Olek
--
/Horatiu
The 11/22/2022 12:43, Alexander Lobakin wrote:
>
> From: Horatiu Vultur <[email protected]>
> Date: Mon, 21 Nov 2022 22:28:48 +0100
>
> > To add support for XDP_TX it is required to be able to write to the DMA
> > area therefore it is required that the pages will be mapped using
> > DMA_BIDIRECTIONAL flag.
> > Therefore check if there are any xdp programs on the interfaces and in
> > that case set DMA_BIDRECTIONAL otherwise use DMA_FROM_DEVICE.
> > Therefore when a new XDP program is added it is required to redo the
> > page_pool.
> >
> > Signed-off-by: Horatiu Vultur <[email protected]>
> > ---
> > .../ethernet/microchip/lan966x/lan966x_fdma.c | 29 ++++++++++++++----
> > .../ethernet/microchip/lan966x/lan966x_main.h | 2 ++
> > .../ethernet/microchip/lan966x/lan966x_xdp.c | 30 +++++++++++++++++++
> > 3 files changed, 55 insertions(+), 6 deletions(-)
>
> [...]
>
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > index 8ebde1eb6a09c..05c5a28206558 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > @@ -11,6 +11,8 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
> > struct lan966x_port *port = netdev_priv(dev);
> > struct lan966x *lan966x = port->lan966x;
> > struct bpf_prog *old_prog;
> > + bool old_xdp, new_xdp;
> > + int err;
> >
> > if (!lan966x->fdma) {
> > NL_SET_ERR_MSG_MOD(xdp->extack,
> > @@ -18,7 +20,20 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
> > return -EOPNOTSUPP;
> > }
> >
> > + old_xdp = lan966x_xdp_present(lan966x);
> > old_prog = xchg(&port->xdp_prog, xdp->prog);
> > + new_xdp = lan966x_xdp_present(lan966x);
> > +
> > + if (old_xdp != new_xdp)
> > + goto out;
>
> Shouldn't it be the other way around? E.g. when there's no prog and
> you're installing it or there is a prog and we're removing it from
> the interface, DMA dir must be changed, so we reload the Pools, but
> if `old_xdp == new_xdp` we should just hotswap them and goto out?
Argh! Yes, it needs to be the other way around.
>
> > +
> > + err = lan966x_fdma_reload_page_pool(lan966x);
> > + if (err) {
> > + xchg(&port->xdp_prog, old_prog);
> > + return err;
> > + }
> > +
> > +out:
> > if (old_prog)
> > bpf_prog_put(old_prog);
> >
>
> [...]
>
> > --
> > 2.38.0
>
> Thanks,
> Olek
--
/Horatiu
From: Horatiu Vultur <[email protected]>
Date: Tue, 22 Nov 2022 22:37:24 +0100
> The 11/22/2022 13:04, Alexander Lobakin wrote:
> >
> > From: Horatiu Vultur <[email protected]>
> > Date: Mon, 21 Nov 2022 22:28:50 +0100
> >
> > > Extend lan966x XDP support with the action XDP_REDIRECT. This is similar
> > > with the XDP_TX, so a lot of functionality can be reused.
> > >
> > > Signed-off-by: Horatiu Vultur <[email protected]>
> > > ---
> > > .../ethernet/microchip/lan966x/lan966x_fdma.c | 83 +++++++++++++++----
> > > .../ethernet/microchip/lan966x/lan966x_main.c | 1 +
> > > .../ethernet/microchip/lan966x/lan966x_main.h | 10 ++-
> > > .../ethernet/microchip/lan966x/lan966x_xdp.c | 31 ++++++-
> > > 4 files changed, 109 insertions(+), 16 deletions(-)
[...]
> > I suggest carefully inspecting this struct with pahole (or by just
> > printkaying its layout/sizes/offsets at runtime) and see if there's
> > any holes and how it could be optimized.
> > Also, it's just my personal preference, but it's not that unpopular:
> > I don't trust bools inside structures as they may surprise with
> > their sizes or alignment depending on the architercture. Considering
> > all the blah I wrote, I'd define it as:
> >
> > struct lan966x_tx_dcb_buf {
> > dma_addr_t dma_addr; // can be 8 bytes on 32-bit plat
> > struct net_device *dev; // ensure natural alignment
> > struct sk_buff *skb;
> > struct xdp_frame *xdpf;
> > u32 len;
> > u32 xdp_ndo:1; // put all your booleans here in
> > u32 used:1; // one u32
> > ...
> > };
>
> Thanks for the suggestion. I make sure not that this struct will not
> have any holes.
> Can it be a rule of thumb, that every time when a new member is added to
> a struct, to make sure that it doesn't introduce any holes?
Yass, it's always good to do a quick check each time you're making
changes in a structure. This can prevent not only from excessive
memory usage, but most important from performance hits when some
hot field gets pushed out of the cacheline the field was in
previously.
Minimizing holes and using `u32 :1` vs `bool` for flags is more of
my personal preference, but it's kinda backed by experience, so I
treat it as something worth sharing :D
>
> >
> > BTW, we usually do union { skb, xdpf } since they're mutually
> > exclusive. And to distinguish between XDP and regular Tx you can use
> > one more bit/bool. This can also come handy later when you add XSk
> > support (you will be adding it, right? Please :P).
>
> I think I will take this battle at later point when I will add XSK :)
> After I finish with this patch series, I will need to focus on some VCAP
> support for lan966x.
Sure!
> And maybe after that I will be able to add XSK. Because I need to look
> more at this XSK topic as I have looked too much on it before but I heard
> a lot of great things about it :)
Depends on the real usecases of the hardware. But always good to see
more drivers supporting it :>
>
> >
> > > int len;
> > > dma_addr_t dma_addr;
> > > bool used;
> >
> > [...]
> >
> > > --
> > > 2.38.0
> >
> > Thanks,
> > Olek
>
> --
> /Horatiu
Thanks,
Olek