Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of console_trylock callers

[Sergey Senozhatsky]

Hello,

On (01/22/16 10:48), Petr Mladek wrote:
[..]
> > and in console_unlock()
> > 
> > -               if (do_cond_resched)
> > -                       cond_resched();
> > +               console_conditional_schedule();
> >
> >
> > but for !CONFIG_PREEMPT_COUNT we can't. because of currently held spin_locks/etc
> > that we don't know about.
> 
> Ah, I was not aware that we did not have information about preemption
> without PREEMPT_COUNT.

yes, for example,

static inline void __raw_spin_lock(raw_spinlock_t *lock)
{
	preempt_disable();
	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
}

where preempt_disable()  include/linux/preempt.h

...
#else /* !CONFIG_PREEMPT_COUNT */

/*
 * Even if we don't have any preemption, we need preempt disable/enable
 * to be barriers, so that we don't have things like get_user/put_user
 * that can cause faults and scheduling migrate into our preempt-protected
 * region.
 */
#define preempt_disable()			barrier()
#define preempt_enable()			barrier()


so on !CONFIG_PREEMPT_COUNT kernels we can't rely on console_trylock()
'magic', we need the existing rules.

> > `console_may_schedule' carries a bit of important information for
> > console_conditional_schedule() caller. if it has acquired console_sem
> > via console_lock() - then it can schedule, if via console_trylock() - it cannot.
> > 
> > the last `if via console_trylock() - it cannot' rule is not always true,
> > we clearly can have printk()->console_unlock() from non-atomic contexts
> > (if we know that its non-atomic, which is not the case with !PREEMPT_COUNT).
> 
> By other words, we could automatically detect save context for
> cond_resched() only if PREEMPT_COUNT is enabled. Otherwise, we need to
> keep the current logic (heuristic). Do I get it correctly, please?

yes, I think so.

> I would personally wait a bit for Jack's async console printing.
> It will call console only if oops_in_progress is set. It means that
> this partial optimization won't be needed at all.

ok, thanks. I'd love to see Jan's printk() rework being merged. I have 99 problems
with printk() and console_unlock(). People usually are not aware of the secrets that
printk-console_unlock have; and tend to think that printk is just 'a kernel way' of
spelling printf, with all the consequences that follows -- excessive printk usage,
RCU stalls, soft lockups, etc. And that printk abuse does not necessarily hit the
abuser. A completely 'innocent' user space application that does a syscall which
involves console_lock-console_unlock, can spend seconds in console_unlock pushing
someone's data to console_drivers. console_lock and console_unlock, I think, have a
bit misleading naming. _lock has acquire semantics, _unlock, however, does not
simply release the lock. I even think it'd be good to have console_unlock_fast(),
that would just up_console_sem() w/o any penalty. So some of console_unlock() that
are 'accessible' by user-space /* for example,
  tty_open()
    tty_lookup_driver()
      console_device()
        console_lock()
        console_unlock()

or reading from /proc/consoles, and so on and forth */
could be replaced with console_unlock_fast().

The patch in question is simply a further extension on Tejun's work. And
these two patches already made my life a bit simpler, albeit not all of the
printk/console_unlock problems were addressed.

Jan's patch set is a much more complicated effort, and it may take 2 or
3 (??) kernel releases to finish (there are corner cases: for example,
workers can stall during OOM, etc.), I'd be happy to see it in -next for 4.6,
personally, not sure how realistic this expectation is.

> The other (first) patch still makes sense in the simplified form.

thanks. let's do it this way - I'll keep the preempt disable/enable
removal patch the last in the series, so we can easily drop it (if
Jan's rework is much-much closer). How does that sound?

	-ss

> Best Regards,
> Petr
>