Re: [Lsf-pc] [LSF/MM TOPIC] Matthew's minor MM topics

[Michal Hocko <mhocko@kernel.org>]

On Tue 16-01-18 06:13:54, Matthew Wilcox wrote:
> (trying again with the right MM mailing list address.  Sorry.)
> 
> I have a number of things I'd like to discuss that are purely MM related.
> I don't know if any of them rise to the level of an entire session,
> but maybe lightning talks, or maybe we can dispose of them on the list
> before the summit.
> 
> 1. GFP_DMA / GFP_HIGHMEM / GFP_DMA32
> 
> The documentation is clear that only one of these three bits is allowed
> to be set.  Indeed, we have code that checks that only one of these
> three bits is set.  So why do we have three bits?  Surely this encoding
> works better:
> 
> 00b (normal)
> 01b GFP_DMA
> 10b GFP_DMA32
> 11b GFP_HIGHMEM
> (or some other clever encoding that maps well to the zone_type index)

Didn't you forget about movable zone? Anyway, if you can simplify this
thing I would be more than happy. GFP_ZONE_TABLE makes my head spin
anytime I dare to look.

> 2. kvzalloc_ab_c()
> 
> We could bikeshed on this name all summit long, but the idea is to provide
> an equivalent of kvmalloc_array() which works for array-plus-header.
> These allocations are legion throughout the kernel.  Here's the first
> one I found with a grep:
> 
> drivers/vhost/vhost.c:  newmem = kvzalloc(size + mem.nregions * sizeof(*m->regions), GFP_KERNEL);
> 
> ... and, yep, that one's a security hole.
> 
> The implementation is not hard, viz:
> 
> +static inline void *kvzalloc_ab_c(size_t n, size_t size, size_t c, gfp_t flags)
> +{
> +       if (size != 0 && n > (SIZE_MAX - c) / size)
> +               return NULL;
> +
> +       return kvmalloc(n * size + c, flags);
> +}
> 
> but the name will tie us in knots and getting people to actually use
> it will be worse.  (I actually stole the name from another project,
> but I can't find it now).
> 
> We also need to go through and convert dozens of callers that are
> doing kvzalloc(a * b) into kvzalloc_array(a, b).  Maybe we can ask for
> some coccinelle / smatch / checkpatch help here.

I do not see anything controversial here. Is there anything to be
discussed here? If there is a common pattern then a helper shouldn't be
a big deal, no?

> 3. Maybe we could rename kvfree() to just free()?  Please?  There's
> nothing special about it.  One fewer thing for somebody to learn when
> coming fresh to kernel programming.

I guess one has to learn about kvmalloc already and kvfree is nicely
symmetric to it.

> 4. vmf_insert_(page|pfn|mixed|...)
> 
> vm_insert_foo are invariably called from fault handlers, usually as
> the last thing we do before returning a VM_FAULT code.  As such, why do
> they return an errno that has to be translated?  We would be better off
> returning VM_FAULT codes from these functions.

Which tree are you looking at? git grep vmf_insert_ doesn't show much.
vmf_insert_pfn_p[mu]d and those already return VM_FAULT error code from
a quick look.

> Related, I'd like to introduce a new vm_fault_t typedef for unsigned
> int that indicates that the function returns VM_FAULT flags rather than
> an errno.  We've had so many mistakes in this area.

This sounds like a reasonable thing to do.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>