Re: [PATCH v8] KEYS: Add a list for unreferenced keys

[David Howells]

Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote:

> +	spin_lock_irqsave(&key_graveyard_lock, flags);
> +	list_splice_init(&key_graveyard, &graveyard);
> +	spin_unlock_irqrestore(&key_graveyard_lock, flags);

I would wrap this bit in a check to see if key_graveyard is empty so that we
can avoid disabling irqs and taking the lock if the graveyard is empty.

> +		if (!refcount_inc_not_zero(&key->usage)) {

Sorry, but eww.  You're going to wangle the refcount twice on every key on the
system every time the gc does a pass.  Further, in some cases inc_not_zero is
not the fastest op in the world.

> +			spin_lock_irqsave(&key_graveyard_lock, flags);
> +			list_add_tail(&key->graveyard_link, &key_graveyard);
> +			spin_unlock_irqrestore(&key_graveyard_lock, flags);
>  			schedule_work(&key_gc_work);

This is going to enable and disable interrupts twice and that can be
expensive, depending on the arch.  I wonder if it would be better to do:

			local_irq_save(flags);
			spin_lock(&key_graveyard_lock);
			list_add_tail(&key->graveyard_link, &key_graveyard);
			spin_unlock(&key_graveyard_lock);
			schedule_work(&key_gc_work);
			local_irq_restore(flags);

David