Message-ID: <8af6253d-83aa-403a-a383-8fb5f5ee0d78@redhat.com>
Date: Wed, 11 Sep 2024 18:44:43 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 Richard Narron <richard@...zen.com>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Marcin Wojtas <marcin.s.wojtas@...il.com>,
 Russell King <linux@...linux.org.uk>, "David S . Miller"
 <davem@...emloft.net>, Arnd Bergmann <arnd@...nel.org>,
 Linus Torvalds <torvalds@...uxfoundation.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linux-staging@...ts.linux.dev, linux-mm@...ck.org, stable@...r.kernel.org
Subject: Re: [PATCH hotfix 6.11] minmax: reduce egregious min/max macro
 expansion

Hi,

On 9/11/24 6:37 PM, Lorenzo Stoakes wrote:
> On Wed, Sep 11, 2024 at 06:24:54PM GMT, Hans de Goede wrote:
>> Hi Lorenzo,
>>
>> On 9/11/24 5:34 PM, Lorenzo Stoakes wrote:
>>> Avoid nested min()/max() which results in egregious macro expansion.
>>>
>>> This issue was introduced by commit 867046cc7027 ("minmax: relax check to
>>> allow comparison between unsigned arguments and signed constants") [2].
>>>
>>> Work has been done to address the issue of egregious min()/max() macro
>>> expansion in commit 22f546873149 ("minmax: improve macro expansion and type
>>> checking") and related, however it appears that some issues remain on more
>>> tightly constrained systems.
>>>
>>> Adjust a few known-bad cases of deeply nested macros to avoid doing so to
>>> mitigate this. Porting the patch first proposed in [1] to Linus's tree.
>>>
>>> Running an allmodconfig build using the methodology described in [2] we
>>> observe a 35 MiB reduction in generated code.
>>>
>>> The difference is much more significant prior to recent minmax fixes which
>>> were not backported. As per [1] prior these the reduction is more like 200
>>> MiB.
>>>
>>> This resolves an issue with slackware 15.0 32-bit compilation as reported
>>> by Richard Narron.
>>>
>>> Presumably the min/max fixups would be difficult to backport, this patch
>>> should be easier and fix's Richard's problem in 5.15.
>>>
>>> [0]:https://lore.kernel.org/all/b97faef60ad24922b530241c5d7c933c@AcuMS.aculab.com/
>>> [1]:https://lore.kernel.org/lkml/5882b96e-1287-4390-8174-3316d39038ef@lucifer.local/
>>> [2]:https://lore.kernel.org/linux-mm/36aa2cad-1db1-4abf-8dd2-fb20484aabc3@lucifer.local/
>>>
>>> Reported-by: Richard Narron <richard@...zen.com>
>>> Closes: https://lore.kernel.org/all/4a5321bd-b1f-1832-f0c-cea8694dc5aa@aaazen.com/
>>> Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
>>> Cc: stable@...r.kernel.org
>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>>
>> Thank you for your patch.
>>
>> I must say that I'm not a fan of that this is patching 3 totally
>> unrelated files here in a single patch.
>>
>> This is e.g. going to be a problem if we need to revert one of
>> the changes because of regressions...
>>
>> So I would prefer this to be split into 3 patches.
> 
> Well, I was doing this as a favour to Richard between other work so put
> this together quickly, but you're right this is going to be a pain to
> backport/revert if issues so absolutely - will do.
> 
> Since this is a hotfix I'm going to risk annoying people and shoot out
> a v2 on same day as v1. Sorry in advance.
> 
>>
>> One review comment for the atomisp bits inline / below.
>>
>>> ---
>>>  drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  2 +-
>>>  .../staging/media/atomisp/pci/sh_css_frac.h   | 26 ++++++++++++++-----
>>>  include/linux/skbuff.h                        |  6 ++++-
>>>  3 files changed, 25 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>>> index e809f91c08fb..8b431f90efc3 100644
>>> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>>> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>>> @@ -23,7 +23,7 @@
>>>  /* The PacketOffset field is measured in units of 32 bytes and is 3 bits wide,
>>>   * so the maximum offset is 7 * 32 = 224
>>>   */
>>> -#define MVPP2_SKB_HEADROOM	min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224)
>>> +#define MVPP2_SKB_HEADROOM	clamp_t(int, XDP_PACKET_HEADROOM, NET_SKB_PAD, 224)
>>>
>>>  #define MVPP2_XDP_PASS		0
>>>  #define MVPP2_XDP_DROPPED	BIT(0)
>>> diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h
>>> index b90b5b330dfa..a973394c5bc0 100644
>>> --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
>>> +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
>>> @@ -32,12 +32,24 @@
>>>  #define uISP_VAL_MAX		      ((unsigned int)((1 << uISP_REG_BIT) - 1))
>>>
>>>  /* a:fraction bits for 16bit precision, b:fraction bits for ISP precision */
>>> -#define sDIGIT_FITTING(v, a, b) \
>>> -	min_t(int, max_t(int, (((v) >> sSHIFT) >> max(sFRACTION_BITS_FITTING(a) - (b), 0)), \
>>> -	  sISP_VAL_MIN), sISP_VAL_MAX)
>>> -#define uDIGIT_FITTING(v, a, b) \
>>> -	min((unsigned int)max((unsigned)(((v) >> uSHIFT) \
>>> -	>> max((int)(uFRACTION_BITS_FITTING(a) - (b)), 0)), \
>>> -	  uISP_VAL_MIN), uISP_VAL_MAX)
>>> +static inline int sDIGIT_FITTING(short v, int a, int b)
>>> +{
>>
>> drivers/staging/media/atomisp/pci/isp/kernels/s3a/s3a_1.0/ia_css_s3a.host.c
>>
>> calls this with ia_css_3a_config.af_fir1_coef / .af_fir2_coef
>> as first argument those are of the ia_css_s0_15 type which is:
>>
>> /* Signed fixed point value, 0 integer bits, 15 fractional bits */
>> typedef s32 ia_css_s0_15;
>>
>> please replace the "short v" with "int v"
> 
> Yeah I think you're right, it's odd, because it seems that the shift value
> and the comments implies that this is a short, but perhaps it's more so
> that values are shifted as to obtain 16 bits of precision.
> 
>>
>> I think that you can then also replace clamp_t() with clamp()
> 
> The use of clamp_t() is to avoid egregious macro expansion in
> clamp(). After the series improving min/max the clamp() is probably
> equivalent. But in 5.15 it will likely not be. So this is, in line with the
> purpose of this change, I believe necesasry.

Ok fair enough.

Regards,

Hans

Powered by blists - more mailing lists