It generally looks ok. Only few questions below...

[...]

> +In-flight parent objects
> +------------------------
> +
> +Sometimes it may not be convenient or possible to allocate shadow
> +variables alongside their parent objects.  Or a livepatch fix may
> +require shadow varibles to only a subset of parent object instances.  In
> +these cases, the klp_shadow_get_or_attach() call can be used to attach
> +shadow variables to parents already in-flight.
> +
> +For commit 1d147bfa6429, a good spot to attach a shadow spinlock is
> +inside ieee80211_sta_ps_deliver_wakeup():
> +
> +#define PS_LOCK 1
> +void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
> +{
> +     DEFINE_SPINLOCK(ps_lock_fallback);
> +     spinlock_t *ps_lock;
> +
> +     /* sync with ieee80211_tx_h_unicast_ps_buf */
> +     ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
> +                     &ps_lock_fallback, sizeof(ps_lock_fallback),
> +                     GFP_ATOMIC);
> +
> +     ps_lock = klp_shadow_get(sta, PS_LOCK);
> +     if (ps_lock)
> +             spin_lock(ps_lock);

ps_lock = klp_shadow_get(sta, PS_LOCK); should not be needed, should it?

[...]

> +/*
> + * klp_shadow_set() - initialize a shadow variable
> + * @shadow:  shadow variable to initialize
> + * @obj:     pointer to parent object
> + * @id:              data identifier
> + * @data:    pointer to data to attach to parent
> + * @size:    size of attached data
> + */
> +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
> +                               unsigned long id, void *data, size_t size)
> +{
> +     shadow->obj = obj;
> +     shadow->id = id;
> +
> +     if (data)
> +             memcpy(shadow->data, data, size);
> +}
> +
> +/**
> + * klp_shadow_add() - add a shadow variable to the hashtable
> + * @shadow:  shadow variable to add
> + */
> +static inline void klp_shadow_add(struct klp_shadow *shadow)
> +{
> +     hash_add_rcu(klp_shadow_hash, &shadow->node,
> +                  (unsigned long)shadow->obj);
> +}

It would be nice to add a comment that a caller must hold klp_shadow_lock 
spinlock.

> +/**
> + * klp_shadow_attach() - allocate and add a new shadow variable
> + * @obj:     pointer to parent object
> + * @id:              data identifier
> + * @data:    pointer to data to attach to parent
> + * @size:    size of attached data
> + * @gfp_flags:       GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, this routine
> + * will issue a WARN, exit early and return NULL.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space.  If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed.  The new shadow variable is then added to the
> + * global hashtable.
> + *
> + * Return: the shadow variable data element, NULL on duplicate or
> + * failure.
> + */
> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> +                     size_t size, gfp_t gfp_flags)
> +{
> +     struct klp_shadow *new_shadow;
> +     void *shadow_data;
> +     unsigned long flags;
> +
> +     /* Take error exit path if <obj, id> already exists */
> +     shadow_data = klp_shadow_get(obj, id);
> +     if (unlikely(shadow_data))
> +             goto err_exists;
> +
> +     /* Allocate a new shadow variable for use inside the lock below */
> +     new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> +     if (!new_shadow)
> +             goto err;
> +
> +     /* 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 variable found, throw away allocation and take
> +              * error exit path */
> +             spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +             kfree(shadow_data);
> +             goto err_exists;
> +     }
> +
> +     /* No <obj, id> found, add the newly allocated one */
> +     shadow_data = data;
> +     klp_shadow_set(new_shadow, obj, id, data, size);
> +     klp_shadow_add(new_shadow);
> +     spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +     return shadow_data;

I may be missing something, but shouldn't this return new_shadow->data? 
You return original data here which seems strange.

> +
> +err_exists:
> +     WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +err:
> +     return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> +
> +/**
> + * klp_shadow_get_or_attach() - get existing or attach a new shadow variable
> + * @obj:     pointer to parent object
> + * @id:              data identifier
> + * @data:    pointer to data to attach to parent
> + * @size:    size of attached data
> + * @gfp_flags:       GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, it will be
> + * used (but *not* updated) in the return value of this function.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space.  If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed.  The new shadow variable is then added to the
> + * global hashtable.
> + *
> + * Return: the shadow variable data element, NULL on failure.
> + */
> +void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> +                            size_t size, gfp_t gfp_flags)
> +{
> +     struct klp_shadow *new_shadow;
> +     void *shadow_data;
> +     unsigned long flags;
> +
> +     /* Return a shadow variable if <obj, id> already exists */
> +     shadow_data = klp_shadow_get(obj, id);
> +     if (shadow_data)
> +             goto ret;

This could be "return ret;" directly. But no big deal.

> +     /* Allocate a new shadow variable for use inside the lock below */
> +     new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> +     if (!new_shadow)
> +             goto err;
> +
> +     /* 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 variable found, throw away allocation and
> +              * return the shadow variable data */
> +             spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +             kfree(shadow_data);
> +             goto ret;
> +     }
> +
> +     /* No <obj, id> found, so attach the newly allocated one */
> +     shadow_data = data;

Again. "shadow_data = new_shadow->data;"?

> +     klp_shadow_set(new_shadow, obj, id, data, size);
> +     klp_shadow_add(new_shadow);
> +     spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +ret:
> +     return shadow_data;
> +err:
> +     return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_get_or_attach);
> +
> +/**
> + * klp_shadow_update_or_attach() - update or attach a new shadow variable
> + * @obj:     pointer to parent object
> + * @id:              data identifier
> + * @data:    pointer to data to attach to parent
> + * @size:    size of attached data
> + * @gfp_flags:       GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, it will be
> + * updated and used in the return value of this function.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space.  If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed.  The new shadow variable is then added to the
> + * global hashtable.
> + *
> + * Return: the shadow variable data element, NULL on failure.
> + */
> +void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
> +                               size_t size, gfp_t gfp_flags)
> +{
> +     struct klp_shadow *new_shadow;
> +     void *shadow_data;
> +     unsigned long flags;
> +
> +     /* Update/return a shadow variable if <obj, id> already exists */
> +     spin_lock_irqsave(&klp_shadow_lock, flags);
> +     shadow_data = klp_shadow_get(obj, id);
> +     if (shadow_data)
> +             goto update_unlock_ret;
> +     spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +     /* Allocate a new shadow variable for use inside the lock below */
> +     new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> +     if (!new_shadow)
> +             goto err;
> +
> +     /* Look for <obj, id> again under the lock, this time with an
> +      * allocation in hand to use if need to use it. */
> +     spin_lock_irqsave(&klp_shadow_lock, flags);
> +     shadow_data = klp_shadow_get(obj, id);
> +     if (unlikely(shadow_data)) {
> +
> +             /* Shadow variable found, throw away allocation and
> +              * update/return the existing one */
> +             kfree(new_shadow);
> +             goto update_unlock_ret;
> +     }
> +
> +     /* Could not find one, so attach the new one */
> +     shadow_data = data;

Dtto.

> +     klp_shadow_set(new_shadow, obj, id, data, size);
> +     klp_shadow_add(new_shadow);
> +     spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +     return shadow_data;
> +
> +update_unlock_ret:
> +     /* Update already attached shadow variable */
> +     new_shadow = container_of(shadow_data, struct klp_shadow, data);
> +     klp_shadow_set(new_shadow, obj, id, data, size);
> +     spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +     return shadow_data;
> +err:
> +     return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_update_or_attach);

The rest is good.

Thanks,
Miroslav

Reply via email to