Re: [PATCH 2/2] ipc: semaphores: do not hold ipc lock more than necessary

[Linus Torvalds]

On Fri, Mar 1, 2013 at 4:16 PM, Davidlohr Bueso <davidlohr.bueso@xxxxxx> wrote:
> +static inline struct sem_array *sem_obtain_object(struct ipc_namespace *ns, int id)
> +{
> +       struct kern_ipc_perm *ipcp = ipc_obtain_object(&sem_ids(ns), id);
> +
> +       if (IS_ERR(ipcp))
> +               return (struct sem_array *)ipcp;

This should use ERR_CAST() to make it more obvious what's going on.

> +static inline struct sem_array *sem_obtain_object_check(struct ipc_namespace *ns,
> +                                                       int id)
> +{
> +       struct kern_ipc_perm *ipcp = ipc_obtain_object_check(&sem_ids(ns), id);
> +
> +       if (IS_ERR(ipcp))
> +               return (struct sem_array *)ipcp;

Same here.

> +/*
> + * Call inside the rcu read section.
> + */
> +static inline void sem_getref(struct sem_array *sma)
> +{
> +       spin_lock(&(sma)->sem_perm.lock);
> +       ipc_rcu_getref(sma);
> +       ipc_unlock(&(sma)->sem_perm);
> +}

This really makes me wonder if we shouldn't just use an atomic counter
for "refcount". But I guess that would be a separate patch.

But all the uses of refcount really look like the normal atomic ops
migth be the right thing. Especially if we no longer expect to hold
the lock most of the time.

> +               spin_lock(&sma->sem_perm.lock);

I really would almost want to make these things be "ipc_lock_object()"
rather than an open-coded spinlock like this. But that's not a big
deal.

Patch looks fine to me in general.

           Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/