Messages in this thread | | | Date | Fri, 30 Jul 2010 12:20:27 +0300 | Subject | Re: [PATCH 01/13] AppArmor: misc. base functions and defines | From | Pekka Enberg <> |
| |
On Fri, Jul 30, 2010 at 12:47 AM, John Johansen <john.johansen@canonical.com> wrote: > +/** > + * kvmalloc - do allocation preferring kmalloc but falling back to vmalloc > + * @size: size of allocation > + * > + * Return: allocated buffer or NULL if failed > + * > + * It is possible that policy being loaded from the user is larger than > + * what can be allocated by kmalloc, in those cases fall back to vmalloc. > + */ > +void *kvmalloc(size_t size) > +{ > + void *buffer = NULL; > + > + if (size == 0) > + return NULL; > + > + /* do not attempt kmalloc if we need more than 16 pages at once */ > + if (size <= (16*PAGE_SIZE)) > + buffer = kmalloc(size, GFP_NOIO | __GFP_NOWARN);
16 pages is a lot of memory for 64 K pages. What's the purpose of GFP_NOIO here? vmalloc() will do GFP_KERNEL allocations anyway.
> + if (!buffer) { > + /* see kvfree for why size must be at least work_struct size > + * when allocated via vmalloc > + */ > + if (size < sizeof(struct work_struct)) > + size = sizeof(struct work_struct); > + buffer = vmalloc(size); > + } > + return buffer; > +}
Please don't hide this into apparmor internals. People have invented this function in the past so maybe it's time to put it in mm/util.c?
> + > +/** > + * do_vfree - workqueue routine for freeing vmalloced memory > + * @work: data to be freed > + * > + * The work_struct is overlaid to the data being freed, as at the point > + * the work is scheduled the data is no longer valid, be its freeing > + * needs to be delayed until safe. > + */ > +static void do_vfree(struct work_struct *work) > +{ > + vfree(work); > +} > + > +/** > + * kvfree - free an allocation do by kvmalloc > + * @buffer: buffer to free (MAYBE_NULL) > + * > + * Free a buffer allocated by kvmalloc > + */ > +void kvfree(void *buffer) > +{ > + if (is_vmalloc_addr(buffer)) { > + /* Data is no longer valid so just use the allocated space > + * as the work_struct > + */ > + struct work_struct *work = (struct work_struct *) buffer; > + INIT_WORK(work, do_vfree); > + schedule_work(work);
I don't understand this part here. Is it needed for interrupt contexts or does vfree() sleep somewhere? If it's for the former, I think we can just add a comment saying that kvmalloc/kvfree is not safe from interrupt context and remove the schedule_work() parts here.
> + } else > + kfree(buffer); > +} -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |