netdev - Re: [PATCH net-next] r8169: Support for byte queue limits

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:	Fri, 02 Dec 2011 16:11:27 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	igorm@....rs
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	Realtek linux nic maintainers <nic_swsd@...ltek.com>,
	Francois Romieu <romieu@...zoreil.com>,
	Tom Herbert <therbert@...gle.com>
Subject: Re: [PATCH net-next] r8169: Support for byte queue limits

Le vendredi 02 décembre 2011 à 15:54 +0100, Igor Maravić a écrit :
> > Hmm... What about rtl8169_tx_clear_range() or rtl8169_tx_clear() ?
> >
> 
> You mean we should put netdev_reset_queue() in those functions?
> 
> I think that we shouldn't put any thing in these functions.
> 
> When ever they are called, they are precede/followed by function
> rtl8169_hw_reset() which calls rtl_hw_reset() which calls
> rtl8169_init_ring_indexes() in which I've put netdev_reset_queue()
> 
> Only time when rtl8169_tx_clear_range() isn't preceded/followed by
> rtl8169_hw_reset() is in function rtl8169_xmit_frags(). In that
> function it's
> called only in case there's an error. In case there was an error in that
> function, netdev_sent_queue() will never be called, so we are going to have
> accurate length of queue.

Quite frankly, any queued skb is eventually freed, so
netdev_reset_queue() could be avoided.

r8169 driver has some problems (see current threads on netdev about it),
and its not very clear that BQL locking requirements are going to be
respected. This can lead to inaccurate BQL counts and a txqueue freeze.

(In particular, a chip reset can be triggered by an IRQ, while another
cpu is currently sending an skb through ndo_start_xmit())

So I advise not using netdev_reset_queue() in this driver, only
netdev_completed_queue(), and make sure that their calls are serialized
under a given lock (see include/linux/dynamic_queue_limits.h comments)





--
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