Re: [PATCH v5 1/2] printk: Add console owner and waiter logic to load balance console writes

[Steven Rostedt]

On Fri, 19 Jan 2018 11:37:13 +0900
Byungchul Park <byungchul.park@xxxxxxx> wrote:

> On 1/19/2018 12:21 AM, Steven Rostedt wrote:
> > On Thu, 18 Jan 2018 13:01:46 +0900
> > Byungchul Park <byungchul.park@xxxxxxx> wrote:
> >   
> >>> I disagree. It is like a spinlock. You can say a spinlock() that is
> >>> blocked is also waiting for an event. That event being the owner does a
> >>> spin_unlock().  
> >>
> >> That's exactly what I was saying. Excuse me but, I don't understand
> >> what you want to say. Could you explain more? What do you disagree?  
> > 
> > I guess I'm confused at what you are asking for then.  
> 
> Sorry for not enough explanation. What I asked you for is:
> 
>     1. Relocate acquire()s/release()s.
>     2. So make it simpler and remove unnecessary one.
>     3. So make it look like the following form,
>        because it's a thing simulating "wait and event".
> 
>        A context
>        ---------
>        lock_map_acquire(wait); /* Or lock_map_acquire_read(wait) */
>                                /* "Read" one is better though..    */

why? I'm assuming you are talking about adding this to the current
owner off the console_owner? This is a mutually exclusive section, no
parallel access. Why the Read?

> 
>        /* A section, we suspect a wait for an event might happen. */
>        ...
> 
>        lock_map_release(wait);
> 
>        The place actually doing the wait
>        ---------------------------------
>        lock_map_acquire(wait);
>        lock_map_release(wait);
> 
>        wait_for_event(wait); /* Actually do the wait */
> 
> Honestly, you used acquire()s/release()s as if they are cross-
> release stuff which mainly handles general waits and events,
> not only things doing "acquire -> critical area -> release".
> But that's not in the mainline at the moment.

Maybe it is more like that. Because, the thing I'm doing is passing off
a semaphore ownership to the waiter.

>From a previous email:

> > +			if (spin) {
> > +				/* We spin waiting for the owner to release us */
> > +				spin_acquire(&console_owner_dep_map, 0, 0, _THIS_IP_);
> > +				/* Owner will clear console_waiter on hand off */
> > +				while (READ_ONCE(console_waiter))
> > +					cpu_relax();
> > +
> > +				spin_release(&console_owner_dep_map, 1, _THIS_IP_);  
> 
> Why don't you move this over "while (READ_ONCE(console_waiter))" and
> right after acquire()?
> 
> As I said last time, only acquisitions between acquire() and release()
> are meaningful. Are you taking care of acquisitions within cpu_relax()?
> If so, leave it.

There is no acquisitions between acquire and release. To get to 
"if (spin)" the acquire had to already been done. If it was released,
this spinner is now the new "owner". There's no race with anyone else.
But it doesn't technically have it till console_waiter is set to NULL.
Why would we call release() before that? Or maybe I'm missing something.

Or are you just saying that it doesn't matter if it is before or after
the while() loop, to just put it before? Does it really matter?

-- Steve