... > and it will spit out > > Longest line is drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:1136 (2346kB) > ' ((((((pkt_size) + __builtin_choose_expr((sizeof(int) == > sizeof(*(8 ? ((void *)((long)((__builtin_...' > > to tell me that we have that insane 2.2 *megabyte* line due to the > MVPP2_SKB_HEADROOM thing, and I should apply this patch: > > -#define MVPP2_SKB_HEADROOM min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224) > +#define MVPP2_SKB_HEADROOM > MIN_T(int,MAX_T(int,XDP_PACKET_HEADROOM, NET_SKB_PAD), 224) > > to fix it. Or (if I've got is right): #define MVPP2_SKB_HEADROOM clamp(XDP_PACKET_HEADROOM, NET_SKB_PAD, 224) Hmmm... I've found: #define XDP_PACKET_HEADROOM 256 #define NET_SKB_PAD max(32, L1_CACHE_BYTES) I'd bet that some architecture even has a non-constant L1_CACHE_BYTES. But the 256 means the headroom is always 224 (whatever that limit is related to). It is definitely worth freeing up MIN() and MAX() for: #define MIN(x, y) ( \ BUILD_BUG_ON_ZERO(__is_constexpr((x) + (y)) + ((x) < (y) ? (x) : (y))) which is then usable for static initialisers. Just assuming that no one is silly enough to get a negative constant compared to an unsigned value. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sat, Jul 27, 2024 at 08:08:39AM GMT, David Laight wrote: > ... > > and it will spit out > > > > Longest line is drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:1136 (2346kB) > > ' ((((((pkt_size) + __builtin_choose_expr((sizeof(int) == > > sizeof(*(8 ? ((void *)((long)((__builtin_...' > > > > to tell me that we have that insane 2.2 *megabyte* line due to the > > MVPP2_SKB_HEADROOM thing, and I should apply this patch: > > > > -#define MVPP2_SKB_HEADROOM min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224) > > +#define MVPP2_SKB_HEADROOM > > MIN_T(int,MAX_T(int,XDP_PACKET_HEADROOM, NET_SKB_PAD), 224) > > > > to fix it. Yeah sorry just saw you had already addresed this Linus... I just went with a clamp()_t in my patch. > > Or (if I've got is right): > #define MVPP2_SKB_HEADROOM clamp(XDP_PACKET_HEADROOM, NET_SKB_PAD, 224) > I'm pretty sure you can clamp_t(int, ...) here safely based on usage. Part of the expansion is because of NET_SKB_PAD as you mention below... > Hmmm... > I've found: > #define XDP_PACKET_HEADROOM 256 > #define NET_SKB_PAD max(32, L1_CACHE_BYTES) > I'd bet that some architecture even has a non-constant L1_CACHE_BYTES. I checked and unless I missed something, nope. You'll see in my proposed patch that I jut replace this with a dumb #if as a result. > > But the 256 means the headroom is always 224 (whatever that limit is related to). > > It is definitely worth freeing up MIN() and MAX() for: > #define MIN(x, y) ( \ > BUILD_BUG_ON_ZERO(__is_constexpr((x) + (y)) + ((x) < (y) ? (x) : (y))) > which is then usable for static initialisers. > Just assuming that no one is silly enough to get a negative constant > compared to an unsigned value. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
From: Lorenzo Stoakes > Sent: 27 July 2024 19:59 > > On Sat, Jul 27, 2024 at 08:08:39AM GMT, David Laight wrote: > > ... > > > and it will spit out > > > > > > Longest line is drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:1136 (2346kB) > > > ' ((((((pkt_size) + __builtin_choose_expr((sizeof(int) == > > > sizeof(*(8 ? ((void *)((long)((__builtin_...' > > > > > > to tell me that we have that insane 2.2 *megabyte* line due to the > > > MVPP2_SKB_HEADROOM thing, and I should apply this patch: > > > > > > -#define MVPP2_SKB_HEADROOM min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224) > > > +#define MVPP2_SKB_HEADROOM > > > MIN_T(int,MAX_T(int,XDP_PACKET_HEADROOM, NET_SKB_PAD), 224) > > > > > > to fix it. > > Yeah sorry just saw you had already addresed this Linus... I just went with a > clamp()_t in my patch. > > > > > Or (if I've got is right): > > #define MVPP2_SKB_HEADROOM clamp(XDP_PACKET_HEADROOM, NET_SKB_PAD, 224) > > > > I'm pretty sure you can clamp_t(int, ...) here safely based on usage. Why doesn't a plain clamp() work? The xxx_t() variants really shouldn't be used very often. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, Jul 28, 2024 at 11:17:19AM GMT, David Laight wrote: > From: Lorenzo Stoakes > > Sent: 27 July 2024 19:59 > > > > On Sat, Jul 27, 2024 at 08:08:39AM GMT, David Laight wrote: > > > ... > > > > and it will spit out > > > > > > > > Longest line is drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:1136 (2346kB) > > > > ' ((((((pkt_size) + __builtin_choose_expr((sizeof(int) == > > > > sizeof(*(8 ? ((void *)((long)((__builtin_...' > > > > > > > > to tell me that we have that insane 2.2 *megabyte* line due to the > > > > MVPP2_SKB_HEADROOM thing, and I should apply this patch: > > > > > > > > -#define MVPP2_SKB_HEADROOM min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224) > > > > +#define MVPP2_SKB_HEADROOM > > > > MIN_T(int,MAX_T(int,XDP_PACKET_HEADROOM, NET_SKB_PAD), 224) > > > > > > > > to fix it. > > > > Yeah sorry just saw you had already addresed this Linus... I just went with a > > clamp()_t in my patch. > > > > > > > > Or (if I've got is right): > > > #define MVPP2_SKB_HEADROOM clamp(XDP_PACKET_HEADROOM, NET_SKB_PAD, 224) > > > > > > > I'm pretty sure you can clamp_t(int, ...) here safely based on usage. > > Why doesn't a plain clamp() work? It was on the assumption that it'd cause less of a combinatorial explosion due to the various clever checks your macros have in them. However I stand corrected after checking - it's actually a little smaller if we use clamp(), so I am fine with us just using clamp() here. > The xxx_t() variants really shouldn't be used very often. I disagree, but for the sakes of civility (+ sanity + thread length) I'm not going to go on about it :) I might (politely!) suggest, however, that under the circumstances it might be best to refrain from suggesting what should/shouldn't be used when it comes to these macros. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
On Sat, 27 Jul 2024 at 12:01, Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> > > -#define MVPP2_SKB_HEADROOM min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224)
> > > +#define MVPP2_SKB_HEADROOM
> > > MIN_T(int,MAX_T(int,XDP_PACKET_HEADROOM, NET_SKB_PAD), 224)
>
> Yeah sorry just saw you had already addresed this Linus... I just went with a
> clamp()_t in my patch.
I think your patch is better. I threw away my disgusting MIN_T() thing
here. It made more sense in pageblock-flags.h where it replaced
"min_t()".
Linus
© 2016 - 2026 Red Hat, Inc.