Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying.

[Petr Mladek]

On Fri 2017-01-13 11:28:43, Sergey Senozhatsky wrote:
> On (01/12/17 15:18), Petr Mladek wrote:
> > On Mon 2016-12-26 20:34:07, Sergey Senozhatsky wrote:
> > > console_trylock() used to always forbid rescheduling; but it got changed
> > > like a yaer ago.
> > > 
> > > the other thing is... do we really need to console_conditional_schedule()
> > > from fbcon_*()? console_unlock() does cond_resched() after every line it
> > > prints. wouldn't that be enough?
> > > 
> > > so may be we can drop some of console_conditional_schedule()
> > > call sites in fbcon. or update console_conditional_schedule()
> > > function to always return the current preemption value, not the
> > > one we saw in console_trylock().
> > 
> > I was curious if it makes sense to remove
> > console_conditional_schedule() completely.
> 
> I was looking at this option at some point as well.
> 
> > In practice, it never allows rescheduling when the console driver
> > is called via console_unlock(). It is since 2006 and the commit
> > 78944e549d36673eb62 ("vt: printk: Fix framebuffer console
> > triggering might_sleep assertion"). This commit added
> > that
> > 
> > 	console_may_schedule = 0;
> >
> > into console_unlock() before the console drivers are called.
> > 
> > 
> > On the other hand, it seems that the rescheduling was always
> > enabled when some console operations were called via
> > tty_operations. For example:
> > 
> > struct tty_operations con_ops
> > 
> >   con_ops->con_write()
> >   -> do_con_write()  #calls console_lock()
> >    -> do_con_trol()
> >     -> fbcon_scroll()
> >      -> fbcon_redraw_move()
> >       -> console_conditional_schedule()
> > 
> > , where console_lock() sets console_may_schedule = 1;
> > 
> > 
> > A complete console scroll/redraw might take a while. The rescheduling
> > would make sense => IMHO, we should keep console_conditional_schedule()
> > or some alternative in the console drivers as well.
> > 
> > But I am afraid that we could not use the automatic detection.
> > We are not able to detect preemption when CONFIG_PREEMPT_COUNT
> 
> can one actually have a preemptible kernel with !CONFIG_PREEMPT_COUNT?
> how? it's not even possible to change CONFIG_PREEMPT_COUNT in menuconfig.
> the option is automatically selected by PREEMPT. and if PREEMPT is not
> selected then _cond_resched() is just "{ rcu_all_qs(); return 0; }"

CONFIG_PREEMPT_COUNT is always enabled in preemptive kernel. But
we do not mind about preemtible kernel. It reschedules automatically
anywhere in preemptive context.

The problem is non-preemptive kernel. It is able to reschedule
only when someone explicitely calls cond_resched() or schedule().
In this case, we are able to detect the preemtive context
automatically only with CONFIG_PREEMPT_COUNT enabled.
We must not call cond_resched() if we are not sure.

> ...
> > We cannot put the automatic detection into console_conditional_schedule().
> 
> why can't we?

Because it would newer call cond_resched() in non-preemptive kernel
with CONFIG_PREEMPT_COUNT disabled. IMHO, we want to call it,
for example, when we scroll the entire screen from tty_operations.

Or do I miss anything?


> > I am going to prepare a patch for this.
> 
> I'm on it.

Uff, I already have one and am very close to send it.

Sigh, I do not want to race who will prepare and send the patch.
I just do not feel comfortable in the reviewer-only role.
I feel like just searching for problems in other's patches
and annoying them with my complains. I know that it is important
but I also want to produce something.

Also I feel that I still need to improve my coding skills.
And I need some training.

Finally, I would not start writing my patch if your one needed
only small updates. But my investigation pushed me very
different way from your proposal. It looked ugly to push
all coding to your side.

Best Regards,
Petr