netdev - Re: [PATCH v4 2/9] bgmac: leave interrupts disabled as long as there is work to do

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]

Date:	Mon, 13 Apr 2015 18:35:12 +0200
From:	Rafał Miłecki <zajec5@...il.com>
To:	Felix Fietkau <nbd@...nwrt.org>
Cc:	Network Development <netdev@...r.kernel.org>,
	Hauke Mehrtens <hauke@...ke-m.de>
Subject: Re: [PATCH v4 2/9] bgmac: leave interrupts disabled as long as there
 is work to do

On 13 April 2015 at 17:44, Felix Fietkau <nbd@...nwrt.org> wrote:
> On 2015-04-13 17:06, Rafał Miłecki wrote:
>> On 13 April 2015 at 17:03, Felix Fietkau <nbd@...nwrt.org> wrote:
>>> On 2015-04-13 16:34, Rafał Miłecki wrote:
>>>> On 13 April 2015 at 15:52, Felix Fietkau <nbd@...nwrt.org> wrote:
>>>>> Always poll rx and tx during NAPI poll instead of relying on the status
>>>>> of the first interrupt. This prevents bgmac_poll from leaving unfinished
>>>>> work around until the next IRQ.
>>>>> In my tests this makes bridging/routing throughput under heavy load more
>>>>> stable and ensures that no new IRQs arrive as long as bgmac_poll uses up
>>>>> the entire budget.
>>>>
>>>> What do you think about keeping u32 int_status; and just updating it
>>>> at the end of bgmac_poll? In case you decide to implement multiple TX
>>>> queues, it may be cheaper to just check a single bit in memory instead
>>>> reading DMA ring status.
>>> Events might arrive in the mean time. I ran some tests, and not checking
>>> the irq status for processing rx/tx gave me fewer total IRQs under load.
>>
>> But you do check the status, I mean the following line:
>> if (bgmac_read(bgmac, BGMAC_INT_STATUS) & (BGMAC_IS_TX0 | BGMAC_IS_RX))
>>
>> Would it make sense to do
>> bgmac->int_status = bgmac_read(bgmac, BGMAC_INT_STATUS);
> I don't really see the point in storing the status from the initial IRQ.
>
> The check there is only to decide whether the poll function should run
> at all. By the time bgmac_poll is called, more events may have arrived
> already, making the initial status useless.

Ah, I didn't think about delay between bgmac_poll calls.

All my point was to not call bgmac_dma_tx_free or bgmac_dma_rx_read
when not needed. Imagine having 4 TX queues and 1 RX queue. You will
read 4 BGMAC_DMA_TX_STATUS registers and 1 BGMAC_DMA_RX_STATUS
registers plus do some calculations every time. By reading
BGMAC_INT_STATUS (and checking its bits) you could avoid this. In
theory a very tiny optimization, no idea if worth implement, I'll
leave it up to you.

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html