[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 5/6] xen/tasklet: Return -ERESTART from continue_hypercall_on_cpu()

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 10 Dec 2019 17:55:42 +0000
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=andrew.cooper3@xxxxxxxxxx; spf=Pass smtp.mailfrom=Andrew.Cooper3@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx
  • Autocrypt: addr=andrew.cooper3@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFLhNn8BEADVhE+Hb8i0GV6mihnnr/uiQQdPF8kUoFzCOPXkf7jQ5sLYeJa0cQi6Penp VtiFYznTairnVsN5J+ujSTIb+OlMSJUWV4opS7WVNnxHbFTPYZVQ3erv7NKc2iVizCRZ2Kxn srM1oPXWRic8BIAdYOKOloF2300SL/bIpeD+x7h3w9B/qez7nOin5NzkxgFoaUeIal12pXSR Q354FKFoy6Vh96gc4VRqte3jw8mPuJQpfws+Pb+swvSf/i1q1+1I4jsRQQh2m6OTADHIqg2E ofTYAEh7R5HfPx0EXoEDMdRjOeKn8+vvkAwhviWXTHlG3R1QkbE5M/oywnZ83udJmi+lxjJ5 YhQ5IzomvJ16H0Bq+TLyVLO/VRksp1VR9HxCzItLNCS8PdpYYz5TC204ViycobYU65WMpzWe LFAGn8jSS25XIpqv0Y9k87dLbctKKA14Ifw2kq5OIVu2FuX+3i446JOa2vpCI9GcjCzi3oHV e00bzYiHMIl0FICrNJU0Kjho8pdo0m2uxkn6SYEpogAy9pnatUlO+erL4LqFUO7GXSdBRbw5 gNt25XTLdSFuZtMxkY3tq8MFss5QnjhehCVPEpE6y9ZjI4XB8ad1G4oBHVGK5LMsvg22PfMJ ISWFSHoF/B5+lHkCKWkFxZ0gZn33ju5n6/FOdEx4B8cMJt+cWwARAQABtClBbmRyZXcgQ29v cGVyIDxhbmRyZXcuY29vcGVyM0BjaXRyaXguY29tPokCOgQTAQgAJAIbAwULCQgHAwUVCgkI CwUWAgMBAAIeAQIXgAUCWKD95wIZAQAKCRBlw/kGpdefoHbdD/9AIoR3k6fKl+RFiFpyAhvO 59ttDFI7nIAnlYngev2XUR3acFElJATHSDO0ju+hqWqAb8kVijXLops0gOfqt3VPZq9cuHlh IMDquatGLzAadfFx2eQYIYT+FYuMoPZy/aTUazmJIDVxP7L383grjIkn+7tAv+qeDfE+txL4 SAm1UHNvmdfgL2/lcmL3xRh7sub3nJilM93RWX1Pe5LBSDXO45uzCGEdst6uSlzYR/MEr+5Z JQQ32JV64zwvf/aKaagSQSQMYNX9JFgfZ3TKWC1KJQbX5ssoX/5hNLqxMcZV3TN7kU8I3kjK mPec9+1nECOjjJSO/h4P0sBZyIUGfguwzhEeGf4sMCuSEM4xjCnwiBwftR17sr0spYcOpqET ZGcAmyYcNjy6CYadNCnfR40vhhWuCfNCBzWnUW0lFoo12wb0YnzoOLjvfD6OL3JjIUJNOmJy RCsJ5IA/Iz33RhSVRmROu+TztwuThClw63g7+hoyewv7BemKyuU6FTVhjjW+XUWmS/FzknSi dAG+insr0746cTPpSkGl3KAXeWDGJzve7/SBBfyznWCMGaf8E2P1oOdIZRxHgWj0zNr1+ooF /PzgLPiCI4OMUttTlEKChgbUTQ+5o0P080JojqfXwbPAyumbaYcQNiH1/xYbJdOFSiBv9rpt TQTBLzDKXok86LkCDQRS4TZ/ARAAkgqudHsp+hd82UVkvgnlqZjzz2vyrYfz7bkPtXaGb9H4 Rfo7mQsEQavEBdWWjbga6eMnDqtu+FC+qeTGYebToxEyp2lKDSoAsvt8w82tIlP/EbmRbDVn 7bhjBlfRcFjVYw8uVDPptT0TV47vpoCVkTwcyb6OltJrvg/QzV9f07DJswuda1JH3/qvYu0p vjPnYvCq4NsqY2XSdAJ02HrdYPFtNyPEntu1n1KK+gJrstjtw7KsZ4ygXYrsm/oCBiVW/OgU g/XIlGErkrxe4vQvJyVwg6YH653YTX5hLLUEL1NS4TCo47RP+wi6y+TnuAL36UtK/uFyEuPy wwrDVcC4cIFhYSfsO0BumEI65yu7a8aHbGfq2lW251UcoU48Z27ZUUZd2Dr6O/n8poQHbaTd 6bJJSjzGGHZVbRP9UQ3lkmkmc0+XCHmj5WhwNNYjgbbmML7y0fsJT5RgvefAIFfHBg7fTY/i kBEimoUsTEQz+N4hbKwo1hULfVxDJStE4sbPhjbsPCrlXf6W9CxSyQ0qmZ2bXsLQYRj2xqd1 bpA+1o1j2N4/au1R/uSiUFjewJdT/LX1EklKDcQwpk06Af/N7VZtSfEJeRV04unbsKVXWZAk uAJyDDKN99ziC0Wz5kcPyVD1HNf8bgaqGDzrv3TfYjwqayRFcMf7xJaL9xXedMcAEQEAAYkC HwQYAQgACQUCUuE2fwIbDAAKCRBlw/kGpdefoG4XEACD1Qf/er8EA7g23HMxYWd3FXHThrVQ HgiGdk5Yh632vjOm9L4sd/GCEACVQKjsu98e8o3ysitFlznEns5EAAXEbITrgKWXDDUWGYxd pnjj2u+GkVdsOAGk0kxczX6s+VRBhpbBI2PWnOsRJgU2n10PZ3mZD4Xu9kU2IXYmuW+e5KCA vTArRUdCrAtIa1k01sPipPPw6dfxx2e5asy21YOytzxuWFfJTGnVxZZSCyLUO83sh6OZhJkk b9rxL9wPmpN/t2IPaEKoAc0FTQZS36wAMOXkBh24PQ9gaLJvfPKpNzGD8XWR5HHF0NLIJhgg 4ZlEXQ2fVp3XrtocHqhu4UZR4koCijgB8sB7Tb0GCpwK+C4UePdFLfhKyRdSXuvY3AHJd4CP 4JzW0Bzq/WXY3XMOzUTYApGQpnUpdOmuQSfpV9MQO+/jo7r6yPbxT7CwRS5dcQPzUiuHLK9i nvjREdh84qycnx0/6dDroYhp0DFv4udxuAvt1h4wGwTPRQZerSm4xaYegEFusyhbZrI0U9tJ B8WrhBLXDiYlyJT6zOV2yZFuW47VrLsjYnHwn27hmxTC/7tvG3euCklmkn9Sl9IAKFu29RSo d5bD8kMSCYsTqtTfT6W4A3qHGvIDta3ptLYpIAOD2sY3GYq2nf3Bbzx81wZK14JdDDHUX2Rs 6+ahAA==
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Tue, 10 Dec 2019 17:55:55 +0000
  • Ironport-sdr: 4TU2ShugrOm+XPUwhNcXpA0jdXCqPw56AeODuX9C+RQX8dARevcMC1n83FESTqUIwLAK8mktZT 171DVxipaXFyEVvE+42HLOfUAJhKbON/1prrJQDDG7IxnZ7pT/jmXrN7HLinFbC/DoYLR0Dxkq aunHfoG6hZnTIz3woLdK9Na8qLZnrGyHZgx0/iIIsdLwfy48iO3tI0xPvKfzylBNWowA54fAYJ GCcwu0vWwLNn9sgVKzeqFFhTCNUz+8fQdX3UXrxKD018eIAO+ywXV8ii5X4Mg8Ivv0S8L245jc 6xw=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 10/12/2019 08:55, Jan Beulich wrote:
> On 09.12.2019 18:49, Andrew Cooper wrote:
>> On 09/12/2019 16:52, Jan Beulich wrote:
>>> On 05.12.2019 23:30, Andrew Cooper wrote:
>>>> Some hypercalls tasklets want to create a continuation, rather than fail 
>>>> the
>>>> hypercall with a hard error.  By the time the tasklet is executing, it is 
>>>> too
>>>> late to create the continuation, and even continue_hypercall_on_cpu() 
>>>> doesn't
>>>> have enough state to do it correctly.
>>> I think it would be quite nice if you made clear what piece of state
>>> it is actually missing. To be honest, I don't recall anymore.
>> How to correctly mutate the registers and/or memory (which is specific
>> to the hypercall subop in some cases).
> Well, in-memory arguments can be accessed as long as the mapping is
> the right one (which it typically wouldn't be inside a tasklet). Do
> existing continue_hypercall_on_cpu() users need this? Looking over
> patch 4 again, I didn't think so. (Which isn't to say that removing
> the latent issue is not a good thing.)
> In-register values can be changed as long as the respective exit
> path will suitably pick up the value, which I thought was always
> the case.
> Hence I'm afraid your single reply sentence didn't really clarify
> matters. I'm sorry if this is just because of me being dense.

How, physically, would you arrange for continue_hypercall_on_cpu() to
make the requisite state adjustments?

Yes - registers and memory can be accessed, but only the hypercall
(sub?)op handler knows how to mutate them appropriately.

You'd have to copy the mutation logic into continue_hypercall_on_cpu(),
and pass in op/subops and a union of all pointers, *and* whatever
intermediate state the subop handler needs.

Or you can return -ERESTART and let the caller DTRT with the state it
has in context, as it would in other cases requiring a continuation.

>>>> There is one RFC point.  The statement in the header file of "If this 
>>>> function
>>>> returns 0 then the function is guaranteed to run at some point in the 
>>>> future."
>>>> was never true.  In the case of a CPU miss, the hypercall would be blindly
>>>> failed with -EINVAL.
>>> "Was never true" sounds like "completely broken". Afaict it was true
>>> in all cases except the purely hypothetical one of the tasklet ending
>>> up executing on the wrong CPU.
>> There is nothing hypothetical about it.  It really will go wrong when a
>> CPU gets offlined.
> Accepted, but it's still not like "completely broken".

I didn't mean it like that.  I mean "it has never had the property it
claimed", which is distinct from "the claim used to be true, but was
then accidentally regressed".

> I would even
> suppose the case wasn't considered when CPU offlining support was
> introduced, not when continue_hypercall_on_cpu() came into existence
> (which presumably is when the comment was written).
> Anyway - yes, I agree this is a fair solution to the issue at hand.
>>>> The current behaviour with this patch is to not cancel the continuation, 
>>>> which
>>>> I think is less bad, but still not great.  Thoughts?
>>> Well, that's a guest live lock then aiui.
>> It simply continues again.  It will livelock only if the hypercall picks
>> a bad cpu all the time.
> Oh, I see I was mislead by continue_hypercall_tasklet_handler() not
> updating info->cpu, not paying attention to it actually freeing info.
> Plus a crucial aspect looks to be that there are no "chained" uses of
> continue_hypercall_on_cpu() anymore (the microcode loading one being
> gone now) - afaict any such wouldn't guarantee forward progress with
> this new model (without recording somewhere which CPUs had been dealt
> with already).

I'd forgotten that we had that, but I can't say I'm sad to see the back
of it.  I recall at the time saying that it wasn't a clever move.

For now, I suggest that we ignore that case.  If an when a real usecase
appears, we can consider making adjustments.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.