Re: [PATCH 5/6] drm/amdgpu: Don't hardcode thread name length

2020-11-26 Thread Christian König 

Am 25.11.20 um 18:01 schrieb Luben Tuikov:

On 2020-11-25 04:55, Christian König wrote:

Am 25.11.20 um 04:17 schrieb Luben Tuikov:

Introduce a macro DRM_THREAD_NAME_LEN
and use that to define ring name size,
instead of hardcoding it to 16.

Signed-off-by: Luben Tuikov 
---
   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 +-
   include/drm/gpu_scheduler.h  | 2 ++
   2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 7112137689db..bbd46c6dec65 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -230,7 +230,7 @@ struct amdgpu_ring {
unsignedwptr_offs;
unsignedfence_offs;
uint64_tcurrent_ctx;
-   charname[16];
+   charname[DRM_THREAD_NAME_LEN];
u32 trail_seq;
unsignedtrail_fence_offs;
u64 trail_fence_gpu_addr;
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 61f7121e1c19..3a5686c3b5e9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -30,6 +30,8 @@
   
   #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
   
+#define DRM_THREAD_NAME_LEN TASK_COMM_LEN

+

The thread name is an amdgpu specific thing. I don't think we should
have that in the scheduler.

I need it in DRM when creating the done thread from the name
of the main scheduler thread. Since DRM creates threads,
the main scheduler thread and the done thread, it would
be good to have a preliminary limit to the name string.


And why do you use TASK_COMM_LEN here? That is completely unrelated stuff.

If you trace down into the kernel, TASK_COMM_LEN seems to be used in
snprintf() when naming a kernel thread, and its value is 16--same
as the one used in amdgpu.


Oh, that's new to me. In my memory this name was used as a filename in 
debugfs only.




So the size of the name string transitions from amdgpu to DRM to kernel
proper, where amdgpu and kernel proper set it to max 16, but DRM doesn't
give it a limit.

Sure, I can remove it from DRM, and just use a local limit
when snprintf() the name when creating a tread, possibly
using TASK_COMM_LEN. (That's in the next patch.)


Yeah, just use TASK_COMM_LEN directly where appropriate.

Regards,
Christian.



Would that be better? I can do that in v2 of this patchset.

Thanks,
Luben


Regards,
Christian.


   struct drm_gpu_scheduler;
   struct drm_sched_rq;
   

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/6] drm/amdgpu: Don't hardcode thread name length

2020-11-25 Thread Luben Tuikov 
On 2020-11-25 04:55, Christian König wrote:
> Am 25.11.20 um 04:17 schrieb Luben Tuikov:
>> Introduce a macro DRM_THREAD_NAME_LEN
>> and use that to define ring name size,
>> instead of hardcoding it to 16.
>>
>> Signed-off-by: Luben Tuikov 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 +-
>>   include/drm/gpu_scheduler.h  | 2 ++
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index 7112137689db..bbd46c6dec65 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -230,7 +230,7 @@ struct amdgpu_ring {
>>  unsignedwptr_offs;
>>  unsignedfence_offs;
>>  uint64_tcurrent_ctx;
>> -charname[16];
>> +charname[DRM_THREAD_NAME_LEN];
>>  u32 trail_seq;
>>  unsignedtrail_fence_offs;
>>  u64 trail_fence_gpu_addr;
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 61f7121e1c19..3a5686c3b5e9 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -30,6 +30,8 @@
>>   
>>   #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
>>   
>> +#define DRM_THREAD_NAME_LEN TASK_COMM_LEN
>> +
> 
> The thread name is an amdgpu specific thing. I don't think we should 
> have that in the scheduler.

I need it in DRM when creating the done thread from the name
of the main scheduler thread. Since DRM creates threads,
the main scheduler thread and the done thread, it would
be good to have a preliminary limit to the name string.

> 
> And why do you use TASK_COMM_LEN here? That is completely unrelated stuff.

If you trace down into the kernel, TASK_COMM_LEN seems to be used in
snprintf() when naming a kernel thread, and its value is 16--same
as the one used in amdgpu.

So the size of the name string transitions from amdgpu to DRM to kernel
proper, where amdgpu and kernel proper set it to max 16, but DRM doesn't
give it a limit.

Sure, I can remove it from DRM, and just use a local limit
when snprintf() the name when creating a tread, possibly
using TASK_COMM_LEN. (That's in the next patch.)

Would that be better? I can do that in v2 of this patchset.

Thanks,
Luben

> 
> Regards,
> Christian.
> 
>>   struct drm_gpu_scheduler;
>>   struct drm_sched_rq;
>>   
> 

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/6] drm/amdgpu: Don't hardcode thread name length

2020-11-25 Thread Christian König 

Am 25.11.20 um 04:17 schrieb Luben Tuikov:

Introduce a macro DRM_THREAD_NAME_LEN
and use that to define ring name size,
instead of hardcoding it to 16.

Signed-off-by: Luben Tuikov 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 +-
  include/drm/gpu_scheduler.h  | 2 ++
  2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 7112137689db..bbd46c6dec65 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -230,7 +230,7 @@ struct amdgpu_ring {
unsignedwptr_offs;
unsignedfence_offs;
uint64_tcurrent_ctx;
-   charname[16];
+   charname[DRM_THREAD_NAME_LEN];
u32 trail_seq;
unsignedtrail_fence_offs;
u64 trail_fence_gpu_addr;
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 61f7121e1c19..3a5686c3b5e9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -30,6 +30,8 @@
  
  #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
  
+#define DRM_THREAD_NAME_LEN TASK_COMM_LEN

+


The thread name is an amdgpu specific thing. I don't think we should 
have that in the scheduler.


And why do you use TASK_COMM_LEN here? That is completely unrelated stuff.

Regards,
Christian.


  struct drm_gpu_scheduler;
  struct drm_sched_rq;
  


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/6] drm/amdgpu: Don't hardcode thread name length

2020-11-24 Thread Luben Tuikov 
Introduce a macro DRM_THREAD_NAME_LEN
and use that to define ring name size,
instead of hardcoding it to 16.

Signed-off-by: Luben Tuikov 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 +-
 include/drm/gpu_scheduler.h  | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 7112137689db..bbd46c6dec65 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -230,7 +230,7 @@ struct amdgpu_ring {
unsignedwptr_offs;
unsignedfence_offs;
uint64_tcurrent_ctx;
-   charname[16];
+   charname[DRM_THREAD_NAME_LEN];
u32 trail_seq;
unsignedtrail_fence_offs;
u64 trail_fence_gpu_addr;
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 61f7121e1c19..3a5686c3b5e9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -30,6 +30,8 @@
 
 #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
 
+#define DRM_THREAD_NAME_LEN TASK_COMM_LEN
+
 struct drm_gpu_scheduler;
 struct drm_sched_rq;
 
-- 
2.29.2.154.g7f7ebe054a

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx