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