If user has explicitly requested the number of {R,T}XQs, then it is
unnecessary to get the count of already available {R,T}XQs from the
PF avail_{r,t}xqs bitmap. This value will get overridden by user specified
value in any case.
This patch does minor re-organization of the code for improving the flow
and readabiltiy. This scope of improvement was found during the review of
the ICE driver code.
FYI, I could not test this change due to unavailability of the hardware.
It would be helpful if somebody can test this patch and provide Tested-by
Tag. Many thanks!
Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
Cc: [email protected]
Cc: Jeff Kirsher <[email protected]>
Signed-off-by: Salil Mehta <[email protected]>
--
Change V1->V2
(*) Fixed the comments from Anthony Nguyen(Intel)
Link: https://lkml.org/lkml/2021/4/12/1997
---
drivers/net/ethernet/intel/ice/ice_lib.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index d13c7fc8fb0a..d77133d6baa7 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -161,12 +161,13 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
switch (vsi->type) {
case ICE_VSI_PF:
- vsi->alloc_txq = min3(pf->num_lan_msix,
- ice_get_avail_txq_count(pf),
- (u16)num_online_cpus());
if (vsi->req_txq) {
vsi->alloc_txq = vsi->req_txq;
vsi->num_txq = vsi->req_txq;
+ } else {
+ vsi->alloc_txq = min3(pf->num_lan_msix,
+ ice_get_avail_txq_count(pf),
+ (u16)num_online_cpus());
}
pf->num_lan_tx = vsi->alloc_txq;
@@ -175,12 +176,13 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags)) {
vsi->alloc_rxq = 1;
} else {
- vsi->alloc_rxq = min3(pf->num_lan_msix,
- ice_get_avail_rxq_count(pf),
- (u16)num_online_cpus());
if (vsi->req_rxq) {
vsi->alloc_rxq = vsi->req_rxq;
vsi->num_rxq = vsi->req_rxq;
+ } else {
+ vsi->alloc_rxq = min3(pf->num_lan_msix,
+ ice_get_avail_rxq_count(pf),
+ (u16)num_online_cpus());
}
}
--
2.17.1
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of
> Salil Mehta
> Sent: Tuesday, April 13, 2021 3:45 PM
> To: [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Jeff Kirsher <[email protected]>; intel-
> [email protected]
> Subject: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R,
> T}XQ check/code for efficiency+readability
>
> If user has explicitly requested the number of {R,T}XQs, then it is
> unnecessary to get the count of already available {R,T}XQs from the PF
> avail_{r,t}xqs bitmap. This value will get overridden by user specified value in
> any case.
>
> This patch does minor re-organization of the code for improving the flow and
> readabiltiy. This scope of improvement was found during the review of the
> ICE driver code.
>
> FYI, I could not test this change due to unavailability of the hardware.
> It would be helpful if somebody can test this patch and provide Tested-by
> Tag. Many thanks!
>
> Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
> Cc: [email protected]
> Cc: Jeff Kirsher <[email protected]>
> Signed-off-by: Salil Mehta <[email protected]>
> --
> Change V1->V2
> (*) Fixed the comments from Anthony Nguyen(Intel)
> Link: https://lkml.org/lkml/2021/4/12/1997
> ---
> drivers/net/ethernet/intel/ice/ice_lib.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
Tested-by: Tony Brelinski <[email protected]> (A Contingent Worker at Intel)
> From: Brelinski, TonyX [mailto:[email protected]]
> Sent: Tuesday, April 20, 2021 9:26 PM
>
> > From: Intel-wired-lan <[email protected]> On Behalf Of
> > Salil Mehta
> > Sent: Tuesday, April 13, 2021 3:45 PM
> > To: [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; Jeff Kirsher <[email protected]>; intel-
> > [email protected]
> > Subject: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R,
> > T}XQ check/code for efficiency+readability
> >
> > If user has explicitly requested the number of {R,T}XQs, then it is
> > unnecessary to get the count of already available {R,T}XQs from the PF
> > avail_{r,t}xqs bitmap. This value will get overridden by user specified value
> in
> > any case.
> >
> > This patch does minor re-organization of the code for improving the flow and
> > readabiltiy. This scope of improvement was found during the review of the
> > ICE driver code.
> >
> > FYI, I could not test this change due to unavailability of the hardware.
> > It would be helpful if somebody can test this patch and provide Tested-by
> > Tag. Many thanks!
> >
> > Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
> > Cc: [email protected]
> > Cc: Jeff Kirsher <[email protected]>
> > Signed-off-by: Salil Mehta <[email protected]>
> > --
> > Change V1->V2
> > (*) Fixed the comments from Anthony Nguyen(Intel)
> > Link: https://lkml.org/lkml/2021/4/12/1997
> > ---
> > drivers/net/ethernet/intel/ice/ice_lib.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
>
> Tested-by: Tony Brelinski <[email protected]> (A Contingent Worker at
> Intel)
Many thanks!
Salil.
Dear Salil,
Thank you very much for your patch.
In the git commit message summary, could you please use imperative mood [1]?
> Re-organize reqstd/avail {R, T}XQ check/code for efficiency+readability
It’s a bit long though. Maybe:
> Avoid unnecessary assignment with user specified {R,T}XQs
Am 14.04.21 um 00:44 schrieb Salil Mehta:
> If user has explicitly requested the number of {R,T}XQs, then it is
> unnecessary to get the count of already available {R,T}XQs from the
> PF avail_{r,t}xqs bitmap. This value will get overridden by user specified
> value in any case.
>
> This patch does minor re-organization of the code for improving the flow
> and readabiltiy. This scope of improvement was found during the review of
readabil*it*y
> the ICE driver code.
>
> FYI, I could not test this change due to unavailability of the hardware.
> It would be helpful if somebody can test this patch and provide Tested-by
> Tag. Many thanks!
This should go outside the commit message (below the --- for example).
> Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
Did you check the behavior before is actually a bug? Or is it just for
the detection heuristic for commits to be applied to the stable series?
> Cc: [email protected]
> Cc: Jeff Kirsher <[email protected]>
> Signed-off-by: Salil Mehta <[email protected]>
> --
> Change V1->V2
> (*) Fixed the comments from Anthony Nguyen(Intel)
> Link: https://lkml.org/lkml/2021/4/12/1997
> ---
> drivers/net/ethernet/intel/ice/ice_lib.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index d13c7fc8fb0a..d77133d6baa7 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -161,12 +161,13 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
>
> switch (vsi->type) {
> case ICE_VSI_PF:
> - vsi->alloc_txq = min3(pf->num_lan_msix,
> - ice_get_avail_txq_count(pf),
> - (u16)num_online_cpus());
> if (vsi->req_txq) {
> vsi->alloc_txq = vsi->req_txq;
> vsi->num_txq = vsi->req_txq;
> + } else {
> + vsi->alloc_txq = min3(pf->num_lan_msix,
> + ice_get_avail_txq_count(pf),
> + (u16)num_online_cpus());
> }
I am curious, did you check the compiler actually creates different
code, or did it notice the inefficiency by itself and optimized it already?
>
> pf->num_lan_tx = vsi->alloc_txq;
> @@ -175,12 +176,13 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
> if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags)) {
> vsi->alloc_rxq = 1;
> } else {
> - vsi->alloc_rxq = min3(pf->num_lan_msix,
> - ice_get_avail_rxq_count(pf),
> - (u16)num_online_cpus());
> if (vsi->req_rxq) {
> vsi->alloc_rxq = vsi->req_rxq;
> vsi->num_rxq = vsi->req_rxq;
> + } else {
> + vsi->alloc_rxq = min3(pf->num_lan_msix,
> + ice_get_avail_rxq_count(pf),
> + (u16)num_online_cpus());
> }
> }
>
Kind regards,
Paul
> From: Paul Menzel [mailto:[email protected]]
> Sent: Wednesday, April 21, 2021 6:36 AM
> To: Salil Mehta <[email protected]>
> Cc: [email protected]; [email protected]; Linuxarm
> <[email protected]>; [email protected]; Jeff Kirsher
> <[email protected]>; [email protected]; David S.
> Miller <[email protected]>; Jakub Kicinski <[email protected]>
> Subject: Re: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail
> {R, T}XQ check/code for efficiency+readability
>
> Dear Salil,
>
>
> Thank you very much for your patch.
Thanks for the review.
> In the git commit message summary, could you please use imperative mood [1]?
No issues. There is always a scope of improvement.
> > Re-organize reqstd/avail {R, T}XQ check/code for efficiency+readability
>
> It’s a bit long though. Maybe:
>
> Avoid unnecessary assignment with user specified {R,T}XQs
Umm..above conveys the wrong meaning as this is not what patch is doing.
If you see the code, in the presence of the user specified {R,T}XQs it
avoids fetching available {R,T}XQ count.
What about below?
"Avoid unnecessary avail_{r,t}xq assignments if user has specified Qs"
> Am 14.04.21 um 00:44 schrieb Salil Mehta:
> > If user has explicitly requested the number of {R,T}XQs, then it is
> > unnecessary to get the count of already available {R,T}XQs from the
> > PF avail_{r,t}xqs bitmap. This value will get overridden by user specified
> > value in any case.
> >
> > This patch does minor re-organization of the code for improving the flow
> > and readabiltiy. This scope of improvement was found during the review of
>
> readabil*it*y
Thanks. Missed that earlier. My shaky fingers :(
> > the ICE driver code.
> >
> > FYI, I could not test this change due to unavailability of the hardware.
> > It would be helpful if somebody can test this patch and provide Tested-by
> > Tag. Many thanks!
>
> This should go outside the commit message (below the --- for example).
Agreed.
> > Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
>
> Did you check the behavior before is actually a bug? Or is it just for
> the detection heuristic for commits to be applied to the stable series?
Right, later was the idea.
> > Cc: [email protected]
> > Cc: Jeff Kirsher <[email protected]>
> > Signed-off-by: Salil Mehta <[email protected]>
> > --
> > Change V1->V2
> > (*) Fixed the comments from Anthony Nguyen(Intel)
> > Link: https://lkml.org/lkml/2021/4/12/1997
> > ---
> > drivers/net/ethernet/intel/ice/ice_lib.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
> b/drivers/net/ethernet/intel/ice/ice_lib.c
> > index d13c7fc8fb0a..d77133d6baa7 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > @@ -161,12 +161,13 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16
> vf_id)
> >
> > switch (vsi->type) {
> > case ICE_VSI_PF:
> > - vsi->alloc_txq = min3(pf->num_lan_msix,
> > - ice_get_avail_txq_count(pf),
> > - (u16)num_online_cpus());
> > if (vsi->req_txq) {
> > vsi->alloc_txq = vsi->req_txq;
> > vsi->num_txq = vsi->req_txq;
> > + } else {
> > + vsi->alloc_txq = min3(pf->num_lan_msix,
> > + ice_get_avail_txq_count(pf),
> > + (u16)num_online_cpus());
> > }
>
> I am curious, did you check the compiler actually creates different
> code, or did it notice the inefficiency by itself and optimized it already?
I have not looked into that detail but irrespective of what compiler generates
I would like to keep the code in a shape which is more efficient and more readable.
I do understand in certain cases we have to do tradeoff between efficiency
and readability but I do not see that here.
> > pf->num_lan_tx = vsi->alloc_txq;
> > @@ -175,12 +176,13 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16
> vf_id)
> > if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags)) {
> > vsi->alloc_rxq = 1;
> > } else {
> > - vsi->alloc_rxq = min3(pf->num_lan_msix,
> > - ice_get_avail_rxq_count(pf),
> > - (u16)num_online_cpus());
> > if (vsi->req_rxq) {
> > vsi->alloc_rxq = vsi->req_rxq;
> > vsi->num_rxq = vsi->req_rxq;
> > + } else {
> > + vsi->alloc_rxq = min3(pf->num_lan_msix,
> > + ice_get_avail_rxq_count(pf),
> > + (u16)num_online_cpus());
> > }
> > }
> >
>
>
> Kind regards,
>
> Paul
[CC: Remove Jeff, as email is rejected]
Dear Salil,
Am 21.04.21 um 09:41 schrieb Salil Mehta:
>> From: Paul Menzel [mailto:[email protected]]
>> Sent: Wednesday, April 21, 2021 6:36 AM
[…]
>> In the git commit message summary, could you please use imperative mood [1]?
>
> No issues. There is always a scope of improvement.
>
>>> Re-organize reqstd/avail {R, T}XQ check/code for efficiency+readability
>>
>> It’s a bit long though. Maybe:
>>
>> Avoid unnecessary assignment with user specified {R,T}XQs
>
> Umm..above conveys the wrong meaning as this is not what patch is doing.
>
> If you see the code, in the presence of the user specified {R,T}XQs it
> avoids fetching available {R,T}XQ count.
>
> What about below?
>
> "Avoid unnecessary avail_{r,t}xq assignments if user has specified Qs"
Sounds good, still a little long. Maybe:
> Avoid unnecessary avail_{r,t}xq assignments with user specified Qs
>> Am 14.04.21 um 00:44 schrieb Salil Mehta:
>>> If user has explicitly requested the number of {R,T}XQs, then it is
>>> unnecessary to get the count of already available {R,T}XQs from the
>>> PF avail_{r,t}xqs bitmap. This value will get overridden by user specified
>>> value in any case.
>>>
>>> This patch does minor re-organization of the code for improving the flow
>>> and readabiltiy. This scope of improvement was found during the review of
>>
>> readabil*it*y
>
> Thanks. Missed that earlier. My shaky fingers :(
>
>>> the ICE driver code.
>>>
>>> FYI, I could not test this change due to unavailability of the hardware.
>>> It would be helpful if somebody can test this patch and provide Tested-by
>>> Tag. Many thanks!
>>
>> This should go outside the commit message (below the --- for example).
>
> Agreed.
>
>>> Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
>>
>> Did you check the behavior before is actually a bug? Or is it just for
>> the detection heuristic for commits to be applied to the stable series?
>
> Right, later was the idea.
>
>>> Cc: [email protected]
>>> Cc: Jeff Kirsher <[email protected]>
>>> Signed-off-by: Salil Mehta <[email protected]>
>>> --
>>> Change V1->V2
>>> (*) Fixed the comments from Anthony Nguyen(Intel)
>>> Link: https://lkml.org/lkml/2021/4/12/1997
>>> ---
>>> drivers/net/ethernet/intel/ice/ice_lib.c | 14 ++++++++------
>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> index d13c7fc8fb0a..d77133d6baa7 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> @@ -161,12 +161,13 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
>>>
>>> switch (vsi->type) {
>>> case ICE_VSI_PF:
>>> - vsi->alloc_txq = min3(pf->num_lan_msix,
>>> - ice_get_avail_txq_count(pf),
>>> - (u16)num_online_cpus());
>>> if (vsi->req_txq) {
>>> vsi->alloc_txq = vsi->req_txq;
>>> vsi->num_txq = vsi->req_txq;
>>> + } else {
>>> + vsi->alloc_txq = min3(pf->num_lan_msix,
>>> + ice_get_avail_txq_count(pf),
>>> + (u16)num_online_cpus());
>>> }
>>
>> I am curious, did you check the compiler actually creates different
>> code, or did it notice the inefficiency by itself and optimized it already?
>
> I have not looked into that detail but irrespective of what compiler generates
> I would like to keep the code in a shape which is more efficient and more readable.
>
> I do understand in certain cases we have to do tradeoff between efficiency
> and readability but I do not see that here.
I agree, as *efficiency* is mentioned several times, I assume it was
tested. Thank you for the clarification.
>>> pf->num_lan_tx = vsi->alloc_txq;
>>> @@ -175,12 +176,13 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
>>> if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags)) {
>>> vsi->alloc_rxq = 1;
>>> } else {
>>> - vsi->alloc_rxq = min3(pf->num_lan_msix,
>>> - ice_get_avail_rxq_count(pf),
>>> - (u16)num_online_cpus());
>>> if (vsi->req_rxq) {
>>> vsi->alloc_rxq = vsi->req_rxq;
>>> vsi->num_rxq = vsi->req_rxq;
>>> + } else {
>>> + vsi->alloc_rxq = min3(pf->num_lan_msix,
>>> + ice_get_avail_rxq_count(pf),
>>> + (u16)num_online_cpus());
>>> }
>>> }
>>>
Kind regards,
Paul