* [PATCH] net: ena: initialize dim_sample
@ 2023-01-08 14:38 Tom Rix
2023-01-09 10:34 ` Eric Dumazet
2023-01-10 16:44 ` Jiri Pirko
0 siblings, 2 replies; 7+ messages in thread
From: Tom Rix @ 2023-01-08 14:38 UTC (permalink / raw)
To: shayagr, akiyano, darinzon, ndagan, saeedb, davem, edumazet,
kuba, pabeni, nathan, ndesaulniers, khalasa, wsa+renesas,
yuancan, tglx, 42.hyeyoo
Cc: netdev, linux-kernel, llvm, Tom Rix
clang static analysis reports this problem
drivers/net/ethernet/amazon/ena/ena_netdev.c:1821:2: warning: Passed-by-value struct
argument contains uninitialized data (e.g., field: 'comp_ctr') [core.CallAndMessage]
net_dim(&ena_napi->dim, dim_sample);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net_dim can call dim_calc_stats() which uses the comp_ctr element,
so it must be initialized.
Fixes: 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive interrupt moderation")
Signed-off-by: Tom Rix <trix@redhat.com>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index e8ad5ea31aff..938184465eae 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1805,7 +1805,7 @@ static void ena_dim_work(struct work_struct *w)
static void ena_adjust_adaptive_rx_intr_moderation(struct ena_napi *ena_napi)
{
- struct dim_sample dim_sample;
+ struct dim_sample dim_sample = {};
struct ena_ring *rx_ring = ena_napi->rx_ring;
if (!rx_ring->per_napi_packets)
--
2.27.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ena: initialize dim_sample
2023-01-08 14:38 [PATCH] net: ena: initialize dim_sample Tom Rix
@ 2023-01-09 10:34 ` Eric Dumazet
2023-01-10 16:58 ` Shay Agroskin
2023-01-10 16:44 ` Jiri Pirko
1 sibling, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2023-01-09 10:34 UTC (permalink / raw)
To: Tom Rix
Cc: shayagr, akiyano, darinzon, ndagan, saeedb, davem, kuba, pabeni,
nathan, ndesaulniers, khalasa, wsa+renesas, yuancan, tglx,
42.hyeyoo, netdev, linux-kernel, llvm
On Sun, Jan 8, 2023 at 3:38 PM Tom Rix <trix@redhat.com> wrote:
>
> clang static analysis reports this problem
> drivers/net/ethernet/amazon/ena/ena_netdev.c:1821:2: warning: Passed-by-value struct
> argument contains uninitialized data (e.g., field: 'comp_ctr') [core.CallAndMessage]
> net_dim(&ena_napi->dim, dim_sample);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> net_dim can call dim_calc_stats() which uses the comp_ctr element,
> so it must be initialized.
This seems to be a dim_update_sample() problem really, when comp_ctr
has been added...
Your patch works, but we could avoid pre-initializing dim_sample in all callers,
then re-writing all but one field...
diff --git a/include/linux/dim.h b/include/linux/dim.h
index 6c5733981563eadf5f06c59c5dc97df961692b02..4604ced4517268ef8912cd8053ac8f4d2630f977
100644
--- a/include/linux/dim.h
+++ b/include/linux/dim.h
@@ -254,6 +254,7 @@ dim_update_sample(u16 event_ctr, u64 packets, u64
bytes, struct dim_sample *s)
s->pkt_ctr = packets;
s->byte_ctr = bytes;
s->event_ctr = event_ctr;
+ s->comp_ctr = 0;
}
/**
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ena: initialize dim_sample
2023-01-08 14:38 [PATCH] net: ena: initialize dim_sample Tom Rix
2023-01-09 10:34 ` Eric Dumazet
@ 2023-01-10 16:44 ` Jiri Pirko
1 sibling, 0 replies; 7+ messages in thread
From: Jiri Pirko @ 2023-01-10 16:44 UTC (permalink / raw)
To: Tom Rix
Cc: shayagr, akiyano, darinzon, ndagan, saeedb, davem, edumazet,
kuba, pabeni, nathan, ndesaulniers, khalasa, wsa+renesas,
yuancan, tglx, 42.hyeyoo, netdev, linux-kernel, llvm
Sun, Jan 08, 2023 at 03:38:43PM CET, trix@redhat.com wrote:
>clang static analysis reports this problem
>drivers/net/ethernet/amazon/ena/ena_netdev.c:1821:2: warning: Passed-by-value struct
> argument contains uninitialized data (e.g., field: 'comp_ctr') [core.CallAndMessage]
> net_dim(&ena_napi->dim, dim_sample);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>net_dim can call dim_calc_stats() which uses the comp_ctr element,
>so it must be initialized.
>
>Fixes: 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive interrupt moderation")
>Signed-off-by: Tom Rix <trix@redhat.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ena: initialize dim_sample
2023-01-09 10:34 ` Eric Dumazet
@ 2023-01-10 16:58 ` Shay Agroskin
2023-01-10 17:17 ` Tom Rix
0 siblings, 1 reply; 7+ messages in thread
From: Shay Agroskin @ 2023-01-10 16:58 UTC (permalink / raw)
To: Eric Dumazet
Cc: Tom Rix, akiyano, darinzon, ndagan, saeedb, davem, kuba, pabeni,
nathan, ndesaulniers, khalasa, wsa+renesas, yuancan, tglx,
42.hyeyoo, netdev, linux-kernel, llvm
Eric Dumazet <edumazet@google.com> writes:
> On Sun, Jan 8, 2023 at 3:38 PM Tom Rix <trix@redhat.com> wrote:
>>
>> clang static analysis reports this problem
>> drivers/net/ethernet/amazon/ena/ena_netdev.c:1821:2: warning:
>> Passed-by-value struct
>> argument contains uninitialized data (e.g., field:
>> 'comp_ctr') [core.CallAndMessage]
>> net_dim(&ena_napi->dim, dim_sample);
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> net_dim can call dim_calc_stats() which uses the comp_ctr
>> element,
>> so it must be initialized.
>
> This seems to be a dim_update_sample() problem really, when
> comp_ctr
> has been added...
>
> Your patch works, but we could avoid pre-initializing dim_sample
> in all callers,
> then re-writing all but one field...
>
> diff --git a/include/linux/dim.h b/include/linux/dim.h
> index
> 6c5733981563eadf5f06c59c5dc97df961692b02..4604ced4517268ef8912cd8053ac8f4d2630f977
> 100644
> --- a/include/linux/dim.h
> +++ b/include/linux/dim.h
> @@ -254,6 +254,7 @@ dim_update_sample(u16 event_ctr, u64
> packets, u64
> bytes, struct dim_sample *s)
> s->pkt_ctr = packets;
> s->byte_ctr = bytes;
> s->event_ctr = event_ctr;
> + s->comp_ctr = 0;
> }
>
> /**
Hi,
I'd rather go with Eric's solution to this issue than zero the
whole struct in ENA
Thanks,
Shay
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ena: initialize dim_sample
2023-01-10 16:58 ` Shay Agroskin
@ 2023-01-10 17:17 ` Tom Rix
2023-01-11 8:46 ` Shay Agroskin
0 siblings, 1 reply; 7+ messages in thread
From: Tom Rix @ 2023-01-10 17:17 UTC (permalink / raw)
To: Shay Agroskin, Eric Dumazet
Cc: akiyano, darinzon, ndagan, saeedb, davem, kuba, pabeni, nathan,
ndesaulniers, khalasa, wsa+renesas, yuancan, tglx, 42.hyeyoo,
netdev, linux-kernel, llvm
On 1/10/23 8:58 AM, Shay Agroskin wrote:
>
> Eric Dumazet <edumazet@google.com> writes:
>
>> On Sun, Jan 8, 2023 at 3:38 PM Tom Rix <trix@redhat.com> wrote:
>>>
>>> clang static analysis reports this problem
>>> drivers/net/ethernet/amazon/ena/ena_netdev.c:1821:2: warning:
>>> Passed-by-value struct
>>> argument contains uninitialized data (e.g., field: 'comp_ctr')
>>> [core.CallAndMessage]
>>> net_dim(&ena_napi->dim, dim_sample);
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> net_dim can call dim_calc_stats() which uses the comp_ctr element,
>>> so it must be initialized.
>>
>> This seems to be a dim_update_sample() problem really, when comp_ctr
>> has been added...
>>
>> Your patch works, but we could avoid pre-initializing dim_sample in
>> all callers,
>> then re-writing all but one field...
>>
>> diff --git a/include/linux/dim.h b/include/linux/dim.h
>> index
>> 6c5733981563eadf5f06c59c5dc97df961692b02..4604ced4517268ef8912cd8053ac8f4d2630f977
>> 100644
>> --- a/include/linux/dim.h
>> +++ b/include/linux/dim.h
>> @@ -254,6 +254,7 @@ dim_update_sample(u16 event_ctr, u64 packets, u64
>> bytes, struct dim_sample *s)
>> s->pkt_ctr = packets;
>> s->byte_ctr = bytes;
>> s->event_ctr = event_ctr;
>> + s->comp_ctr = 0;
>> }
>>
>> /**
>
> Hi,
>
> I'd rather go with Eric's solution to this issue than zero the whole
> struct in ENA
Please look at the other callers of dim_update_sample. The common
pattern is to initialize the struct.
This alternative will work, but the pattern of initializing the struct
the other (~20) callers should be refactored.
Tom
>
> Thanks,
> Shay
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ena: initialize dim_sample
2023-01-10 17:17 ` Tom Rix
@ 2023-01-11 8:46 ` Shay Agroskin
2023-01-11 14:29 ` Tom Rix
0 siblings, 1 reply; 7+ messages in thread
From: Shay Agroskin @ 2023-01-11 8:46 UTC (permalink / raw)
To: Tom Rix
Cc: Eric Dumazet, akiyano, darinzon, ndagan, saeedb, davem, kuba,
pabeni, nathan, ndesaulniers, khalasa, wsa+renesas, yuancan,
tglx, 42.hyeyoo, netdev, linux-kernel, llvm
Tom Rix <trix@redhat.com> writes:
> On 1/10/23 8:58 AM, Shay Agroskin wrote:
>>
>> Eric Dumazet <edumazet@google.com> writes:
>>
>>> On Sun, Jan 8, 2023 at 3:38 PM Tom Rix <trix@redhat.com>
>>> wrote:
>>>>
>>>> clang static analysis reports this problem
>>>> drivers/net/ethernet/amazon/ena/ena_netdev.c:1821:2: warning:
>>>> Passed-by-value struct
>>>> argument contains uninitialized data (e.g., field:
>>>> 'comp_ctr')
>>>> [core.CallAndMessage]
>>>> net_dim(&ena_napi->dim, dim_sample);
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> net_dim can call dim_calc_stats() which uses the comp_ctr
>>>> element,
>>>> so it must be initialized.
>>>
>>> This seems to be a dim_update_sample() problem really, when
>>> comp_ctr
>>> has been added...
>>>
>>> Your patch works, but we could avoid pre-initializing
>>> dim_sample in
>>> all callers,
>>> then re-writing all but one field...
>>>
>>> diff --git a/include/linux/dim.h b/include/linux/dim.h
>>> index
>>> 6c5733981563eadf5f06c59c5dc97df961692b02..4604ced4517268ef8912cd8053ac8f4d2630f977
>>> 100644
>>> --- a/include/linux/dim.h
>>> +++ b/include/linux/dim.h
>>> @@ -254,6 +254,7 @@ dim_update_sample(u16 event_ctr, u64
>>> packets, u64
>>> bytes, struct dim_sample *s)
>>> s->pkt_ctr = packets;
>>> s->byte_ctr = bytes;
>>> s->event_ctr = event_ctr;
>>> + s->comp_ctr = 0;
>>> }
>>>
>>> /**
>>
>> Hi,
>>
>> I'd rather go with Eric's solution to this issue than zero the
>> whole
>> struct in ENA
>
> Please look at the other callers of dim_update_sample. The
> common
> pattern is to initialize the struct.
>
> This alternative will work, but the pattern of initializing the
> struct
> the other (~20) callers should be refactored.
>
> Tom
>
While Eric's patch might be bigger if you also remove the
pre-initialization in the other drivers, the Linux code itself
would be smaller (granted not significantly) and
it make less room for pitfalls in adding DIM support in other
drivers.
Is there a good argument against using Eric's patch other than
'the other patch would be bigger' ?
Shay
>>
>> Thanks,
>> Shay
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ena: initialize dim_sample
2023-01-11 8:46 ` Shay Agroskin
@ 2023-01-11 14:29 ` Tom Rix
0 siblings, 0 replies; 7+ messages in thread
From: Tom Rix @ 2023-01-11 14:29 UTC (permalink / raw)
To: Shay Agroskin
Cc: Eric Dumazet, akiyano, darinzon, ndagan, saeedb, davem, kuba,
pabeni, nathan, ndesaulniers, khalasa, wsa+renesas, yuancan,
tglx, 42.hyeyoo, netdev, linux-kernel, llvm
On 1/11/23 12:46 AM, Shay Agroskin wrote:
>
> Tom Rix <trix@redhat.com> writes:
>
>> On 1/10/23 8:58 AM, Shay Agroskin wrote:
>>>
>>> Eric Dumazet <edumazet@google.com> writes:
>>>
>>>> On Sun, Jan 8, 2023 at 3:38 PM Tom Rix <trix@redhat.com> wrote:
>>>>>
>>>>> clang static analysis reports this problem
>>>>> drivers/net/ethernet/amazon/ena/ena_netdev.c:1821:2: warning:
>>>>> Passed-by-value struct
>>>>> argument contains uninitialized data (e.g., field: 'comp_ctr')
>>>>> [core.CallAndMessage]
>>>>> net_dim(&ena_napi->dim, dim_sample);
>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> net_dim can call dim_calc_stats() which uses the comp_ctr element,
>>>>> so it must be initialized.
>>>>
>>>> This seems to be a dim_update_sample() problem really, when comp_ctr
>>>> has been added...
>>>>
>>>> Your patch works, but we could avoid pre-initializing dim_sample in
>>>> all callers,
>>>> then re-writing all but one field...
>>>>
>>>> diff --git a/include/linux/dim.h b/include/linux/dim.h
>>>> index
>>>> 6c5733981563eadf5f06c59c5dc97df961692b02..4604ced4517268ef8912cd8053ac8f4d2630f977
>>>>
>>>> 100644
>>>> --- a/include/linux/dim.h
>>>> +++ b/include/linux/dim.h
>>>> @@ -254,6 +254,7 @@ dim_update_sample(u16 event_ctr, u64 packets, u64
>>>> bytes, struct dim_sample *s)
>>>> s->pkt_ctr = packets;
>>>> s->byte_ctr = bytes;
>>>> s->event_ctr = event_ctr;
>>>> + s->comp_ctr = 0;
>>>> }
>>>>
>>>> /**
>>>
>>> Hi,
>>>
>>> I'd rather go with Eric's solution to this issue than zero the whole
>>> struct in ENA
>>
>> Please look at the other callers of dim_update_sample. The common
>> pattern is to initialize the struct.
>>
>> This alternative will work, but the pattern of initializing the struct
>> the other (~20) callers should be refactored.
>>
>> Tom
>>
>
> While Eric's patch might be bigger if you also remove the
> pre-initialization in the other drivers, the Linux code itself would
> be smaller (granted not significantly) and
> it make less room for pitfalls in adding DIM support in other drivers.
>
> Is there a good argument against using Eric's patch other than 'the
> other patch would be bigger' ?
No, I think it a better approach and if Eric can take it forward that
would be great.
However when you start refactoring, it may grow larger than the single fix.
For instance, passing the structure by value could be changed to passing
by reference.
Tom
>
> Shay
>
>>>
>>> Thanks,
>>> Shay
>>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-01-11 14:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-08 14:38 [PATCH] net: ena: initialize dim_sample Tom Rix
2023-01-09 10:34 ` Eric Dumazet
2023-01-10 16:58 ` Shay Agroskin
2023-01-10 17:17 ` Tom Rix
2023-01-11 8:46 ` Shay Agroskin
2023-01-11 14:29 ` Tom Rix
2023-01-10 16:44 ` Jiri Pirko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).