Colin King <[email protected]> writes:
> From: Colin Ian King <[email protected]>
>
> The pointer primary_channel is being assigned with a value that is never,
> The assignment is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/hv/channel_mgmt.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>
> if (channel->primary_channel == NULL) {
> list_del(&channel->listentry);
> -
> - primary_channel = channel;
> } else {
> primary_channel = channel->primary_channel;
> spin_lock_irqsave(&primary_channel->lock, flags);
If I'm looking at the right source (5.7-rc1) it *is* beeing used:
if (channel->primary_channel == NULL) {
list_del(&channel->listentry);
primary_channel = channel;
} else {
primary_channel = channel->primary_channel;
spin_lock_irqsave(&primary_channel->lock, flags);
list_del(&channel->sc_list);
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
/*
* We need to free the bit for init_vp_index() to work in the case
* of sub-channel, when we reload drivers like hv_netvsc.
*/
if (channel->affinity_policy == HV_LOCALIZED)
cpumask_clear_cpu(channel->target_cpu,
&primary_channel->alloced_cpus_in_node);
^^^^^ HERE ^^^^^
--
Vitaly
On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
> Colin King <[email protected]> writes:
>
>> From: Colin Ian King <[email protected]>
>>
>> The pointer primary_channel is being assigned with a value that is never,
>> The assignment is redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <[email protected]>
>> ---
>> drivers/hv/channel_mgmt.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
>> --- a/drivers/hv/channel_mgmt.c
>> +++ b/drivers/hv/channel_mgmt.c
>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>>
>> if (channel->primary_channel == NULL) {
>> list_del(&channel->listentry);
>> -
>> - primary_channel = channel;
>> } else {
>> primary_channel = channel->primary_channel;
>> spin_lock_irqsave(&primary_channel->lock, flags);
>
> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
>
> if (channel->primary_channel == NULL) {
> list_del(&channel->listentry);
>
> primary_channel = channel;
> } else {
> primary_channel = channel->primary_channel;
> spin_lock_irqsave(&primary_channel->lock, flags);
> list_del(&channel->sc_list);
> spin_unlock_irqrestore(&primary_channel->lock, flags);
> }
>
> /*
> * We need to free the bit for init_vp_index() to work in the case
> * of sub-channel, when we reload drivers like hv_netvsc.
> */
> if (channel->affinity_policy == HV_LOCALIZED)
> cpumask_clear_cpu(channel->target_cpu,
> &primary_channel->alloced_cpus_in_node);
> ^^^^^ HERE ^^^^^
>
I was basing my change on linux-next that has removed a hunk of code:
commit bcefa400900739310e8ef0cb34cbe029c404455c
Author: Andrea Parri (Microsoft) <[email protected]>
Date: Mon Apr 6 02:15:11 2020 +0200
Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
logic
The logic is unused since commit 509879bdb30b8 ("Drivers: hv: Introduce
a policy for controlling channel affinity").
This logic assumes that a channel target_cpu doesn't change during the
lifetime of a channel, but this assumption is incompatible with the new
functionality that allows changing the vCPU a channel will interrupt.
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
Link:
https://lore.kernel.org/r/[email protected]
Reviewed-by: Michael Kelley <[email protected]>
Signed-off-by: Wei Liu <[email protected]>
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 7fb6eb647f14..476592b0bc00 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -433,14 +433,6 @@ void hv_process_channel_removal(struct
vmbus_channel *channel)
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
- /*
- * We need to free the bit for init_vp_index() to work in the case
- * of sub-channel, when we reload drivers like hv_netvsc.
- */
- if (channel->affinity_policy == HV_LOCALIZED)
- cpumask_clear_cpu(channel->target_cpu,
- &primary_channel->alloced_cpus_in_node);
-
Colin Ian King <[email protected]> writes:
> On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
>> Colin King <[email protected]> writes:
>>
>>> From: Colin Ian King <[email protected]>
>>>
>>> The pointer primary_channel is being assigned with a value that is never,
>>> The assignment is redundant and can be removed.
>>>
>>> Addresses-Coverity: ("Unused value")
>>> Signed-off-by: Colin Ian King <[email protected]>
>>> ---
>>> drivers/hv/channel_mgmt.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
>>> --- a/drivers/hv/channel_mgmt.c
>>> +++ b/drivers/hv/channel_mgmt.c
>>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>>>
>>> if (channel->primary_channel == NULL) {
>>> list_del(&channel->listentry);
>>> -
>>> - primary_channel = channel;
>>> } else {
>>> primary_channel = channel->primary_channel;
>>> spin_lock_irqsave(&primary_channel->lock, flags);
>>
>> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
>>
>> if (channel->primary_channel == NULL) {
>> list_del(&channel->listentry);
>>
>> primary_channel = channel;
>> } else {
>> primary_channel = channel->primary_channel;
>> spin_lock_irqsave(&primary_channel->lock, flags);
>> list_del(&channel->sc_list);
>> spin_unlock_irqrestore(&primary_channel->lock, flags);
>> }
>>
>> /*
>> * We need to free the bit for init_vp_index() to work in the case
>> * of sub-channel, when we reload drivers like hv_netvsc.
>> */
>> if (channel->affinity_policy == HV_LOCALIZED)
>> cpumask_clear_cpu(channel->target_cpu,
>> &primary_channel->alloced_cpus_in_node);
>> ^^^^^ HERE ^^^^^
>>
>
> I was basing my change on linux-next that has removed a hunk of code:
>
> commit bcefa400900739310e8ef0cb34cbe029c404455c
> Author: Andrea Parri (Microsoft) <[email protected]>
> Date: Mon Apr 6 02:15:11 2020 +0200
>
> Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
> logic
>
Ah, please add the right 'Fixes:' tag then.
--
Vitaly
On Wed, Apr 15, 2020 at 12:43:08PM +0200, Vitaly Kuznetsov wrote:
> Colin Ian King <[email protected]> writes:
>
> > On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
> >> Colin King <[email protected]> writes:
> >>
> >>> From: Colin Ian King <[email protected]>
> >>>
> >>> The pointer primary_channel is being assigned with a value that is never,
> >>> The assignment is redundant and can be removed.
> >>>
> >>> Addresses-Coverity: ("Unused value")
> >>> Signed-off-by: Colin Ian King <[email protected]>
> >>> ---
> >>> drivers/hv/channel_mgmt.c | 2 --
> >>> 1 file changed, 2 deletions(-)
> >>>
> >>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> >>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> >>> --- a/drivers/hv/channel_mgmt.c
> >>> +++ b/drivers/hv/channel_mgmt.c
> >>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
> >>>
> >>> if (channel->primary_channel == NULL) {
> >>> list_del(&channel->listentry);
> >>> -
> >>> - primary_channel = channel;
> >>> } else {
> >>> primary_channel = channel->primary_channel;
> >>> spin_lock_irqsave(&primary_channel->lock, flags);
> >>
> >> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
> >>
> >> if (channel->primary_channel == NULL) {
> >> list_del(&channel->listentry);
> >>
> >> primary_channel = channel;
> >> } else {
> >> primary_channel = channel->primary_channel;
> >> spin_lock_irqsave(&primary_channel->lock, flags);
> >> list_del(&channel->sc_list);
> >> spin_unlock_irqrestore(&primary_channel->lock, flags);
> >> }
> >>
> >> /*
> >> * We need to free the bit for init_vp_index() to work in the case
> >> * of sub-channel, when we reload drivers like hv_netvsc.
> >> */
> >> if (channel->affinity_policy == HV_LOCALIZED)
> >> cpumask_clear_cpu(channel->target_cpu,
> >> &primary_channel->alloced_cpus_in_node);
> >> ^^^^^ HERE ^^^^^
> >>
> >
> > I was basing my change on linux-next that has removed a hunk of code:
> >
> > commit bcefa400900739310e8ef0cb34cbe029c404455c
> > Author: Andrea Parri (Microsoft) <[email protected]>
> > Date: Mon Apr 6 02:15:11 2020 +0200
> >
> > Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
> > logic
> >
>
> Ah, please add the right 'Fixes:' tag then.
I don't think the Fixes tag is particularly useful in this instance.
Andrea's commit is not yet in Linus' tree. If I rebase hyper-next over
the next 2.5 months the tag is going to have a stale commit hash in it.
Wei.
>
> --
> Vitaly
>
We have this discussion over and over. I always say it helps to have
the commit mentioned in the commit message but it's not a Fixes tag.
So I think that the commit message should say something like
"commit 1234234 ("blah blah") removed some code so this variable isn't
used any more". I think it helps the review process. But then if we
mention the commit everyone says to use the Fixes tag.
It turns out if you leave out the commit entirely then people still
complain but a lot less frequently. It shouldn't work that way but
reviewers are illogical.
regards,
dan carpenter
On 16/04/2020 11:02, Dan Carpenter wrote:
> We have this discussion over and over. I always say it helps to have
> the commit mentioned in the commit message but it's not a Fixes tag.
> So I think that the commit message should say something like
> "commit 1234234 ("blah blah") removed some code so this variable isn't
> used any more". I think it helps the review process. But then if we
> mention the commit everyone says to use the Fixes tag.
>
> It turns out if you leave out the commit entirely then people still
> complain but a lot less frequently. It shouldn't work that way but
> reviewers are illogical.
Yep, to my knowledge the policy for this kind of commit is not described
in any documentation, so it's ambiguous. Either way, one can't win
without it being properly codified.
Colin
>
> regards,
> dan carpenter
>