lkml.org 
[lkml]   [2019]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch V2 06/16] x86/io: Speedup schedule out of I/O bitmap user
On Tue, 12 Nov 2019, Andy Lutomirski wrote:
> On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > @@ -50,6 +48,11 @@ long ksys_ioperm(unsigned long from, uns
> > * limit correct.
> > */
> > preempt_disable();
> > + t->io_bitmap_ptr = bitmap;
> > + set_thread_flag(TIF_IO_BITMAP);
> > + /* Make the bitmap base in the TSS valid */
> > + tss = this_cpu_ptr(&cpu_tss_rw);
> > + tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_VALID;
> > refresh_tss_limit();
> > preempt_enable();
> > }
>
> It's not shown in the diff, but the very next line of code turns
> preemption back off. This means that we might schedule right here
> with TIF_IO_BITMAP set, the base set to VALID, but the wrong data in
> the bitmap. I *think* this will actually end up being okay, but it
> certainly makes understanding the code harder. Can you adjust the
> code so that preemption stays off?
>
> More importantly, the code below this modifies the TSS copy in place
> instead of writing a whole new copy. But now that you've added your
> optimization, the TSS copy might be *someone else's* IO bitmap. So I
> think you might end up with more io ports allowed than you intended.
> For example:
>
> Task A uses ioperm() to enable all ports.
> Switch to task B. Now the TSS base is INVALID but all bitmap bits are still 0.
> Task B calls ioperm().
>
> The code will set the base to VALID and will correctly set up the
> thread's copy of the bitmap, but I think the copy will only update the
> bits 0 through whatever ioperm() touched and not the bits above that
> in the TSS.

Yeah, you are right. Did not think about that. Will fix that up.

> I would believe that this is fixed later in your patch set. If so,
> perhaps you should just memcpy() the whole thing without trying to
> optimize in this patch and then let the changes later re-optimize it
> as appropriate. IOW change memcpy(tss->io_bitmap, t->io_bitmap_ptr,
> bytes_updated); to memcpy(..., BYTES_PER_LONG * IO_BITMAP_LONGS) or
> similar.

Right.

Thanks for spotting that!

tglx

\
 
 \ /
  Last update: 2019-11-12 18:09    [W:0.062 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site