Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects

[Andrew Cooper <amc96@xxxxxxxx>]

On 30/09/2021 09:07, Dario Faggioli wrote:
> On Mon, 2021-09-27 at 09:51 +0200, Jan Beulich wrote:
>> On 24.09.2021 16:51, Dario Faggioli wrote:
>>> On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
>>>
>>>> There is one buggy race record, TRC_RTDS_BUDGET_BURN.  As it must
>>>> remain
>>>> __packed (as cur_budget is misaligned), change bool has_extratime
>>>> to
>>>> uint32_t
>>>> to compensate.
>>>>
>>> Mmm... maybe my understanding of data alignment inside structs is a
>>> bit
>>> lacking, but what the actual issue here, and what would we need to
>>> do
>>> to fix it (where, by fix, I mean us being able to get rid of the
>>> `__packed`)?
>>>
>>> If rearranging fields is not enough, we can think about making
>>> priority_level and has_extratime smaller, or even combining them in
>>> just one field and decode the information in xentrace.
>> I guess Andrew has tried to avoid re-arranging field order so that
>> the consumer side doesn't need to also change.
>>
> Right, but is it really worth it, in this case?
>
> Aren't we (very very likely) in control, by having them here in the
> tree, of all the consumers? And is is this a stable ABI?
>
> Also, both xentrace_format and xenalyze are being modified in this
> series anyway...
>
> Maybe there's still something I'm missing, but if we've getting rid of
> those ugly bitfields and __packed attributes, it seems to me that doing
> it completely --i.e., including in this case-- is worth the small
> change in the tools.

This patch needs backporting to stable trees.  We shouldn't be changing
the ABI, even if its stability is unclear.

Which means patch 2 needs altering to avoid ABI changes.  /sigh

~Andre