Re: [Xen-devel] [PATCH v3 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Jan Beulich <JBeulich@xxxxxxxx>]

On 30.07.2019 18:36, Andrew Cooper wrote:
> On 30/07/2019 10:49, Jan Beulich wrote:
>> On 30.07.2019 11:35, Andrew Cooper wrote:
>>> On 30/07/2019 09:42, Jan Beulich wrote:
>>>> On 29.07.2019 19:38, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>> @@ -293,14 +293,17 @@ SECTIONS
>>>>>           __bss_start = .;
>>>>>           *(.bss.stack_aligned)
>>>>>           *(.bss.page_aligned*)
>>>>> -       *(.bss)
>>>>> -       . = ALIGN(SMP_CACHE_BYTES);
>>>>> +       . = ALIGN(PAGE_SIZE);
>>>>>           __per_cpu_start = .;
>>>>> +       *(.bss.percpu.page_aligned)
>>>>> +       . = ALIGN(PAGE_SIZE);
>>>> But this goes too far: What we want is for the TSS to occupy a full
>>>> page, not for whatever random other page-aligned object ends up
>>>> last here (should any every appear).
>>> Come again?  This is ridiculous.
>>>
>>> Objects in a section following foo.page_aligned should never end up in
>>> the tail of the final page of foo.page_aligned, because then they are in
>>> the wrong section.
>> How do you derive "wrong section"? Sections have an alignment and a
>> size. The latter doesn't need to be a multiple of the former. The
>> section ends wherever its size says it ends. Using this property is
>> actually desirable in other cases, to limit waste of space.
> 
> The principle of least surprise, for a section with page_aligned in its
> name, is that the section starts and ends on a page boundary.  This is
> what people expect when they see a name like that.

Hmm, when I see "aligned" I think "aligned", and nothing else. What's
more odd - your response here is inconsistent with your earlier one in
https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02084.html
There you did appear to agree that .bss.page_aligned may end with an
object the size of which is not a multiple of PAGE_SIZE. Furthermore
you even make provisions for this to happen right in the patch
description here.

(As an aside, you also don't seem to have any problems with .text.kexec
being page aligned but not a multiple of PAGE_SIZE in size. Granted this
section doesn't carry "page_aligned" in its name.)

>>> A short TSS is a pain to deal with, but even you said you didn't like
>>> the xen_tss idea when you suggested it.
>>>
>>> The name of the section is very deliberately not TSS specific, because
>>> there is plenty of other cleanup which will end up with objects in this
>>> section.
>> Well, if they're all strictly a multiple of PAGE_SIZE, then writing
>> down a respective requirement might be acceptable. But even then Inot 
>> submitted by me.
>> wouldn't be overly happy going that route.
> 
> This reply, like most others in this thread, is actively unhelpful, and
> I give up.
> 
> I can't read your mind.  Neither can anyone else, and noone has the time
> to divine what you want.
> 
> If you don't come up with something more helpful than "I don't like it
> this way", then I'm going to commit this series in its current form, and
> you can adjusting it to your own taste, in your own time.
> 
> This goes for other series as well, including ones submitted by others.
> It is absolutely critical that review feedback identifies, in a clear
> manner, how you expect the issue to be resolved.
> 
> For any non-trivial piece of feedback, if it can't be phrased in the
> form "I would this patch ok if you alter $X to $Y", then it is probably
> wants rethinking.  Whatever the feedback actually is, that gives a
> concrete $X which is the perceived problem, and a concrete $Y which is
> either a clear discussion point, or a clear direction to work towards.

I have to admit that I'm rather surprised to get _this_ as a reply here,
when we've already sketched out the correct alternative. Despite me not
particularly liking it, I don't see anything wrong with

union _aligned(PAGE_SIZE) tss_union {
     struct __packed tss_struct {
         uint32_t :32;
         uint64_t rsp0, rsp1, rsp2;
         uint64_t :64;
         /*
          * Interrupt Stack Table is 1-based so tss->ist[0] corresponds to an 
IST
          * value of 1 in an Interrupt Descriptor.
          */
         uint64_t ist[7];
         uint64_t :64;
         uint16_t :16, bitmap;
     };
     char pad[PAGE_SIZE];
};

And since it's a technically correct solution (providing both a type
correctly describing the hardware interface and one correctly describing
our own needs) with no better alternative either of us can see, I think
this (or whatever variation of it) is the way to go.

As to the rest of your rant: I disagree that a reviewer has to always
suggest how things are to be done; that's desirable where possible,
but simply pointing out something is wrong will have to do in certain
cases. In the case here, which is a good example imo, the point of my
response is that from simply looking at the resulting code it is
unclear why _either_ of the two ALIGN(PAGE_SIZE) have been inserted.
This carries the risk of later someone like me coming and deleting
everything that's there without apparent reason (see e.g. 01fe4da624,
albeit istr having done something more extensive at some other time,
but I can't seem to be able to spot it). If anything it should be you
to explain why they need to be there. And while (in a way) you do so
(in the description, the same passage as referenced above), the reason
isn't really as simple as "such that the result is safe even with
objects shorter than a page in length". Instead the reasons actually
differ for both ALIGN()s: In the first case we simply want to avoid
__per_cpu_start living needlessly early. In the latter one you want to
compensate for something that should be done elsewhere (see above).

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