netdev - Re: [PATCH net-next v11 5/7] net: tn40xx: add basic Rx handling

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240619.152130.1121856489909363651.fujita.tomonori@gmail.com>
Date: Wed, 19 Jun 2024 15:21:30 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: kuba@...nel.org
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, andrew@...n.ch,
 horms@...nel.org, jiri@...nulli.us, pabeni@...hat.com,
 linux@...linux.org.uk, hfdevel@....net, naveenm@...vell.com,
 jdamato@...tly.com
Subject: Re: [PATCH net-next v11 5/7] net: tn40xx: add basic Rx handling

On Tue, 18 Jun 2024 18:52:47 -0700
Jakub Kicinski <kuba@...nel.org> wrote:

> On Tue, 18 Jun 2024 14:16:06 +0900 FUJITA Tomonori wrote:
>> +	netif_tx_lock(priv->ndev);
>> +	while (f->m.wptr != f->m.rptr) {
>> +		f->m.rptr += TN40_TXF_DESC_SZ;
>> +		f->m.rptr &= f->m.size_mask;
>> +		/* Unmap all fragments */
>> +		/* First has to come tx_maps containing DMA */
>> +		do {
>> +			dma_unmap_page(&priv->pdev->dev, db->rptr->addr.dma,
>> +				       db->rptr->len, DMA_TO_DEVICE);
>> +			tn40_tx_db_inc_rptr(db);
>> +		} while (db->rptr->len > 0);
>> +		tx_level -= db->rptr->len; /* '-' Because the len is negative */
>> +
>> +		/* Now should come skb pointer - free it */
>> +		dev_kfree_skb_any(db->rptr->addr.skb);
>> +		netdev_dbg(priv->ndev, "dev_kfree_skb_any %p %d\n",
>> +			   db->rptr->addr.skb, -db->rptr->len);
>> +		tn40_tx_db_inc_rptr(db);
>> +	}
> 
> Do you have to hold the Tx lock while unmapping the previous skbs?
> That's the most expensive part of the function, would be good to let
> other CPUs queue new packets at the same time.

I suppose that we can release the lock, unmap the dma address, then
acquire again like the following:

@@ -830,8 +828,13 @@ static void tn40_tx_cleanup(struct tn40_priv *priv)
 		/* Unmap all fragments */
 		/* First has to come tx_maps containing DMA */
 		do {
-			dma_unmap_page(&priv->pdev->dev, db->rptr->addr.dma,
-				       db->rptr->len, DMA_TO_DEVICE);
+			dma_addr_t addr = db->rptr->addr.dma;
+			size_t size =  db->rptr->len;
+
+			netif_tx_unlock(priv->ndev);
+			dma_unmap_page(&priv->pdev->dev, addr,
+				       size, DMA_TO_DEVICE);
+			netif_tx_lock(priv->ndev);
 			tn40_tx_db_inc_rptr(db);
 		} while (db->rptr->len > 0);
 		tx_level -= db->rptr->len; /* '-' Because the len is negative */