On Mon, Jun 19, 2023 at 12:32:55AM +0200, Thomas Gleixner wrote:
> Mike!
>
> Sorry for being late on this ...
>
> On Fri, Jun 16 2023 at 11:50, Mike Rapoport wrote:
> >
> > +void *execmem_data_alloc(size_t size)
> > +{
> > + unsigned long start = execmem_params.modules.data.start;
> > + unsigned long end = execmem_params.modules.data.end;
> > + pgprot_t pgprot = execmem_params.modules.data.pgprot;
> > + unsigned int align = execmem_params.modules.data.alignment;
> > + unsigned long fallback_start = execmem_params.modules.data.fallback_start;
> > + unsigned long fallback_end = execmem_params.modules.data.fallback_end;
> > + bool kasan = execmem_params.modules.flags & EXECMEM_KASAN_SHADOW;
>
> While I know for sure that you read up on the discussion I had with Song
> about data structures, it seems you completely failed to understand it.
>
> > + return execmem_alloc(size, start, end, align, pgprot,
> > + fallback_start, fallback_end, kasan);
>
> Having _seven_ intermediate variables to fill _eight_ arguments of a
> function instead of handing in @size and a proper struct pointer is
> tasteless and disgusting at best.
>
> Six out of those seven parameters are from:
>
> execmem_params.module.data
>
> while the KASAN shadow part is retrieved from
>
> execmem_params.module.flags
>
> So what prevents you from having a uniform data structure, which is
> extensible and decribes _all_ types of allocations?
>
> Absolutely nothing. The flags part can either be in the type dependend
> part or you make the type configs an array as I had suggested originally
> and then execmem_alloc() becomes:
>
> void *execmem_alloc(type, size)
>
> and
>
> static inline void *execmem_data_alloc(size_t size)
> {
> return execmem_alloc(EXECMEM_TYPE_DATA, size);
> }
>
> which gets the type independent parts from @execmem_param.
>
> Just read through your own series and watch the evolution of
> execmem_alloc():
>
> static void *execmem_alloc(size_t size)
>
> static void *execmem_alloc(size_t size, unsigned long start,
> unsigned long end, unsigned int align,
> pgprot_t pgprot)
>
> static void *execmem_alloc(size_t len, unsigned long start,
> unsigned long end, unsigned int align,
> pgprot_t pgprot,
> unsigned long fallback_start,
> unsigned long fallback_end,
> bool kasan)
>
> In a month from now this function will have _ten_ parameters and tons of
> horrible wrappers which convert an already existing data structure into
> individual function arguments.
>
> Seriously?
>
> If you want this function to be [ab]used outside of the exec_param
> configuration space for whatever non-sensical reasons then this still
> can be either:
>
> void *execmem_alloc(params, type, size)
>
> static inline void *execmem_data_alloc(size_t size)
> {
> return execmem_alloc(&exec_param, EXECMEM_TYPE_DATA, size);
> }
>
> or
>
> void *execmem_alloc(type_params, size);
>
> static inline void *execmem_data_alloc(size_t size)
> {
> return execmem_alloc(&exec_param.data, size);
> }
>
> which both allows you to provide alternative params, right?
>
> Coming back to my conversation with Song:
>
> "Bad programmers worry about the code. Good programmers worry about
> data structures and their relationships."
Thomas, you're confusing an internal interface with external, I made the
same mistake reviewing Song's patchset...
