Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable

[Jason Wang <jasowang@redhat.com>]

On 09/01/2014 06:19 PM, Peter Zijlstra wrote:
> On Mon, Sep 01, 2014 at 12:04:34PM +0200, Peter Zijlstra wrote:
>> > On Mon, Sep 01, 2014 at 12:52:19PM +0300, Michael S. Tsirkin wrote:
>>> > > On Mon, Sep 01, 2014 at 11:31:59AM +0200, Peter Zijlstra wrote:
>>>> > > > On Fri, Aug 22, 2014 at 07:01:05AM +0200, Mike Galbraith wrote:
>>>>>> > > > > > +++ b/include/net/busy_poll.h
>>>>>> > > > > > @@ -109,7 +109,8 @@ static inline bool sk_busy_loop(struct sock *sk, int nonblock)
>>>>>> > > > > >  		cpu_relax();
>>>>>> > > > > >  
>>>>>> > > > > >  	} while (!nonblock && skb_queue_empty(&sk->sk_receive_queue) &&
>>>>>> > > > > > -		 !need_resched() && !busy_loop_timeout(end_time));
>>>>>> > > > > > +		 !need_resched() && !busy_loop_timeout(end_time) &&
>>>>>> > > > > > +		 nr_running_this_cpu() < 2);
>>>>>> > > > > >  
>>>> > > > 
>>>> > > > So as has been said by now; this is horrible.
>>>> > > > 
>>>> > > > We should not export nr_running like this ever. Your usage of < 2
>>>> > > > implies this can be hit with nr_running == 0, and therefore you can also
>>>> > > > hit it with nr_running == 1 where the one is not network related and you
>>>> > > > get random delays.
>>>> > > > 
>>>> > > > Worse still, you have BH (and thereby preemption) disabled, you should
>>>> > > > not _ever_ have undefined and indefinite waits like that.
>>>> > > > 
>>>> > > > You also destroy any hope of dropping into lower power states; even when
>>>> > > > there's never going to be a packet ever again, also bad.
>>> > > 
>>> > > Hmm this patch sometimes makes us exit from the busy loop *earlier*.
>>> > > How can this interfere with dropping into lower power states?
>> > 
>> > Ah.. jetlag.. :/ I read it like it owuld indefinitely spin if there was
>> > only the 'one' task, not avoid the spin unless there was the one task.
>> > 
>> > The nr_running thing is still horrible, but let me reread this patch
>> > description to see if it explains why that is a good thing.
> OK I suppose that more or less makes sense, the contextual behaviour is
> of course tedious in that it makes behaviour less predictable. The
> 'other' tasks might not want to generate data and you then destroy
> throughput by not spinning.

The patch try to make sure:
- the the performance of busy read was not worse than it was disabled in
any cases.
- the performance improvement of a single socket was not achieved by
sacrificing the total performance (all other processes) of the system
 
If 'other' tasks are also CPU or I/O intensive jobs, we switch to do
them so the total performance were kept or even increased, and the
performance of current process were guaranteed not worse than when busy
read was disabled (or even better since it may still do busy read
sometimes when it was the only runnable process). If 'other' task are
not intensive, they just do little work and sleep soon, then the busy
read can still work in most of the time during the future reads, we may
still get obvious improvements.
> I'm not entirely sure I see how its all supposed to work though; the
> various poll functions call sk_busy_poll() and do_select() also loops.
>
> The patch only kills the sk_busy_poll() loop, but then do_select() will
> still loop and not sleep, so how is this helping?

Yes, the patch only help for processes who did a blocking reads (busy
read). For select(), maybe we can do the same thing but need more test
and thoughts.