netdev - RE: [PATCH 1/2] qdisc_restart - readability changes, one bug fix.

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: <1182139885.6082.3.camel@localhost.localdomain>
Date:	Mon, 18 Jun 2007 09:41:25 +0530
From:	Krishna Kumar <kumarkr@...ux.vnet.ibm.com>
To:	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>
Cc:	davem@...emloft.net, hadi@...erus.ca, herbert@...dor.apana.org.au,
	tgraf@...g.ch, kaber@...sh.net, netdev@...r.kernel.org
Subject: RE: [PATCH 1/2] qdisc_restart - readability changes, one bug fix.

On Fri, 2007-06-15 at 11:01 -0700, Waskiewicz Jr, Peter P wrote:

Hi Peter,

> I agree that the case shouldn't happen, and will only surface if the
> driver is indeed buggy.  I've thought about this conditional being
> removed for awhile, since it will protect against a poorly written
> driver wrt locking, but then again a driver behaving like that shouldn't
> be making it into the kernel.  That being said, out-of-tree drivers are
> heavily used (we have an out-of-tree e1000), and something like this
> could be missed.  But since that is the corner case of a crappy driver,
> I'm ok with this being removed.

Removing this check will work more efficiently for correctly written
drivers, but it will also catch the wrongly written drivers and free
up all the skbs (and print messages) as without this change, nothing
happens and resources are never freed up.

> These, at a glance, look very similar to the qdisc ->enqueue() and
> ->dequeue() function pointers.  I know I had to look a few times to
> realize they were separate calls through the qdisc and this new function
> name.  Perhaps dequeue_skb() can become dev_dequeue_skb() and
> requeue_skb() can be dev_requeue_skb()?  Something that helps
> differentiate these from the function pointers of the qdisc_ops might
> help distinguish what layer the operation is running on.

That is definitely required, I will send out a revised patch.

> I know there's been discussion on the driver side of the world to stop
> using unlikely() and likely() clauses.  I personally think it's ok in
> situations like this where you have one corner-case conditional without
> an else, but it's something to keep in mind when using them in
> new/rewritten code like this.

OK. The reason I had put the unlikely is that we are executing qdisc_restart
due to the synchronous queuing of a packet, or since we were rescheduled
after either :
	a. not getting tx lock (and when qdisc_restart had atleast one
	   packet in the queue, look for the qdisc_qlen return value in
	   handle_dev_cpu_collision).
	b. tx was busy and packet was requeue'd.

So in most (all?) cases, an skb should be found (I was not able to find a
case where skb can be NULL).

> The patch looks fine to me outside of the choice of names for
> dequeue_skb() and requeue_skb(), but I can be easily swayed.

I will send an updated patch for review. Thanks for your review comments.

- KK


-
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