On Thu, 14 Mar 2019 12:36:26 +0100, Michal Hocko wrote: > > On Thu 14-03-19 11:30:03, Vlastimil Babka wrote: > > On 3/14/19 11:15 AM, Michal Hocko wrote: > > > On Thu 14-03-19 10:42:49, Vlastimil Babka wrote: > > >> alloc_pages_exact*() allocates a page of sufficient order and then splits it > > >> to return only the number of pages requested. That makes it incompatible with > > >> __GFP_COMP, because compound pages cannot be split. > > >> > > >> As shown by [1] things may silently work until the requested size (possibly > > >> depending on user) stops being power of two. Then for CONFIG_DEBUG_VM, BUG_ON() > > >> triggers in split_page(). Without CONFIG_DEBUG_VM, consequences are unclear. > > >> > > >> There are several options here, none of them great: > > >> > > >> 1) Don't do the spliting when __GFP_COMP is passed, and return the whole > > >> compound page. However if caller then returns it via free_pages_exact(), > > >> that will be unexpected and the freeing actions there will be wrong. > > >> > > >> 2) Warn and remove __GFP_COMP from the flags. But the caller wanted it, so > > >> things may break later somewhere. > > >> > > >> 3) Warn and return NULL. However NULL may be unexpected, especially for > > >> small sizes. > > >> > > >> This patch picks option 3, as it's best defined. > > > > > > The question is whether callers of alloc_pages_exact do have any > > > fallback because if they don't then this is forcing an always fail path > > > and I strongly suspect this is not really what users want. I would > > > rather go with 2) because "callers wanted it" is much less probable than > > > "caller is simply confused and more gfp flags is surely better than > > > fewer". > > > > I initially went with 2 as well, as you can see from v1 :) but then I looked at > > the commit [2] mentioned in [1] and I think ALSA legitimaly uses __GFP_COMP so > > that the pages are then mapped to userspace. Breaking that didn't seem good. > > It used the flag legitimately before because they were allocating > compound pages but now they don't so this is just a conversion bug. We still use __GFP_COMP for allocation of the sound buffers that are also mmapped to user-space. The mentioned commit above [2] was reverted later. But honestly speaking, I'm not sure whether we still need the compound pages. The change was introduced long time ago (commit f3d48f0373c1 in 2005). Is it superfluous nowadays...? > Why should we screw up the helper for that reason? Or put in other words > why a silent fix up adds any risk? IMO, it's good to catch the incompatible usage as early as possible, so that others won't hit the same failure again like I did. There aren't so many users of __GFP_COMP in the whole tree, after all. thanks, Takashi > > The point is that with the warning in place, A developer will immediately know > > that they did something wrong, regardless if the size is power-of-two or not. > > But yeah, if it's adding of __GFP_COMP that is not deterministic, a bug can > > still sit silently for a while. > > > > But maybe we could go with 1) if free_pages_exact() is also adjusted to check > > for CompoundPage and free it properly? > > I dunno, it sounds like it adds even more confusion. > > > >> [1] https://lore.kernel.org/lkml/20181126002805.GI18977@shao2-debian/T/#u > > > > [2] > > https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?id=3a6d1980fe96dbbfe3ae58db0048867f5319cdbf > -- > Michal Hocko > SUSE Labs >
