Re: [RFC PATCH v2 1/2] media: Add function removing all media entity links

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hi Sakari,

On 06/10/2013 12:15 AM, Sakari Ailus wrote:
> Sylwester Nawrocki wrote:
> ...
>>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>>>> index e1cd132..bd85dc3 100644
>>>> --- a/drivers/media/media-entity.c
>>>> +++ b/drivers/media/media-entity.c
>>>> @@ -429,6 +429,57 @@ media_entity_create_link(struct media_entity
>>>> *source, u16 source_pad,
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(media_entity_create_link);
>>>>
>>>> +void __media_entity_remove_links(struct media_entity *entity)
>>>> +{
>>>> +    int i, r;
>>>
>>> u16? r can be defined inside the loop.
>>
>> I would argue 'unsigned int' would be more optimal type for i in most
>> cases. Will move r inside the loop.
>>
>>>> +    for (i = 0; i<  entity->num_links; i++) {
>>>> +        struct media_link *link =&entity->links[i];
>>>> +        struct media_entity *remote;
>>>> +        int num_links;
>>>
>>> num_links is u16 in struct media_entity. I'd use the same type.
>>
>> I would go with 'unsigned int', as a more natural type for the CPU in
>> most cases. It's a minor issue, but I can't see how u16 would have been
>> more optimal than unsigned int for a local variable like this, while
>> this code is mostly used on 32-bit systems at least.
>>
>>>> +        if (link->source->entity == entity)
>>>> +            remote = link->sink->entity;
>>>> +        else
>>>> +            remote = link->source->entity;
>>>> +
>>>> +        num_links = remote->num_links;
>>>> +
>>>> +        for (r = 0; r<  num_links; r++) {
>>>
>>> Is caching remote->num_links needed, or do I miss something?
>>
>> Yes, it is needed, remote->num_links is decremented inside the loop.
> 
> Oh, forgot this one... yes, remote->num_links is decremented, but so is 
> r it's compared to. So I don't think it's necessary to cache it, but 
> it's neither an error to do so.

Indeed, it seems more correct to not cache it, thanks.

>>>> +            struct media_link *rlink =&remote->links[r];
>>>> +
>>>> +            if (rlink != link->reverse)
>>>> +                continue;
>>>> +
>>>> +            if (link->source->entity == entity)
>>>> +                remote->num_backlinks--;
>>>> +
>>>> +            remote->num_links--;
>>>> +
>>>> +            if (remote->num_links<  1)
>>>
>>> How about: if (!remote->num_links) ?
>>
>> Hmm, perhaps squashing the above two lines to:
>>
>>              if (--remote->num_links == 0)
>>
>> ?
> 
> I'm not too fond of --/++ operators as part of more complex structures 
> so I'd keep it separate. Fewer lines doesn't always equate to more 
> readable code. :-)

In general I agree, however it's quite simple statement in this case -
only decrement and test, it's often only one instruction even in the
assembly language...

I'm going to squash following to this patch:

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index f5ea82e..1fb619d 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -436,18 +436,18 @@ void __media_entity_remove_links(struct media_entity *entity)
        for (i = 0; i < entity->num_links; i++) {
                struct media_link *link = &entity->links[i];
                struct media_entity *remote;
-               unsigned int r, num_links;
+               unsigned int r;

                if (link->source->entity == entity)
                        remote = link->sink->entity;
                else
                        remote = link->source->entity;

-               num_links = remote->num_links;
-
-               for (r = 0; r < num_links; r++) {
-                       if (remote->links[r] != link->reverse)
+               while (r < remote->num_links; r++) {
+                       if (remote->links[r] != link->reverse) {
+                               r++;
                                continue;
+                       }

                        if (link->source->entity == entity)
                                remote->num_backlinks--;
@@ -456,7 +456,7 @@ void __media_entity_remove_links(struct media_entity *entity)
                                break;

                        /* Insert last entry in place of the dropped link. */
-                       remote->links[r--] = remote->links[remote->num_links];
+                       remote->links[r] = remote->links[remote->num_links];
                }
        }

So the loop looks something like:


		while (r < remote->num_links) {
			if (remote->links[r] != link->reverse) {
				r++;
				continue;
			}

			if (link->source->entity == entity)
				remote->num_backlinks--;

			if (--remote->num_links == 0)
				break;

			/* Insert last entry in place of the dropped link. */
			remote->links[r] = remote->links[remote->num_links];
		}

Does it sound OK ?

Regards,
Sylwester