Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1489497 > unrolled thread

[PATCH 00/14] GPU-DRM-TTM: Fine-tuning for several function implementations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 00/14] GPU-DRM-TTM: Fine-tuning for several function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 21:40 +0200
    Re: [PATCH 00/14] GPU-DRM-TTM: Fine-tuning for several function  implementations Christian König <deathsimple@vodafone.de> - 2016-09-23 11:50 +0200
      Re: GPU-DRM-TTM: Fine-tuning for several function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 12:30 +0200
        Re: GPU-DRM-TTM: Fine-tuning for several function implementations Christian König <deathsimple@vodafone.de> - 2016-09-23 12:40 +0200
          Re: GPU-DRM-TTM: Fine-tuning for several function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 13:10 +0200
            Re: GPU-DRM-TTM: Fine-tuning for several function implementations Christian König <deathsimple@vodafone.de> - 2016-09-23 13:20 +0200
              Re: GPU-DRM-TTM: Fine-tuning for several function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 13:50 +0200
                Re: GPU-DRM-TTM: Fine-tuning for several function implementations Christian König <deathsimple@vodafone.de> - 2016-09-23 15:10 +0200
        Re: GPU-DRM-TTM: Fine-tuning for several function implementations Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-23 15:00 +0200
          Re: GPU-DRM-TTM: Fine-tuning for several function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 15:50 +0200

#1489497 — [PATCH 00/14] GPU-DRM-TTM: Fine-tuning for several function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 19:00:01 +0200

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (14):
  Use kmalloc_array() in two functions
  Rename a jump label in ttm_alloc_new_pages()
  Rename jump labels in ttm_page_pool_free()
  Rename a jump label in ttm_page_pool_get_pages()
  Use kmalloc_array() in two more functions
  Rename a jump label in ttm_dma_pool_alloc_new_pages()
  Rename jump labels in ttm_dma_page_pool_free()
  Rename a jump label in ttm_dma_pool_shrink_scan()
  Return directly after a failed kzalloc() in ttm_dma_page_alloc_init()
  Return directly after a failed kobject_init_and_add() in ttm_dma_page_alloc_init()
  Return an error code only as a constant in ttm_dma_pool_init()
  Less function calls in ttm_dma_pool_init() after error detection
  Delete unnecessary variable initialisations in ttm_dma_pool_init()
  Mark an array of text strings as "const" in ttm_dma_pool_init()

 drivers/gpu/drm/ttm/ttm_page_alloc.c     | 30 ++++++++---------
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 58 +++++++++++++++-----------------
 2 files changed, 42 insertions(+), 46 deletions(-)

-- 
2.10.0

[toc] | [next] | [standalone]

#1489904

First of all please stop sending your patches as a reply to an earlier 
and completely unrelated series.

Second please prefix all TTM related patches with "drm/ttm:".

Additional to that I don't really see the point in renaming some of the 
jump labels, if you call it "restart" or "lock_restart" doesn't make 
much difference.

Regards,
Christian.

Am 22.09.2016 um 19:32 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 22 Sep 2016 19:00:01 +0200
>
> Several update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (14):
>    Use kmalloc_array() in two functions
>    Rename a jump label in ttm_alloc_new_pages()
>    Rename jump labels in ttm_page_pool_free()
>    Rename a jump label in ttm_page_pool_get_pages()
>    Use kmalloc_array() in two more functions
>    Rename a jump label in ttm_dma_pool_alloc_new_pages()
>    Rename jump labels in ttm_dma_page_pool_free()
>    Rename a jump label in ttm_dma_pool_shrink_scan()
>    Return directly after a failed kzalloc() in ttm_dma_page_alloc_init()
>    Return directly after a failed kobject_init_and_add() in ttm_dma_page_alloc_init()
>    Return an error code only as a constant in ttm_dma_pool_init()
>    Less function calls in ttm_dma_pool_init() after error detection
>    Delete unnecessary variable initialisations in ttm_dma_pool_init()
>    Mark an array of text strings as "const" in ttm_dma_pool_init()
>
>   drivers/gpu/drm/ttm/ttm_page_alloc.c     | 30 ++++++++---------
>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 58 +++++++++++++++-----------------
>   2 files changed, 42 insertions(+), 46 deletions(-)
>

[toc] | [prev] | [next] | [standalone]

#1489924 — Re: GPU-DRM-TTM: Fine-tuning for several function implementations

> Additional to that I don't really see the point in renaming some of the jump labels,

I am suggesting changes for another collateral software evolution.


> if you call it "restart" or "lock_restart" doesn't make much difference.

Do other identifiers fit better to a specification from the document "CodingStyle"
like the following?

"…
Choose label names which say what the goto does or why the goto exists.
…"


Does this wording need any more adjustments?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1489935 — Re: GPU-DRM-TTM: Fine-tuning for several function implementations

Am 23.09.2016 um 12:20 schrieb SF Markus Elfring:
>> Additional to that I don't really see the point in renaming some of the jump labels,
> I am suggesting changes for another collateral software evolution.
>
>
>> if you call it "restart" or "lock_restart" doesn't make much difference.
> Do other identifiers fit better to a specification from the document "CodingStyle"
> like the following?

No, not really.

>
> "…
> Choose label names which say what the goto does or why the goto exists.
> …"
>
>
> Does this wording need any more adjustments?

Of hand I can't find any better wording.

It's just the names like "out" or "restart" perfectly explain why the 
labels exists. So they fulfill this requirement from the coding style as 
far as I can see.

So why do you want to change them?

Regards,
Christian.

>
> Regards,
> Markus

[toc] | [prev] | [next] | [standalone]

#1489948 — Re: GPU-DRM-TTM: Fine-tuning for several function implementations

> It's just the names like "out" or "restart" perfectly explain why the labels exists.

I have got an other impression.


> So they fulfill this requirement from the coding style as far as I can see.

Short identifiers might look more convenient in some cases because
they are quicker to type.


> So why do you want to change them?

1. I suggest to select identifiers also for jump labels which are more meaningful
   and eventually unique for some function implementations.

2. How do you think about to add a single space character before any label?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1489952 — Re: GPU-DRM-TTM: Fine-tuning for several function implementations

Am 23.09.2016 um 13:07 schrieb SF Markus Elfring:
>> It's just the names like "out" or "restart" perfectly explain why the labels exists.
> I have got an other impression.
>
>
>> So they fulfill this requirement from the coding style as far as I can see.
> Short identifiers might look more convenient in some cases because
> they are quicker to type.
>
>
>> So why do you want to change them?
> 1. I suggest to select identifiers also for jump labels which are more meaningful
>     and eventually unique for some function implementations.

I completely disagree. A longer identifier is not necessarily more 
meaningful than a shorter one.

The difference between calling a label "retry" and "lock_retry" is 
negligible, doesn't improve readability as far as I can see and is 
actually incorrect because the main meaning of the label is that we 
don't take the lock but rather that we restart the allocation operation.

Calling the label "unlock" instead of "out" is arguable a little better, 
but nothing I would call a major improvement either.

So that is a clear NAK to all those patches.

> 2. How do you think about to add a single space character before any label?

Bad as well. Why would anybody want to do this?

Christian.

>
> Regards,
> Markus

[toc] | [prev] | [next] | [standalone]

#1489967 — Re: GPU-DRM-TTM: Fine-tuning for several function implementations

> Calling the label "unlock" instead of "out" is arguable a little better,

Thanks that you can follow a renaming for this direction in principle.


> but nothing I would call a major improvement either.

This was not my intention for such an use case.

I am proposing some small software updates according to such a design pattern.


> So that is a clear NAK to all those patches.

Do you reject also update steps like the following then?

* drm/ttm: Use kmalloc_array() in two (or four?) functions"

* drm/ttm: Less function calls in ttm_dma_pool_init() after error detection

* Would you like to improve the usage of the variables "n" and "t"
  in the function "ttm_dma_pool_init" any further as Joe Perches suggested it?


>> 2. How do you think about to add a single space character before any label?
> 
> Bad as well. Why would anybody want to do this?

Do you find another software evolution interesting according to a recent commit?

"docs: Remove space-before-label guidance from CodingStyle"
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a


Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1490054 — Re: GPU-DRM-TTM: Fine-tuning for several function implementations

Am 23.09.2016 um 13:49 schrieb SF Markus Elfring:
>> Calling the label "unlock" instead of "out" is arguable a little better,
> Thanks that you can follow a renaming for this direction in principle.
>
>
>> but nothing I would call a major improvement either.
> This was not my intention for such an use case.
>
> I am proposing some small software updates according to such a design pattern.
>
>
>> So that is a clear NAK to all those patches.
> Do you reject also update steps like the following then?
>
> * drm/ttm: Use kmalloc_array() in two (or four?) functions"
>
> * drm/ttm: Less function calls in ttm_dma_pool_init() after error detection

The reason behind the advise to use kmalloc_array() is to avoid overruns 
when one of the parameters come from an IOCTL and so are controllable by 
user space.

Those overruns where the source of numerous security problems, but in 
this case the parameters don't come from an IOCTL and aren't user space 
controllable.

So this change actually doesn't make to much sense either, but I'm 
leaning towards accepting them for coding style consistency.

Regards,
Christian.

> * Would you like to improve the usage of the variables "n" and "t"
>    in the function "ttm_dma_pool_init" any further as Joe Perches suggested it?
>
>
>>> 2. How do you think about to add a single space character before any label?
>> Bad as well. Why would anybody want to do this?
> Do you find another software evolution interesting according to a recent commit?
>
> "docs: Remove space-before-label guidance from CodingStyle"
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a
>
>
> Regards,
> Markus

[toc] | [prev] | [next] | [standalone]

#1490029 — Re: GPU-DRM-TTM: Fine-tuning for several function implementations

On Fri, Sep 23, 2016 at 12:20:54PM +0200, SF Markus Elfring wrote:
> > if you call it "restart" or "lock_restart" doesn't make much difference.
> 
> Do other identifiers fit better to a specification from the document "CodingStyle"
> like the following?
> 
> "…
> Choose label names which say what the goto does or why the goto exists.
> …"
>
> 
> Does this wording need any more adjustments?

No.  I wrote that and "restart" seems like a pretty clear name to me.  I
never wrote that you should harrass people with your nonsense patches.
In fact, I have asked you over and over again to stop.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1490117 — Re: GPU-DRM-TTM: Fine-tuning for several function implementations

>> Do other identifiers fit better to a specification from the document "CodingStyle"
>> like the following?
>>
>> "…
>> Choose label names which say what the goto does or why the goto exists.
>> …"
>>
>>
>> Does this wording need any more adjustments?
> 
> No.

I have got an other impression.

The terse description can trigger disagreements about the "what" and "why",
can't it?


> I wrote that and "restart" seems like a pretty clear name to me.

This identifier might be good enough to some degree.
I imagined that it would become better by the addition of a bit of information
from the jump target.


> I never wrote that you should harrass people with your nonsense patches.

This is true in principle.

But your adjustment for the document "CodingStyle" supported also a reconsideration
of the corresponding identifier selection.

Some developers disagreed with a proposed renaming while others reacted
in a positive way.


> In fact, I have asked you over and over again to stop.

This happened under different software update contexts occasionally.

Regards,
Markus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web