Re: [Xen-devel] [PATCH v5 2/2] x86: add accessors for scratch cpu mask

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.03.2020 16:51, Roger Pau Monné wrote:
> On Wed, Mar 11, 2020 at 04:37:50PM +0100, Jan Beulich wrote:
>> On 11.03.2020 16:34, Roger Pau Monné wrote:
>>> On Fri, Feb 28, 2020 at 01:42:58PM +0100, Jan Beulich wrote:
>>>> On 28.02.2020 13:07, Roger Pau Monne wrote:
>>>>> Current usage of the per-CPU scratch cpumask is dangerous since
>>>>> there's no way to figure out if the mask is already being used except
>>>>> for manual code inspection of all the callers and possible call paths.
>>>>>
>>>>> This is unsafe and not reliable, so introduce a minimal get/put
>>>>> infrastructure to prevent nested usage of the scratch mask and usage
>>>>> in interrupt context.
>>>>>
>>>>> Move the declaration of scratch_cpumask to smp.c in order to place the
>>>>> declaration and the accessors as close as possible.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> Ping? This seems to have the required RB, but hasn't been committed.
>>
>> While as per the R-b this technically is fine, I continue to be
>> uncertain whether we actually want to go this far.
> 
> If this had been in place 5500d265a2a8fa6 ('x86/smp: use APIC ALLBUT
> destination shorthand when possible') wouldn't have introduced a
> bogus usage of the scratch per cpu mask, as the check would have
> triggered.
> 
> After finding that one of my commits introduced a bug I usually do the
> exercise of trying to figure out which checks or safeguards would have
> prevented it, and hence came up with this patch.
> 
> I would also like to note that this adds 0 overhead to non-debug
> builds.
> 
>> Andrew, as
>> per a discussion we had when I was pondering whether to commit
>> this, also looks to have similar concerns (which iirc he said he
>> had voiced on irc).
> 
> Is the concern only related to the fact that you have to use the
> get/put accessors and thus more lines of code are added, or is there
> something else?

Afaic - largely this, along with it making it more likely that
error paths will be non-trivial (and hence possibly get converted
to use goto-s). I can't speak for Andrew, of course.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel