From: Eliezer Tamir <eliezer.tamir@linux.intel.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Miller <davem@davemloft.net>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Network Development <netdev@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
David Woodhouse <dwmw2@infradead.org>,
Eliezer Tamir <eliezer@tamir.org.il>
Subject: Re: [PATCH net-next] net: rename low latency sockets functions to busy poll
Date: Mon, 08 Jul 2013 20:14:27 +0300 [thread overview]
Message-ID: <51DAF373.4040606@linux.intel.com> (raw)
In-Reply-To: <CA+55aFxgrOtsv7vCtic97ehpvqyezCzDcyqHVD+821BiTuiRtw@mail.gmail.com>
On 08/07/2013 19:37, Linus Torvalds wrote:
> On Mon, Jul 8, 2013 at 6:20 AM, Eliezer Tamir
> <eliezer.tamir@linux.intel.com> wrote:
>>
>> - /* only if on, have sockets with POLL_LL and not out of time */
>> - if (ll_flag && can_ll && can_poll_ll(ll_start, ll_time))
>> + /* only if found POLL_BUSY_LOOP sockets && not out of time */
>> + if (!need_resched() && can_busy_loop &&
>> + busy_loop_range(busy_start, busy_end))
>> continue;
>
> Better, but still horribly ugly. I also suspect the need_resched()
> test should be done after testing can_busy_loop, just in case the
> compiler can avoid having to load things off the current pointer.
I think there is no way for the compiler to know the value of
can_busy_loop at compile time. It depends on the replies we get
from polling the sockets. ll_flag was there to make sure the compiler
will know when things are defined out.
I would be very happy to hear suggestions for better names for things.
> I also think that we should clear busy_flag if we don't continue, no?
I'm not sure. If the time the user specified to busy-poll is not over,
and the reason we didn't do it last time was that the sockets did not
report that they have valid polling information (perhaps a route changed
or a device we reset), but how we do have sockets that can busy-poll,
wouldn't polling be the right thing to do?
> I also don't understand why the code keeps both busy_start and
> busy_end around. It all looks completely pointless. Why have two
> variables, and why make the comparisons more complicated, and the code
> look more complex than it actually is?
Originally the code used time_after() and only kept the start time.
People on the list voiced concerns that using sched_clock() might be
risky since we may end up on another CPU with a different time.
We used sched_clock() because we don't need the time to be very
accurate, this is only a cut-off time to make sure we never spin
forever when no event ever happens.
I then changed this to use time_in_range() so that if we wake up with a
completely random time, we will be out of the range and fail safely.
If you think that is wrong we can go back to use time_after().
> I also suspect there's a lot of other micro-optimizations that could
> be done: while keeping *two* 64-bit timeouts is just completely insane
> on a 32-bit architecture, keeping even just one is debatable. I
> suspect the timeouts should be just "unsigned long", so that 32-bit
> architectures don't bother with unnecessary 64-bit clocks. 32-bit
> clocks are several seconds even if you count nanoseconds, and you
> actually only do microseconds anyway (so instead of shifting the
> microseconds left by ten like you do, shift the nanoseconds of
> sched_clock right by ten, and now you only need 32-bit values).
OK, but please answer my questions above, it is starting to be late here
and I would really like to send a fix that everyone will find
acceptable today.
Thanks,
Eliezer
next prev parent reply other threads:[~2013-07-08 17:14 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-08 13:20 [PATCH net-next] net: rename low latency sockets functions to busy poll Eliezer Tamir
2013-07-08 16:37 ` Linus Torvalds
2013-07-08 17:14 ` Eliezer Tamir [this message]
2013-07-08 19:37 ` Linus Torvalds
2013-07-08 19:46 ` Eliezer Tamir
2013-07-08 19:59 ` Stephen Hemminger
2013-07-08 20:05 ` Stephen Hemminger
2013-07-08 20:10 ` Linus Torvalds
2013-07-09 2:27 ` David Miller
2013-07-09 22:25 ` Jonathan Corbet
2013-07-09 23:06 ` David Miller
2013-07-10 3:29 ` Eliezer Tamir
2013-07-10 4:41 ` David Miller
2013-07-10 5:21 ` Eliezer Tamir
2013-07-10 6:10 ` Eric Dumazet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51DAF373.4040606@linux.intel.com \
--to=eliezer.tamir@linux.intel.com \
--cc=akpm@linux-foundation.org \
--cc=davem@davemloft.net \
--cc=dwmw2@infradead.org \
--cc=eliezer@tamir.org.il \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).