netdev - Re: [PATCH] net: forcedeth: add xmit

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: <84839e5f-4543-bbd9-37db-e1777a84992c@oracle.com>
Date:   Mon, 21 Oct 2019 17:56:06 +0800
From:   Zhu Yanjun <yanjun.zhu@...cle.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     rain.1986.08.12@...il.com, davem@...emloft.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH] net: forcedeth: add xmit_more support


On 2019/10/19 6:48, Jakub Kicinski wrote:
> On Fri, 18 Oct 2019 06:01:25 -0400, Zhu Yanjun wrote:
>> This change adds support for xmit_more based on the igb commit 6f19e12f6230
>> ("igb: flush when in xmit_more mode and under descriptor pressure") and
>> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data") that
>> were made to igb to support this feature. The function netif_xmit_stopped
>> is called to check if transmit queue on device is currently unable to send
>> to determine if we must write the tail because we can add no further
>> buffers.
>> When normal packets and/or xmit_more packets fill up tx_desc, it is
>> necessary to trigger NIC tx reg.
> Looks broken. You gotta make sure you check the kick on _every_ return
> path. There are 4 return statements in each function, you only touched
> 2.

In nv_start_xmit,

2240         if (unlikely(empty_slots <= entries)) {
2241                 netif_stop_queue(dev);
2242                 np->tx_stop = 1;
2243                 spin_unlock_irqrestore(&np->lock, flags);
2244
2245                 /* When normal packets and/or xmit_more packets fill up
2246                  * tx_desc, it is necessary to trigger NIC tx reg.
2247                  */
2248                 ret = NETDEV_TX_BUSY;
2249                 goto TXKICK;
2250         }
The above indicates tx_desc is full, it is necessary to trigger NIC HW xmit.

2261                 if (unlikely(dma_mapping_error(&np->pci_dev->dev,
2262 np->put_tx_ctx->dma))) {
2263                         /* on DMA mapping error - drop the packet */
2264                         dev_kfree_skb_any(skb);
2265 u64_stats_update_begin(&np->swstats_tx_syncp);
2266                         nv_txrx_stats_inc(stat_tx_dropped);
2267 u64_stats_update_end(&np->swstats_tx_syncp);
2268                         return NETDEV_TX_OK;
2269                 }

and

2300                         if 
(unlikely(dma_mapping_error(&np->pci_dev->dev,
2301 np->put_tx_ctx->dma))) {
2302
2303                                 /* Unwind the mapped fragments */
2304                                 do {
2305                                         nv_unmap_txskb(np, 
start_tx_ctx);
2306                                         if (unlikely(tmp_tx_ctx++ 
== np->last_tx_ctx))
2307                                                 tmp_tx_ctx = 
np->tx_skb;
2308                                 } while (tmp_tx_ctx != np->put_tx_ctx);
2309                                 dev_kfree_skb_any(skb);
2310                                 np->put_tx_ctx = start_tx_ctx;
2311 u64_stats_update_begin(&np->swstats_tx_syncp);
2312 nv_txrx_stats_inc(stat_tx_dropped);
2313 u64_stats_update_end(&np->swstats_tx_syncp);
2314                                 return NETDEV_TX_OK;
2315                         }

The above are dma_mapping_error. It seems that triggering NIC HW xmit is 
not needed.

So when "tx_desc full" error, HW NIC xmit is triggerred. When 
dma_mapping_error,

NIC HW xmit is not triggerred.

That is why only 2 "return" are touched.

>
> Also the labels should be lower case.

This patch passes checkpatch.pl. It seems that "not lower case" is not a 
problem?

If you think it is a problem, please show me where it is defined.

Zhu Yanjun

>