On Wed, Feb 12, 2020 at 1:43 PM Marcelo Diop-Gonzalez
<marcgonzalez@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> .../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
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
