From: Andy Lutomirski <luto@kernel.org>
To: Ingo Molnar <mingo@kernel.org>,
Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
LKML <linux-kernel@vger.kernel.org>,
the arch/x86 maintainers <x86@kernel.org>,
Stephen Hemminger <stephen@networkplumber.org>,
Willy Tarreau <w@1wt.eu>, Juergen Gross <jgross@suse.com>,
Sean Christopherson <sean.j.christopherson@intel.com>,
"H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further
Date: Sun, 10 Nov 2019 09:17:39 -0800 [thread overview]
Message-ID: <54ae3c3a-f49a-f5cc-d174-de6e64afe899@kernel.org> (raw)
In-Reply-To: <20191107082541.GF30739@gmail.com>
On 11/7/19 12:25 AM, Ingo Molnar wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Wed, Nov 6, 2019 at 12:57 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>>>
>>> Calculate both the position of the first zero bit and the last zero bit to
>>> limit the range which needs to be copied. This does not solve the problem
>>> when the previous tasked had only byte 0 cleared and the next one has only
>>> byte 65535 cleared, but trying to solve that would be too complex and
>>> heavyweight for the context switch path. As the ioperm() usage is very rare
>>> the case which is optimized is the single task/process which uses ioperm().
>>
>> Hmm.
>>
>> I may read this patch wrong, but from what I can tell, if we really
>> have just one process with an io bitmap, we're doing unnecessary
>> copies.
>>
>> If we really have just one process that has an iobitmap, I think we
>> could just keep the bitmap of that process entirely unchanged. Then,
>> when we switch away from it, we set the io_bitmap_base to an invalid
>> base outside the TSS segment, and when we switch back, we set it back
>> to the valid one. No actual bitmap copies at all.
>>
>> So I think that rather than the "begin/end offset" games, we should
>> perhaps have a "what was the last process that used the IO bitmap for
>> this TSS" pointer (and, I think, some sequence counter, so that when
>> the process updates its bitmap, it invalidates that case)?
>>
>> Of course, you can do *nboth*, but if we really think that the common
>> case is "one special process", then I think the begin/end offset is
>> useless, but a "last bitmap process" would be very useful.
>>
>> Am I missing something?
>
> In fact on SMP systems this would result in a very nice optimization:
> pretty quickly *all* TSS's would be populated with that single task's
> bitmap, and it would persist even across migrations from CPU to CPU.
>
> I'd love to get rid of the offset caching and bit scanning games as well
> - it doesn't really help in a number of common scenarios and it
> complicates this non-trivial piece of code a *LOT* - and we probably
> don't really have the natural testing density of this code anymore to
> find any regressions quickly.
I think we should not over-optimize this. I am all for penalizing
ioperm() and iopl() users as much as is convenient for us. There is
simply no legitimate use case. Sorry, DPDK, but "virtio-legacy sucks,
let's optimize the crap out of something that is slow anyway and use
iopl()" is not a good excuse. Just use the %*!7 syscall to write to
/sys/.../resource0 and suck up the probably negligible performance hit.
And tell your customers to upgrade their hypervisors. And quite
kvetching about performance of the control place on an old
software-emulated NIC while you're at it.
For the TLB case, it's worth tracking who last used which ASID and
whether it's still up to date, since *everyone* uses the MMU. For
ioperm, I don't really believe this is worth it.
>
> So intuitively I'd suggest we gravitate towards the simplest
> implementation, with a good caching optimization for the single-task
> case.
I agree with the first bit, but caching on an SMP system is necessarily
subtle. Some kind of invalidation is needed.
>
> I.e. the model I'm suggesting is that if a task uses ioperm() or iopl()
> then it should have a bitmap from that point on until exit(), even if
> it's all zeroes or all ones. Most applications that are using those
> primitives really need it all the time and are using just a few ioports,
> so all the tracking doesn't help much anyway.
>
> On a related note, another simplification would be that in principle we
> could also use just a single bitmap and emulate iopl() as an ioperm(all)
> or ioperm(none) calls. Yeah, it's not fully ABI compatible for mixed
> ioperm()/iopl() uses, but is that ABI actually being relied on in
> practice?
>
Let's please keep the ABI. Or rather, let's attempt to eventually
remove the ABI, but let's not change it in the mean time please.
next prev parent reply other threads:[~2019-11-10 17:17 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-06 19:34 [patch 0/9] x86/iopl: Prevent user space from using CLI/STI with iopl(3) Thomas Gleixner
2019-11-06 19:35 ` [patch 1/9] x86/ptrace: Prevent truncation of bitmap size Thomas Gleixner
2019-11-07 7:31 ` Ingo Molnar
2019-11-06 19:35 ` [patch 2/9] x86/process: Unify copy_thread_tls() Thomas Gleixner
2019-11-08 22:31 ` Andy Lutomirski
2019-11-08 23:43 ` Thomas Gleixner
2019-11-10 12:36 ` Thomas Gleixner
2019-11-10 16:56 ` Andy Lutomirski
2019-11-11 8:52 ` Peter Zijlstra
2019-11-06 19:35 ` [patch 3/9] x86/cpu: Unify cpu_init() Thomas Gleixner
2019-11-08 22:34 ` Andy Lutomirski
2019-11-06 19:35 ` [patch 4/9] x86/io: Speedup schedule out of I/O bitmap user Thomas Gleixner
2019-11-07 9:12 ` Peter Zijlstra
2019-11-07 14:04 ` Thomas Gleixner
2019-11-07 14:08 ` Thomas Gleixner
2019-11-08 22:41 ` Andy Lutomirski
2019-11-08 23:45 ` Thomas Gleixner
2019-11-09 3:32 ` Andy Lutomirski
2019-11-10 12:43 ` Thomas Gleixner
2019-11-09 0:24 ` Andy Lutomirski
2019-11-06 19:35 ` [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further Thomas Gleixner
2019-11-07 1:11 ` Linus Torvalds
2019-11-07 7:44 ` Thomas Gleixner
2019-11-07 8:25 ` Ingo Molnar
2019-11-07 9:17 ` Willy Tarreau
2019-11-07 10:00 ` Thomas Gleixner
2019-11-07 10:13 ` Willy Tarreau
2019-11-07 10:19 ` hpa
2019-11-07 10:27 ` Willy Tarreau
2019-11-07 10:50 ` hpa
2019-11-07 12:56 ` Willy Tarreau
2019-11-07 16:45 ` Eric W. Biederman
2019-11-07 16:53 ` Linus Torvalds
2019-11-07 16:57 ` Willy Tarreau
2019-11-10 17:17 ` Andy Lutomirski [this message]
2019-11-07 7:37 ` Ingo Molnar
2019-11-07 7:45 ` Thomas Gleixner
2019-11-07 8:16 ` Ingo Molnar
2019-11-07 18:02 ` Thomas Gleixner
2019-11-07 19:24 ` Brian Gerst
2019-11-07 19:54 ` Linus Torvalds
2019-11-07 21:00 ` Brian Gerst
2019-11-07 21:32 ` Thomas Gleixner
2019-11-07 23:20 ` hpa
2019-11-07 21:44 ` Linus Torvalds
2019-11-08 1:12 ` H. Peter Anvin
2019-11-08 2:12 ` Brian Gerst
2019-11-10 17:21 ` Andy Lutomirski
2019-11-06 19:35 ` [patch 6/9] x86/iopl: Fixup misleading comment Thomas Gleixner
2019-11-06 19:35 ` [patch 7/9] x86/iopl: Restrict iopl() permission scope Thomas Gleixner
2019-11-07 9:09 ` Peter Zijlstra
2019-11-10 17:26 ` Andy Lutomirski
2019-11-10 20:31 ` Thomas Gleixner
2019-11-10 21:05 ` Andy Lutomirski
2019-11-10 21:21 ` Thomas Gleixner
2019-11-06 19:35 ` [patch 8/9] x86/iopl: Remove legacy IOPL option Thomas Gleixner
2019-11-07 6:11 ` Jürgen Groß
2019-11-07 6:26 ` hpa
2019-11-07 16:44 ` Stephen Hemminger
2019-11-07 9:13 ` Peter Zijlstra
2019-11-06 19:35 ` [patch 9/9] selftests/x86/iopl: Verify that CLI/STI result in #GP Thomas Gleixner
2019-11-07 7:28 ` [patch] x86/iopl: Remove unused local variable, update comments in ksys_ioperm() Ingo Molnar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54ae3c3a-f49a-f5cc-d174-de6e64afe899@kernel.org \
--to=luto@kernel.org \
--cc=hpa@zytor.com \
--cc=jgross@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=sean.j.christopherson@intel.com \
--cc=stephen@networkplumber.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=w@1wt.eu \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).