Messages in this thread |  | | Subject | Re: [PATCH v4] livepatch: introduce shadow variable API | From | Joe Lawrence <> | Date | Wed, 16 Aug 2017 09:40:44 -0400 |
| |
On 08/16/2017 08:43 AM, Miroslav Benes wrote: > >> [ ... snip ... ] > > There is a comment above about locking and we do not take the spinlock > here. That could surprise someone. So I'd keep only klp_shadow_add() > comment, because there it is strictly needed. It depends on the context in > all other cases.
Good catch, I think this changed in this last version when I moved some of the work outside the lock.
> Could you also add a comment above klp_shadow_lock definition about what > it aims to protect? >
How about "klp_shadow_lock provides exclusive access to the klp_shadow_hash and the shadow variables it references." or were thinking of something more detailed?
>> + /* Look for <obj, id> again under the lock */ >> + spin_lock_irqsave(&klp_shadow_lock, flags); >> + shadow_data = klp_shadow_get(obj, id); >> + if (unlikely(shadow_data)) { > > shadow_data is not needed anywhere, so you could do the same as for the > first speculative search and remove shadow_data variable all together.
Ok.
>> [ ... snip ... ] > > Otherwise it looks good. You can add my > > Acked-by: Miroslav Benes <mbenes@suse.cz> > > with those nits fixed.
Thank you for all the suggestions and reviews!
-- Joe
|  |