xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 0/8] assorted replacement of x[mz]alloc_bytes()
Date: Wed, 21 Apr 2021 17:32:07 +0200	[thread overview]
Message-ID: <6afd9e45-ad71-5b39-6bec-c8d37ef56157@suse.com> (raw)
In-Reply-To: <40d373eb-d7c2-27f9-df20-a7fecbe519a5@citrix.com>

On 21.04.2021 17:23, Andrew Cooper wrote:
> On 21/04/2021 15:54, Jan Beulich wrote:
>> In the long run I think we want to do away with these type-unsafe
>> interfaces, the more that they also request (typically) excess
>> alignment. This series of entirely independent patches is
>> eliminating the instances where it's relatively clear that they're
>> not just "blob" allocations.
>>
>> v2 only has commit messages extended.
>>
>> 1: x86/MCE: avoid effectively open-coding xmalloc_array()
>> 2: x86/HVM: avoid effectively open-coding xmalloc_array()
>> 3: x86/oprofile: avoid effectively open-coding xmalloc_array()
>> 4: x86/IRQ: avoid over-alignment in alloc_pirq_struct()
>> 5: EFI/runtime: avoid effectively open-coding xmalloc_array()
>> 6: kexec: avoid effectively open-coding xzalloc_flex_struct()
>> 7: video/lfb: avoid effectively open-coding xzalloc_array()
>> 8: Arm/optee: don't open-code xzalloc_flex_struct()
> 
> I'm tempted to nack this, but for now will go with a firm -2 to the
> whole series.
> 
> It is unreasonable, at an API level, for *lloc_bytes(...) to not be
> interchangeable *alloc_array(char, ...), and the former is the clearer
> way of writing code.
> 
> The alignment details are internal properties, dubious at best, and
> totally unreasonable for maintainer to know or care about as far as the
> API is concerned.  There is also no type safety to be gained by making
> the transformation.
> 
> If you want to improve the alignment, fix the allocator and the
> behind-the-scenes semantics.  Don't make every callsite more complicated
> to follow, and definitely don't introduce perf problems from cacheline
> sharing in the name of typesafey.

So you firmly think x*alloc_bytes() is a good interface to have and to
keep? As said above, I'm of the clear opinion that we should get rid
of it (with what you say in the first sentence of the second from last
paragraph being one of the reasons). The fact that it may be a
shorthand when allocating char[] is really, really minor (and violates
consistency of the code base as a whole).

Also, just to make this explicit, patch 4 really is somewhat different
from the others, and hence would better not fall into a general "I don't
like this and won't look at it" bucket. Granted I'm not sure you'll
flame me for other reasons there ...

Jan


  reply	other threads:[~2021-04-21 15:32 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-21 14:54 Jan Beulich
2021-04-21 14:56 ` [PATCH v2 1/8] x86/MCE: avoid effectively open-coding xmalloc_array() Jan Beulich
2021-04-21 14:56 ` [PATCH v2 2/8] x86/HVM: " Jan Beulich
2021-04-21 14:57 ` [PATCH v2 3/8] x86/oprofile: " Jan Beulich
2021-04-21 14:57 ` [PATCH v2 4/8] x86/IRQ: avoid over-alignment in alloc_pirq_struct() Jan Beulich
2021-04-21 14:58 ` [PATCH v2 5/8] EFI/runtime: avoid effectively open-coding x{m,z}alloc_array() Jan Beulich
2021-04-21 14:58 ` [PATCH v2 6/8] kexec: avoid effectively open-coding xzalloc_flex_struct() Jan Beulich
2021-04-21 14:59 ` [PATCH v2 7/8] video/lfb: avoid effectively open-coding xzalloc_array() Jan Beulich
2021-04-21 14:59 ` [PATCH v2 8/8] Arm/optee: don't open-code xzalloc_flex_struct() Jan Beulich
2021-05-03 13:53   ` Ping: " Jan Beulich
2021-05-04 23:49     ` Volodymyr Babchuk
2021-04-21 15:23 ` [PATCH v2 0/8] assorted replacement of x[mz]alloc_bytes() Andrew Cooper
2021-04-21 15:32   ` Jan Beulich [this message]
2021-04-23  9:44   ` Jan Beulich

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=6afd9e45-ad71-5b39-6bec-c8d37ef56157@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=julien@xen.org \
    --cc=roger.pau@citrix.com \
    --cc=sstabellini@kernel.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH v2 0/8] assorted replacement of x[mz]alloc_bytes()' \
    /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

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).