On Fri, 22 Mar 2024, Dan Carpenter wrote: > On Fri, Mar 22, 2024 at 12:47:42PM +1100, NeilBrown wrote: > > On Thu, 21 Mar 2024, Dan Carpenter wrote: > > > On Mon, Mar 04, 2024 at 09:45:48AM +1100, NeilBrown wrote: > > > > I have in mind a more explicit statement of how much waiting is > > > > acceptable. > > > > > > > > GFP_NOFAIL - wait indefinitely > > > > > > Why not call it GFP_SMALL? It wouldn't fail. The size would have to be > > > less than some limit. If the size was too large, that would trigger a > > > WARN_ON_ONCE(). > > > > I would be happy with GFP_SMALL. It would never return NULL but might > > block indefinitely. It would (as you say) WARN (maybe ONCE) if the size > > was considered "COSTLY" and would possibly BUG if the size exceeded > > KMALLOC_MAX_SIZE. > > I'd like to keep GFP_SMALL much smaller than KMALLOC_MAX_SIZE. IIf > you're allocating larger than that, you'd still be able to GFP_NOFAIL. > I looked quickly an I think over 60% of allocations are just sizeof(*p) > and probably 90% are under 4k. What do you mean exactly by "keep"?? Do you mean WARN_ON if it is "too big" - certainly agree. Do you mean BUG_ON if it is "too big" - maybe agree. Do you mean return NULL if it is "too big" - definitely disagree. Do you mean build failure if it could be too big - I would LOVE that, but I don't think we can do that with current build tools. Thanks, NeilBrown > > > > > > > > > I obviously understand that this duplicates the information in the size > > > parameter but the point is that GFP_SMALL allocations have been > > > reviewed, updated, and don't have error handling code. > > > > We are on the same page here. > > > > > > > > We'd keep GFP_KERNEL which would keep the existing behavior. (Which is > > > that it can sleep and it can fail). I think that maps to GFP_RETRY but > > > GFP_RETRY is an uglier name. > > > > Can it fail though? I know it is allowed to, but does it happen? > > > > In some sense, I don't really care about this, I just want the rules > clear from a static analysis perspective. Btw, you're discussing making > the too small to fail rule official but other times we have discussed > getting rid of it all together. So I think maybe it's better to keep > the rules strict but allow the actual implentation to change later. > > The GFP_SMALL stuff is nice for static analysis because it would warn > about anything larger than whatever the small limit is. So that means I > have fewer allocations to review for integer overflow bugs. > > Btw, Jiri Pirko, was proposing a macro which would automatically > allocate the 60+% of allocations which are sizeof(*p). > https://lore.kernel.org/all/20240315132249.2515468-1-jiri@xxxxxxxxxxx/ > I had offered an alternative macro but the idea is the same: > > #define __ALLOC(p) p __free(kfree) = kzalloc(sizeof(*p), GFP_KERNEL) > struct ice_aqc_get_phy_caps_data *__ALLOC(pcaps); > > Combining no fail allocations with automatic cleanup changes the way you > write code. > > And then on the success patch you have the no_free_ptr() which I haven't > used but I think will be so useful for static analysis. I'm so excited > about this. > > regards, > dan carpenter > > >
