Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
On Thu, May 07, 2015 at 02:14:39PM +0200, David Hildenbrand wrote: > Thanks :), well just to make sure I got your opinion on this correctly: > > 1. You think that 2 counters is the way to go for now ack > 2. You agree that we can't replace preempt_disable()+pagefault_disable() with > preempt_disable() (CONFIG_PREEMPT stuff), so we need to have them separately ack > 3. We need in_atomic() (in the fault handlers only!) in addition to make sure > we > don't mess with irq contexts (In that case I would add a good comment to that > place, describing why preempt_disable() won't help) ack > I think this is the right way to go because: > > a) This way we don't have to modify preempt_disable() logic (including > PREEMPT_COUNT). > > b) There are not that many users relying on > preempt_disable()+pagefault_disable() (compared to pure preempt_disable() or > pagefault_disable() users), so the performance overhead of two cache lines > should be small. Users only making use of one of them should see no difference > in performance. indeed. > c) We correctly decouple preemption and pagefault logic. Therefore we can now > preempt when pagefaults are disabled, which feels right. Right, that's always been the intent of introducing pagefault_disable(). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
* David Hildenbrand wrote: > @Ingo, do you have a strong feeling against this whole > patchset/idea? No objections, sounds good to me now. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
> On Thu, May 07, 2015 at 01:40:30PM +0200, David Hildenbrand wrote:
> > But anyhow, opinions seem to differ how to best handle that whole stuff.
> >
> > I think a separate counter just makes sense, as we are dealing with two
> > different concepts and we don't want to lose the preempt_disable =^ NOP
> > for !CONFIG_PREEMPT.
> >
> > I also think that
> >
> > pagefault_disable()
> > rt = copy_from_user()
> > pagefault_enable()
> >
> > is a valid use case.
> >
> > So any suggestions how to continue?
>
>
> static inline bool __pagefault_disabled(void)
> {
> return current->pagefault_disabled;
> }
>
> static inline bool pagefault_disabled(void)
> {
> return in_atomic() || __pagefault_disabled();
> }
>
> And leave the preempt_disable() + pagefault_disable() for now. You're
> right in that that is clearest.
>
> If we ever get to the point where that really is an issue, I'll try and
> be clever then :-)
>
Thanks :), well just to make sure I got your opinion on this correctly:
1. You think that 2 counters is the way to go for now
2. You agree that we can't replace preempt_disable()+pagefault_disable() with
preempt_disable() (CONFIG_PREEMPT stuff), so we need to have them separately
3. We need in_atomic() (in the fault handlers only!) in addition to make sure we
don't mess with irq contexts (In that case I would add a good comment to that
place, describing why preempt_disable() won't help)
I think this is the right way to go because:
a) This way we don't have to modify preempt_disable() logic (including
PREEMPT_COUNT).
b) There are not that many users relying on
preempt_disable()+pagefault_disable() (compared to pure preempt_disable() or
pagefault_disable() users), so the performance overhead of two cache lines
should be small. Users only making use of one of them should see no difference
in performance.
c) We correctly decouple preemption and pagefault logic. Therefore we can now
preempt when pagefaults are disabled, which feels right.
d) We can get some of that -rt flavor into the base kernel
e) We don't require inatomic variants in pagefault_disable() context as Ingo
suggested (For me, this now feels wrong - I had a different opinion back then
when working on the first revision of this patchset).
We would use inatomic() because preempt_disable() behaves differently with
PREEMPT_COUNT, mixing concepts at the user level.
@Ingo, do you have a strong feeling against this whole patchset/idea?
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
On Thu, May 07, 2015 at 01:40:30PM +0200, David Hildenbrand wrote:
> But anyhow, opinions seem to differ how to best handle that whole stuff.
>
> I think a separate counter just makes sense, as we are dealing with two
> different concepts and we don't want to lose the preempt_disable =^ NOP
> for !CONFIG_PREEMPT.
>
> I also think that
>
> pagefault_disable()
> rt = copy_from_user()
> pagefault_enable()
>
> is a valid use case.
>
> So any suggestions how to continue?
static inline bool __pagefault_disabled(void)
{
return current->pagefault_disabled;
}
static inline bool pagefault_disabled(void)
{
return in_atomic() || __pagefault_disabled();
}
And leave the preempt_disable() + pagefault_disable() for now. You're
right in that that is clearest.
If we ever get to the point where that really is an issue, I'll try and
be clever then :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
On Thu, May 07, 2015 at 01:40:30PM +0200, David Hildenbrand wrote: > I think a separate counter just makes sense, as we are dealing with two > different concepts and we don't want to lose the preempt_disable =^ NOP > for !CONFIG_PREEMPT. Right, let me try and get my head on straight -- I'm so used to PREEMPT=y being the 'hard' case :-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
On Thu, May 07, 2015 at 01:23:35PM +0200, David Hildenbrand wrote: > > On Thu, May 07, 2015 at 12:50:53PM +0200, David Hildenbrand wrote: > > > Just to make sure we have a common understanding (as written in my cover > > > letter): > > > > > > Your suggestion won't work with !CONFIG_PREEMPT (!CONFIG_PREEMPT_COUNT). > > > If > > > there is no preempt counter, in_atomic() won't work. > > > > But there is, we _always_ have a preempt_count, and irq_enter() et al. > > _always_ increment the relevant bits. > > > > The thread_info::preempt_count field it never under PREEMPT_COUNT > > include/asm-generic/preempt.h provides stuff regardless of > > PREEMPT_COUNT. > > > > See how __irq_enter() -> preempt_count_add(HARDIRQ_OFFSET) -> > > __preempt_count_add() _always_ just works. > > > > Its only things like preempt_disable() / preempt_enable() that get > > munged depending on PREEMPT_COUNT/PREEMPT. > > > > Sorry for the confusion. Sure, there is always the count. > > My point is that preempt_disable() won't result in an in_atomic() == true > with !PREEMPT_COUNT, so I don't see any point in adding in to the pagefault > handlers. It is not reliable. It _very_ reliably tells if we're in interrupts! Which your patches break. It also very much avoids touching two cachelines in a number of places. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
> On Thu, May 07, 2015 at 12:50:53PM +0200, David Hildenbrand wrote: > > Just to make sure we have a common understanding (as written in my cover > > letter): > > > > Your suggestion won't work with !CONFIG_PREEMPT (!CONFIG_PREEMPT_COUNT). If > > there is no preempt counter, in_atomic() won't work. > > But there is, we _always_ have a preempt_count, and irq_enter() et al. > _always_ increment the relevant bits. > > The thread_info::preempt_count field it never under PREEMPT_COUNT > include/asm-generic/preempt.h provides stuff regardless of > PREEMPT_COUNT. > > See how __irq_enter() -> preempt_count_add(HARDIRQ_OFFSET) -> > __preempt_count_add() _always_ just works. Okay thinking about this further, I think I got your point. That basically means that the in_atomic() check makes sense for irqs. But in my opinion, it does not help do replace preempt_disable() pagefault_disable() by preempt_disable() (as discussed because of the PREEMPT_COUNT stuff) So I agree that we should better add it to not mess with hard/soft irq. > > Its only things like preempt_disable() / preempt_enable() that get > munged depending on PREEMPT_COUNT/PREEMPT. > But anyhow, opinions seem to differ how to best handle that whole stuff. I think a separate counter just makes sense, as we are dealing with two different concepts and we don't want to lose the preempt_disable =^ NOP for !CONFIG_PREEMPT. I also think that pagefault_disable() rt = copy_from_user() pagefault_enable() is a valid use case. So any suggestions how to continue? Thanks! David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
> > * David Hildenbrand wrote: > > > > On Thu, May 07, 2015 at 12:50:53PM +0200, David Hildenbrand wrote: > > > > Just to make sure we have a common understanding (as written in my cover > > > > letter): > > > > > > > > Your suggestion won't work with !CONFIG_PREEMPT > > > > (!CONFIG_PREEMPT_COUNT). If > > > > there is no preempt counter, in_atomic() won't work. > > > > > > But there is, we _always_ have a preempt_count, and irq_enter() et al. > > > _always_ increment the relevant bits. > > > > > > The thread_info::preempt_count field it never under PREEMPT_COUNT > > > include/asm-generic/preempt.h provides stuff regardless of > > > PREEMPT_COUNT. > > > > > > See how __irq_enter() -> preempt_count_add(HARDIRQ_OFFSET) -> > > > __preempt_count_add() _always_ just works. > > > > > > Its only things like preempt_disable() / preempt_enable() that get > > > munged depending on PREEMPT_COUNT/PREEMPT. > > > > > > > Sorry for the confusion. Sure, there is always the count. > > > > My point is that preempt_disable() won't result in an in_atomic() == true > > with !PREEMPT_COUNT, so I don't see any point in adding in to the pagefault > > handlers. It is not reliable. > > That's why we have the preempt_count_inc()/dec() methods that are > always available. > > So where's the problem? My point: Getting rid of PREEMPT_COUNT (and therefore always doing preempt_count_inc()/dec()) will make preempt_disable() __never__ be a NOP. So with !CONFIG_PREEMPT we will do preemption stuff that is simply not needed. Two concepts that share one mechanism. I think this is broken. David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
* David Hildenbrand wrote: > > On Thu, May 07, 2015 at 12:50:53PM +0200, David Hildenbrand wrote: > > > Just to make sure we have a common understanding (as written in my cover > > > letter): > > > > > > Your suggestion won't work with !CONFIG_PREEMPT (!CONFIG_PREEMPT_COUNT). > > > If > > > there is no preempt counter, in_atomic() won't work. > > > > But there is, we _always_ have a preempt_count, and irq_enter() et al. > > _always_ increment the relevant bits. > > > > The thread_info::preempt_count field it never under PREEMPT_COUNT > > include/asm-generic/preempt.h provides stuff regardless of > > PREEMPT_COUNT. > > > > See how __irq_enter() -> preempt_count_add(HARDIRQ_OFFSET) -> > > __preempt_count_add() _always_ just works. > > > > Its only things like preempt_disable() / preempt_enable() that get > > munged depending on PREEMPT_COUNT/PREEMPT. > > > > Sorry for the confusion. Sure, there is always the count. > > My point is that preempt_disable() won't result in an in_atomic() == true > with !PREEMPT_COUNT, so I don't see any point in adding in to the pagefault > handlers. It is not reliable. That's why we have the preempt_count_inc()/dec() methods that are always available. So where's the problem? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
> On Thu, May 07, 2015 at 12:50:53PM +0200, David Hildenbrand wrote: > > Just to make sure we have a common understanding (as written in my cover > > letter): > > > > Your suggestion won't work with !CONFIG_PREEMPT (!CONFIG_PREEMPT_COUNT). If > > there is no preempt counter, in_atomic() won't work. > > But there is, we _always_ have a preempt_count, and irq_enter() et al. > _always_ increment the relevant bits. > > The thread_info::preempt_count field it never under PREEMPT_COUNT > include/asm-generic/preempt.h provides stuff regardless of > PREEMPT_COUNT. > > See how __irq_enter() -> preempt_count_add(HARDIRQ_OFFSET) -> > __preempt_count_add() _always_ just works. > > Its only things like preempt_disable() / preempt_enable() that get > munged depending on PREEMPT_COUNT/PREEMPT. > Sorry for the confusion. Sure, there is always the count. My point is that preempt_disable() won't result in an in_atomic() == true with !PREEMPT_COUNT, so I don't see any point in adding in to the pagefault handlers. It is not reliable. David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
On Thu, May 07, 2015 at 12:50:53PM +0200, David Hildenbrand wrote: > Just to make sure we have a common understanding (as written in my cover > letter): > > Your suggestion won't work with !CONFIG_PREEMPT (!CONFIG_PREEMPT_COUNT). If > there is no preempt counter, in_atomic() won't work. But there is, we _always_ have a preempt_count, and irq_enter() et al. _always_ increment the relevant bits. The thread_info::preempt_count field it never under PREEMPT_COUNT include/asm-generic/preempt.h provides stuff regardless of PREEMPT_COUNT. See how __irq_enter() -> preempt_count_add(HARDIRQ_OFFSET) -> __preempt_count_add() _always_ just works. Its only things like preempt_disable() / preempt_enable() that get munged depending on PREEMPT_COUNT/PREEMPT. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
* David Hildenbrand wrote: > > AFAICR we did this to avoid having to do both: > > > > preempt_disable(); > > pagefault_disable(); > > > > in a fair number of places -- just like this patch-set does, this is > > touching two cachelines where one would have been enough. > > > > Also, removing in_atomic() from fault handlers like you did > > significantly changes semantics for interrupts (soft, hard and NMI). > > > > So while I agree with most of these patches, I'm very hesitant on the > > above little detail. > > Just to make sure we have a common understanding (as written in my > cover letter): > > Your suggestion won't work with !CONFIG_PREEMPT > (!CONFIG_PREEMPT_COUNT). If there is no preempt counter, in_atomic() > won't work. So doing a preempt_disable() instead of a > pagefault_disable() is not going to work. (not sure how -RT handles > that - most probably with CONFIG_PREEMPT_COUNT being enabled, due to > atomic debug). > > That's why I dropped that check for a reason. So, what's the point of disabling the preempt counter? Looks like the much simpler (and faster) solution would be to eliminate CONFIG_PREEMPT_COUNT (i.e. make it always available), and use it for pagefault-disable. > This patchset is about decoupling both concept. (not ending up with > to mechanisms doing almost the same) So that's really backwards: just because we might not have a handy counter we introduce _another one_, and duplicate checks for it ;-) Why not keep a single counter, if indeed what we care about most in the pagefault_disable() case is atomicity? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
> On Wed, May 06, 2015 at 07:50:25PM +0200, David Hildenbrand wrote:
> > +/*
> > + * Is the pagefault handler disabled? If so, user access methods will not
> > sleep.
> > + */
> > +#define pagefault_disabled() (current->pagefault_disabled != 0)
>
> So -RT has:
>
> static inline bool pagefault_disabled(void)
> {
> return current->pagefault_disabled || in_atomic();
> }
>
> AFAICR we did this to avoid having to do both:
>
> preempt_disable();
> pagefault_disable();
>
> in a fair number of places -- just like this patch-set does, this is
> touching two cachelines where one would have been enough.
>
> Also, removing in_atomic() from fault handlers like you did
> significantly changes semantics for interrupts (soft, hard and NMI).
>
> So while I agree with most of these patches, I'm very hesitant on the
> above little detail.
>
Just to make sure we have a common understanding (as written in my cover
letter):
Your suggestion won't work with !CONFIG_PREEMPT (!CONFIG_PREEMPT_COUNT). If
there is no preempt counter, in_atomic() won't work. So doing a
preempt_disable() instead of a pagefault_disable() is not going to work.
(not sure how -RT handles that - most probably with CONFIG_PREEMPT_COUNT being
enabled, due to atomic debug).
That's why I dropped that check for a reason.
So in my opinion, in_atomic() should never be used in any fault handler - it
has nothing to do with disabled pagefaults. It doesn't give us anything more
besides some false security for atomic environments.
This patchset is about decoupling both concept. (not ending up with to
mechanisms doing almost the same)
That's also what Thomas Gleixner suggested
https://lkml.org/lkml/2014/11/27/820 .
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 01/15] uaccess: count pagefault_disable() levels in pagefault_disabled
On Wed, May 06, 2015 at 07:50:25PM +0200, David Hildenbrand wrote:
> +/*
> + * Is the pagefault handler disabled? If so, user access methods will not
> sleep.
> + */
> +#define pagefault_disabled() (current->pagefault_disabled != 0)
So -RT has:
static inline bool pagefault_disabled(void)
{
return current->pagefault_disabled || in_atomic();
}
AFAICR we did this to avoid having to do both:
preempt_disable();
pagefault_disable();
in a fair number of places -- just like this patch-set does, this is
touching two cachelines where one would have been enough.
Also, removing in_atomic() from fault handlers like you did
significantly changes semantics for interrupts (soft, hard and NMI).
So while I agree with most of these patches, I'm very hesitant on the
above little detail.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
