On 3/4/25 13:18, Lilith Gkini wrote:
> On Tue, Mar 04, 2025 at 12:20:03PM +0100, Vlastimil Babka wrote:
> Thats true. I still had the return fp == search; in my mind, but with all
Ah, right.
> these changes we can just leave it as return search == NULL; as it was,
> because we are handing the edge cases.
>
> By the time it reaches that return line it should be fine.
True.
> I was also thinking of fixing two lines to adhere to the "Breaking long
> lines and strings" (2) from the coding-style.
Hm AFAIK checkpatch was adjusted to only warn at 100 lines. While the style
document wasn't updated, we can leave such a small excess with no change.
> ---
> mm/slub.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 1f50129dcfb3..e06b88137705 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1427,7 +1427,7 @@ static int check_slab(struct kmem_cache *s, struct slab *slab)
> * Determine if a certain object in a slab is on the freelist. Must hold the
> * slab lock to guarantee that the chains are in a consistent state.
> */
> -static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> +static bool on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> {
> int nr = 0;
> void *fp;
> @@ -1437,38 +1437,48 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> fp = slab->freelist;
> while (fp && nr <= slab->objects) {
> if (fp == search)
> - return 1;
> + return true;
> if (!check_valid_pointer(s, slab, fp)) {
> if (object) {
> object_err(s, slab, object,
> "Freechain corrupt");
> set_freepointer(s, object, NULL);
> + break;
> } else {
> slab_err(s, slab, "Freepointer corrupt");
> slab->freelist = NULL;
> slab->inuse = slab->objects;
> slab_fix(s, "Freelist cleared");
> - return 0;
> + return false;
> }
> - break;
> }
> object = fp;
> fp = get_freepointer(s, object);
> nr++;
> }
>
> - max_objects = order_objects(slab_order(slab), s->size);
> + if (fp != NULL && nr > slab->objects) {
In case nr > slab->objects we already know fp can't be NULL, no? So we don't
have to test it?
> + slab_err(s, slab, "Freelist cycle detected");
> + slab->freelist = NULL;
> + slab->inuse = slab->objects;
> + slab_fix(s, "Freelist cleared");
> + return false;
> + }
> +
> + max_objects = order_objects(slab_or0der(slab), s->size);
> if (max_objects > MAX_OBJS_PER_PAGE)
> max_objects = MAX_OBJS_PER_PAGE;
>
> if (slab->objects != max_objects) {
> - slab_err(s, slab, "Wrong number of objects. Found %d but should be %d",
> + slab_err(s, slab,
> + "Wrong number of objects. Found %d but should be %d",
> slab->objects, max_objects);
> slab->objects = max_objects;
> slab_fix(s, "Number of objects adjusted");
> }
> if (slab->inuse != slab->objects - nr) {
> - slab_err(s, slab, "Wrong object count. Counter is %d but counted were %d",
> + slab_err(s, slab,
> + "Wrong object count. Counter is %d but counted were %d",
> slab->inuse, slab->objects - nr);
> slab->inuse = slab->objects - nr;
> slab_fix(s, "Object count adjusted");
>
> I do have to note that the last slab_err is of length 81 with my change,
> but it looks fine. If that one extra character is unacceptable let me
> know so I can change it to something else.
> Or if you think it's completely unnecessary I could leave it as it was
> in the first place.
Yeah can leave it.
> I just thought since we are trying to modernaze I should fix the length
> as well.
>
> Also the CHECKPATCH is complaining about the `fp != NULL` that we can
> just check fp on it's own, which is technically true, but wouldn't make
> readability worse?
> I think its better as it's in my diff cause it's more obvious, but if
> you prefer the singular fp I can change it.
I think it's not necessary to test at all but in case I'm wrong, we can do
what checkpatch suggests to be consistent with the while() condition.
> If these changes are acceptable and we don't have anything further to
> change or add I can send it as a proper commit again, But I should
> probably break it into multiple patches.
It's fine as a single patch. Thanks!
> Maybe one patch for the lines and another for the rest? Or should I
> break the bool change in it's own patch?
