lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <d2c69906-4686-c2e1-0102-a73a2d9ca061@engleder-embedded.com> Date: Tue, 10 Jan 2023 22:21:42 +0100 From: Gerhard Engleder <gerhard@...leder-embedded.com> To: Alexander H Duyck <alexander.duyck@...il.com>, netdev@...r.kernel.org Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com, pabeni@...hat.com, Saeed Mahameed <saeed@...nel.org> Subject: Re: [PATCH net-next v4 08/10] tsnep: Add RX queue info for XDP support On 10.01.23 18:29, Alexander H Duyck wrote: > On Mon, 2023-01-09 at 20:15 +0100, Gerhard Engleder wrote: >> Register xdp_rxq_info with page_pool memory model. This is needed for >> XDP buffer handling. >> >> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com> >> Reviewed-by: Saeed Mahameed <saeed@...nel.org> >> --- >> drivers/net/ethernet/engleder/tsnep.h | 2 ++ >> drivers/net/ethernet/engleder/tsnep_main.c | 39 ++++++++++++++++------ >> 2 files changed, 31 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h >> index 855738d31d73..2268ff793edf 100644 >> --- a/drivers/net/ethernet/engleder/tsnep.h >> +++ b/drivers/net/ethernet/engleder/tsnep.h >> @@ -134,6 +134,8 @@ struct tsnep_rx { >> u32 dropped; >> u32 multicast; >> u32 alloc_failed; >> + >> + struct xdp_rxq_info xdp_rxq; >> }; >> > > Rather than placing it in your Rx queue structure it might be better to > look at placing it in the tsnep_queue struct. The fact is this is more > closely associated with the napi struct then your Rx ring so changing > it to that might make more sense as you can avoid a bunch of extra > overhead with having to pull napi to it. > > Basically what it comes down it is that it is much easier to access > queue[i].rx than it is for the rx to get access to queue[i].napi. I will try it as suggested. >> struct tsnep_queue { >> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c >> index 0c9669edb2dd..451ad1849b9d 100644 >> --- a/drivers/net/ethernet/engleder/tsnep_main.c >> +++ b/drivers/net/ethernet/engleder/tsnep_main.c >> @@ -792,6 +792,9 @@ static void tsnep_rx_ring_cleanup(struct tsnep_rx *rx) >> entry->page = NULL; >> } >> >> + if (xdp_rxq_info_is_reg(&rx->xdp_rxq)) >> + xdp_rxq_info_unreg(&rx->xdp_rxq); >> + >> if (rx->page_pool) >> page_pool_destroy(rx->page_pool); >> >> @@ -807,7 +810,7 @@ static void tsnep_rx_ring_cleanup(struct tsnep_rx *rx) >> } >> } >> >> -static int tsnep_rx_ring_init(struct tsnep_rx *rx) >> +static int tsnep_rx_ring_init(struct tsnep_rx *rx, unsigned int napi_id) >> { >> struct device *dmadev = rx->adapter->dmadev; >> struct tsnep_rx_entry *entry; >> @@ -850,6 +853,15 @@ static int tsnep_rx_ring_init(struct tsnep_rx *rx) >> goto failed; >> } >> >> + retval = xdp_rxq_info_reg(&rx->xdp_rxq, rx->adapter->netdev, >> + rx->queue_index, napi_id); >> + if (retval) >> + goto failed; >> + retval = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq, MEM_TYPE_PAGE_POOL, >> + rx->page_pool); >> + if (retval) >> + goto failed; >> + >> for (i = 0; i < TSNEP_RING_SIZE; i++) { >> entry = &rx->entry[i]; >> next_entry = &rx->entry[(i + 1) % TSNEP_RING_SIZE]; >> @@ -1104,7 +1116,8 @@ static bool tsnep_rx_pending(struct tsnep_rx *rx) >> } >> >> static int tsnep_rx_open(struct tsnep_adapter *adapter, void __iomem *addr, >> - int queue_index, struct tsnep_rx *rx) >> + unsigned int napi_id, int queue_index, >> + struct tsnep_rx *rx) >> { >> dma_addr_t dma; >> int retval; >> @@ -1118,7 +1131,7 @@ static int tsnep_rx_open(struct tsnep_adapter *adapter, void __iomem *addr, >> else >> rx->offset = TSNEP_SKB_PAD; >> >> - retval = tsnep_rx_ring_init(rx); >> + retval = tsnep_rx_ring_init(rx, napi_id); >> if (retval) >> return retval; >> >> @@ -1245,14 +1258,19 @@ static void tsnep_free_irq(struct tsnep_queue *queue, bool first) >> static int tsnep_netdev_open(struct net_device *netdev) >> { >> struct tsnep_adapter *adapter = netdev_priv(netdev); >> - int i; >> - void __iomem *addr; >> int tx_queue_index = 0; >> int rx_queue_index = 0; >> - int retval; >> + unsigned int napi_id; >> + void __iomem *addr; >> + int i, retval; >> >> for (i = 0; i < adapter->num_queues; i++) { >> adapter->queue[i].adapter = adapter; >> + >> + netif_napi_add(adapter->netdev, &adapter->queue[i].napi, >> + tsnep_poll); >> + napi_id = adapter->queue[i].napi.napi_id; >> + > > This is a good example of what I am referring to. > >> if (adapter->queue[i].tx) { >> addr = adapter->addr + TSNEP_QUEUE(tx_queue_index); >> retval = tsnep_tx_open(adapter, addr, tx_queue_index, >> @@ -1263,7 +1281,7 @@ static int tsnep_netdev_open(struct net_device *netdev) >> } >> if (adapter->queue[i].rx) { >> addr = adapter->addr + TSNEP_QUEUE(rx_queue_index); >> - retval = tsnep_rx_open(adapter, addr, >> + retval = tsnep_rx_open(adapter, addr, napi_id, >> rx_queue_index, >> adapter->queue[i].rx); >> if (retval) >> @@ -1295,8 +1313,6 @@ static int tsnep_netdev_open(struct net_device *netdev) >> goto phy_failed; >> >> for (i = 0; i < adapter->num_queues; i++) { >> - netif_napi_add(adapter->netdev, &adapter->queue[i].napi, >> - tsnep_poll); >> napi_enable(&adapter->queue[i].napi); >> >> tsnep_enable_irq(adapter, adapter->queue[i].irq_mask); > > What you could do here is look at making all the napi_add/napi_enable > and xdp_rxq_info items into one function to handle all the enabling. I will rework that. >> @@ -1317,6 +1333,8 @@ static int tsnep_netdev_open(struct net_device *netdev) >> tsnep_rx_close(adapter->queue[i].rx); >> if (adapter->queue[i].tx) >> tsnep_tx_close(adapter->queue[i].tx); >> + >> + netif_napi_del(&adapter->queue[i].napi); >> } >> return retval; >> } >> @@ -1335,7 +1353,6 @@ static int tsnep_netdev_close(struct net_device *netdev) >> tsnep_disable_irq(adapter, adapter->queue[i].irq_mask); >> >> napi_disable(&adapter->queue[i].napi); >> - netif_napi_del(&adapter->queue[i].napi); >> >> tsnep_free_irq(&adapter->queue[i], i == 0); >> > > Likewise here you could take care of all the same items with the page > pool being freed after you have already unregistered and freed the napi > instance. I'm not sure if I understand it right. According to your suggestion above napi and xdp_rxq_info should be freed here? Gerhard
Powered by blists - more mailing lists