lkml.org 
[lkml]   [2015]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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



    \
     
     \ /
      Last update: 2015-05-07 14:21    [W:5.353 / U:0.584 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site