On Mon, Sep 25 2023 at 15:13, Andrzej Hajda wrote:
> @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void)
> static void
> __debug_object_init(void *addr, const struct debug_obj_descr *descr, int
> onstack)
> {
> - enum debug_obj_state state;
> struct debug_bucket *db;
> - struct debug_obj *obj;
> + struct debug_obj *obj, o;
> unsigned long flags;
>
> debug_objects_fill_pool();
> @@ -644,23 +643,19 @@ __debug_object_init(void *addr, const struct
> debug_obj_descr *descr, int onstack
> case ODEBUG_STATE_INACTIVE:
> obj->state = ODEBUG_STATE_INIT;
> break;
> -
> - case ODEBUG_STATE_ACTIVE:
> - state = obj->state;
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_print_object(obj, "init");
> - debug_object_fixup(descr->fixup_init, addr, state);
> - return;
> -
> - case ODEBUG_STATE_DESTROYED:
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> - debug_print_object(obj, "init");
> - return;
> default:
> - break;
> + o = *obj;
> + obj = NULL;
> }
>
> raw_spin_unlock_irqrestore(&db->lock, flags);
> +
> + if (obj)
> + return;
Hmm. I'd rather write is this way:
case ODEBUG_STATE_INIT:
case ODEBUG_STATE_INACTIVE:
obj->state = ODEBUG_STATE_INIT;
raw_spin_unlock_irqrestore(&db->lock, flags);
return;
default:
break;
}
o = *obj;
raw_spin_unlock_irqrestore(&db->lock, flags);
debug_print_object(&o, "init");
if (o.state == ODEBUG_STATE_ACTIVE)
debug_object_fixup(descr->fixup_init, addr, o.state);
This spares the 'obj' pointer modification and the conditional. The
extra raw_spin_unlock_irqrestore() is not the end of the world.
> void debug_object_activate(void *addr, const struct debug_obj_descr *descr)
...
> default:
> - ret = 0;
> break;
> }
> - raw_spin_unlock_irqrestore(&db->lock, flags);
> - if (print_object)
> - debug_print_object(obj, "activate");
> - return ret;
> + } else {
> + o.object = addr;
> + o.state = ODEBUG_STATE_NOTAVAILABLE;
> + o.descr = descr;
> + obj = NULL;
Hrmm. Just keep the
struct debug_obj o = { .object = addr, .state =
ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
around and get rid of this else clause.
> }
>
> raw_spin_unlock_irqrestore(&db->lock, flags);
>
> - /* If NULL the allocation has hit OOM */
> - if (!obj) {
> - debug_objects_oom();
> + if (obj)
> return 0;
Plus a similar change as above to get rid of this conditional and just
have the failure path here.
> @@ -788,30 +777,29 @@ void debug_object_deactivate(void *addr, const struct
> debug_obj_descr *descr)
> case ODEBUG_STATE_INIT:
> case ODEBUG_STATE_INACTIVE:
> case ODEBUG_STATE_ACTIVE:
> - if (!obj->astate)
> + if (!obj->astate) {
> obj->state = ODEBUG_STATE_INACTIVE;
> - else
> - print_object = true;
> - break;
> -
> + break;
> + }
> + fallthrough;
> case ODEBUG_STATE_DESTROYED:
> - print_object = true;
> + o = *obj;
> + obj = NULL;
> break;
> default:
> break;
> }
> + } else {
> + o.object = addr;
> + o.state = ODEBUG_STATE_NOTAVAILABLE;
> + o.descr = descr;
> + obj = NULL;
> }
Same here.
struct debug_obj o = { .object = addr, .state =
ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
...
if (obj) {
switch (obj->state) {
case ODEBUG_STATE_DESTROYED:
o = *obj;
break;
case ODEBUG_STATE_INIT:
case ODEBUG_STATE_INACTIVE:
case ODEBUG_STATE_ACTIVE:
if (obj->astate) {
o = *obj;
break;
}
obj->state = ODEBUG_STATE_INACTIVE;
fallthrough;
default:
raw_spin_unlock_irqrestore(&db->lock, flags);
return;
}
}
raw_spin_unlock_irqrestore(&db->lock, flags);
debug_print_object(&o, "deactivate");
Hmm?
> @@ -970,28 +962,27 @@ debug_object_active_state(void *addr, const struct
> debug_obj_descr *descr,
> if (obj) {
> switch (obj->state) {
> case ODEBUG_STATE_ACTIVE:
> - if (obj->astate == expect)
> + if (obj->astate == expect) {
> obj->astate = next;
> - else
> - print_object = true;
> - break;
> -
> + break;
> + }
> + fallthrough;
> default:
> - print_object = true;
> + o = *obj;
> + obj = NULL;
> break;
> }
> + } else {
> + o.object = addr;
> + o.state = ODEBUG_STATE_NOTAVAILABLE;
> + o.descr = descr;
> + obj = NULL;
> }
Same pattern here.
Thanks,
tglx
