Messages in this thread | | | Date | Tue, 12 Nov 2019 18:08:27 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [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
| |