[PATCH 5/5] staging: vc04_services: don't increment service refcount when it's not needed

Marcelo Diop-Gonzalez marcgonzalez at google.com
Thu Feb 13 17:03:32 UTC 2020


On Wed, Feb 12, 2020 at 1:43 PM Marcelo Diop-Gonzalez
<marcgonzalez at google.com> wrote:
>
> There are a few places where a service's reference count is incremented,
> something quick is done, and the refcount is dropped. This can be made
> a little simpler/faster by not grabbing a reference in these cases.
>
> Signed-off-by: Marcelo Diop-Gonzalez <marcgonzalez at google.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 16 ++++-----
>  .../interface/vchiq_arm/vchiq_core.c          | 36 +++++++++++++------
>  .../interface/vchiq_arm/vchiq_core.h          |  8 ++++-
>  3 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 3ed0e4ea7f5c..b377f18aed45 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -2497,11 +2497,11 @@ vchiq_instance_get_use_count(struct vchiq_instance *instance)
>         int use_count = 0, i;
>
>         i = 0;
> -       while ((service = next_service_by_instance(instance->state,
> -               instance, &i))) {
> +       rcu_read_lock();
> +       while ((service = __next_service_by_instance(instance->state,
> +                                                    instance, &i)))
>                 use_count += service->service_use_count;
> -               unlock_service(service);
> -       }
> +       rcu_read_unlock();
>         return use_count;
>  }
>
> @@ -2524,11 +2524,11 @@ vchiq_instance_set_trace(struct vchiq_instance *instance, int trace)
>         int i;
>
>         i = 0;
> -       while ((service = next_service_by_instance(instance->state,
> -               instance, &i))) {
> +       rcu_read_lock();
> +       while ((service = __next_service_by_instance(instance->state,
> +                                                    instance, &i)))
>                 service->trace = trace;
> -               unlock_service(service);
> -       }
> +       rcu_read_unlock();
>         instance->trace = (trace != 0);
>  }
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> index 65270a5b29db..d7d7f4d9d57f 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -222,28 +222,42 @@ find_closed_service_for_instance(struct vchiq_instance *instance,
>  }
>
>  struct vchiq_service *
> -next_service_by_instance(struct vchiq_state *state, struct vchiq_instance *instance,
> -                        int *pidx)
> +__next_service_by_instance(struct vchiq_state *state,
> +                          struct vchiq_instance *instance,
> +                          int *pidx)
>  {
>         struct vchiq_service *service = NULL;
>         int idx = *pidx;
>
> -       rcu_read_lock();
>         while (idx < state->unused_service) {
>                 struct vchiq_service *srv;
>
>                 srv = rcu_dereference(state->services[idx++]);
>                 if (srv && srv->srvstate != VCHIQ_SRVSTATE_FREE &&
> -                   srv->instance == instance &&
> -                   kref_get_unless_zero(&srv->ref_count)) {
> -                       service = rcu_pointer_handoff(srv);
> +                   srv->instance == instance) {
> +                       service = srv;
>                         break;
>                 }
>         }
> -       rcu_read_unlock();
>
>         *pidx = idx;
> +       return service;
> +}
>
> +struct vchiq_service *
> +next_service_by_instance(struct vchiq_state *state,
> +                        struct vchiq_instance *instance,
> +                        int *pidx)
> +{
> +       struct vchiq_service *service;
> +
> +       rcu_read_lock();
> +       service = __next_service_by_instance(state, instance, pidx);
> +       if (service && kref_get_unless_zero(&service->ref_count))
> +               service = rcu_pointer_handoff(service);
> +       else
> +               service = NULL;
> +       rcu_read_unlock();
>         return service;
>  }

ahh wait, this one is buggy..... If kref_get_unless_zero fails then we
want to keep looking
for the next one. Greg, since you already applied this one, would it
be best for me to send
a patch on top of this or send a V2?

-Marcelo

>
> @@ -283,13 +297,13 @@ unlock_service(struct vchiq_service *service)
>  int
>  vchiq_get_client_id(unsigned int handle)
>  {
> -       struct vchiq_service *service = find_service_by_handle(handle);
> +       struct vchiq_service *service;
>         int id;
>
> +       rcu_read_lock();
> +       service = handle_to_service(handle);
>         id = service ? service->client_id : 0;
> -       if (service)
> -               unlock_service(service);
> -
> +       rcu_read_unlock();
>         return id;
>  }
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> index 30e4965c7666..cedd8e721aae 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -572,7 +572,13 @@ find_closed_service_for_instance(struct vchiq_instance *instance,
>         unsigned int handle);
>
>  extern struct vchiq_service *
> -next_service_by_instance(struct vchiq_state *state, struct vchiq_instance *instance,
> +__next_service_by_instance(struct vchiq_state *state,
> +                          struct vchiq_instance *instance,
> +                          int *pidx);
> +
> +extern struct vchiq_service *
> +next_service_by_instance(struct vchiq_state *state,
> +                        struct vchiq_instance *instance,
>                          int *pidx);
>
>  extern void
> --
> 2.25.0.225.g125e21ebc7-goog
>


More information about the devel mailing list