* [PATCH V2 net] ice: Re-organizes reqstd/avail {R,T}XQ check/code for efficiency+readability
@ 2021-04-13 22:44 Salil Mehta
2021-04-20 20:25 ` [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ " Brelinski, TonyX
2021-04-21 5:35 ` Paul Menzel
0 siblings, 2 replies; 7+ messages in thread
From: Salil Mehta @ 2021-04-13 22:44 UTC (permalink / raw)
To: davem, kuba
Cc: salil.mehta, jesse.brandeburg, anthony.l.nguyen, henry.w.tieman,
netdev, linux-kernel, linuxarm, linuxarm, intel-wired-lan,
Jeff Kirsher
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: intel-wired-lan@lists.osuosl.org
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
--
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ check/code for efficiency+readability
2021-04-13 22:44 [PATCH V2 net] ice: Re-organizes reqstd/avail {R,T}XQ check/code for efficiency+readability Salil Mehta
@ 2021-04-20 20:25 ` Brelinski, TonyX
2021-04-20 21:28 ` Salil Mehta
2021-04-21 5:35 ` Paul Menzel
1 sibling, 1 reply; 7+ messages in thread
From: Brelinski, TonyX @ 2021-04-20 20:25 UTC (permalink / raw)
To: Salil Mehta, davem, kuba
Cc: linuxarm, netdev, linuxarm, linux-kernel, Jeff Kirsher, intel-wired-lan
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Salil Mehta
> Sent: Tuesday, April 13, 2021 3:45 PM
> To: davem@davemloft.net; kuba@kernel.org
> Cc: salil.mehta@huawei.com; linuxarm@openeuler.org;
> netdev@vger.kernel.org; linuxarm@huawei.com; linux-
> kernel@vger.kernel.org; Jeff Kirsher <jeffrey.t.kirsher@intel.com>; intel-
> wired-lan@lists.osuosl.org
> 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: intel-wired-lan@lists.osuosl.org
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> --
> 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 <tonyx.brelinski@intel.com> (A Contingent Worker at Intel)
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ check/code for efficiency+readability
2021-04-20 20:25 ` [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ " Brelinski, TonyX
@ 2021-04-20 21:28 ` Salil Mehta
0 siblings, 0 replies; 7+ messages in thread
From: Salil Mehta @ 2021-04-20 21:28 UTC (permalink / raw)
To: Brelinski, TonyX, davem, kuba
Cc: linuxarm, netdev, Linuxarm, linux-kernel, Jeff Kirsher, intel-wired-lan
> From: Brelinski, TonyX [mailto:tonyx.brelinski@intel.com]
> Sent: Tuesday, April 20, 2021 9:26 PM
>
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Salil Mehta
> > Sent: Tuesday, April 13, 2021 3:45 PM
> > To: davem@davemloft.net; kuba@kernel.org
> > Cc: salil.mehta@huawei.com; linuxarm@openeuler.org;
> > netdev@vger.kernel.org; linuxarm@huawei.com; linux-
> > kernel@vger.kernel.org; Jeff Kirsher <jeffrey.t.kirsher@intel.com>; intel-
> > wired-lan@lists.osuosl.org
> > 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: intel-wired-lan@lists.osuosl.org
> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > --
> > 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 <tonyx.brelinski@intel.com> (A Contingent Worker at
> Intel)
Many thanks!
Salil.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ check/code for efficiency+readability
2021-04-13 22:44 [PATCH V2 net] ice: Re-organizes reqstd/avail {R,T}XQ check/code for efficiency+readability Salil Mehta
2021-04-20 20:25 ` [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ " Brelinski, TonyX
@ 2021-04-21 5:35 ` Paul Menzel
2021-04-21 7:41 ` Salil Mehta
1 sibling, 1 reply; 7+ messages in thread
From: Paul Menzel @ 2021-04-21 5:35 UTC (permalink / raw)
To: Salil Mehta
Cc: linuxarm, netdev, linuxarm, linux-kernel, Jeff Kirsher,
intel-wired-lan, David S. Miller, Jakub Kicinski
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: intel-wired-lan@lists.osuosl.org
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> --
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ check/code for efficiency+readability
2021-04-21 5:35 ` Paul Menzel
@ 2021-04-21 7:41 ` Salil Mehta
2021-04-21 7:54 ` Paul Menzel
0 siblings, 1 reply; 7+ messages in thread
From: Salil Mehta @ 2021-04-21 7:41 UTC (permalink / raw)
To: Paul Menzel
Cc: linuxarm, netdev, Linuxarm, linux-kernel, Jeff Kirsher,
intel-wired-lan, David S. Miller, Jakub Kicinski
> From: Paul Menzel [mailto:pmenzel@molgen.mpg.de]
> Sent: Wednesday, April 21, 2021 6:36 AM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: linuxarm@openeuler.org; netdev@vger.kernel.org; Linuxarm
> <linuxarm@huawei.com>; linux-kernel@vger.kernel.org; Jeff Kirsher
> <jeffrey.t.kirsher@intel.com>; intel-wired-lan@lists.osuosl.org; David S.
> Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> 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: intel-wired-lan@lists.osuosl.org
> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > --
> > 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ check/code for efficiency+readability
2021-04-21 7:41 ` Salil Mehta
@ 2021-04-21 7:54 ` Paul Menzel
2021-04-21 8:08 ` Salil Mehta
0 siblings, 1 reply; 7+ messages in thread
From: Paul Menzel @ 2021-04-21 7:54 UTC (permalink / raw)
To: Salil Mehta
Cc: linuxarm, netdev, linuxarm, linux-kernel, intel-wired-lan,
David S. Miller, Jakub Kicinski
[CC: Remove Jeff, as email is rejected]
Dear Salil,
Am 21.04.21 um 09:41 schrieb Salil Mehta:
>> From: Paul Menzel [mailto:pmenzel@molgen.mpg.de]
>> 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: intel-wired-lan@lists.osuosl.org
>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>>> --
>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ check/code for efficiency+readability
2021-04-21 7:54 ` Paul Menzel
@ 2021-04-21 8:08 ` Salil Mehta
0 siblings, 0 replies; 7+ messages in thread
From: Salil Mehta @ 2021-04-21 8:08 UTC (permalink / raw)
To: Paul Menzel
Cc: linuxarm, netdev, Linuxarm, linux-kernel, intel-wired-lan,
David S. Miller, Jakub Kicinski
> From: Paul Menzel [mailto:pmenzel@molgen.mpg.de]
> Sent: Wednesday, April 21, 2021 8:54 AM
>
> [CC: Remove Jeff, as email is rejected]
Yes, thanks for the reminder. I had noticed it earlier.
[...]
> >>> 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.
I mentioned inefficient because below code gets executed unnecessarily.
/**
* ice_get_avail_q_count - Get count of queues in use
* @pf_qmap: bitmap to get queue use count from
* @lock: pointer to a mutex that protects access to pf_qmap
* @size: size of the bitmap
*/
static u16
ice_get_avail_q_count(unsigned long *pf_qmap, struct mutex *lock, u16 size)
{
unsigned long bit;
u16 count = 0;
mutex_lock(lock);
for_each_clear_bit(bit, pf_qmap, size)
count++;
mutex_unlock(lock);
return count;
}
/**
* ice_get_avail_txq_count - Get count of Tx queues in use
* @pf: pointer to an ice_pf instance
*/
u16 ice_get_avail_txq_count(struct ice_pf *pf)
{
return ice_get_avail_q_count(pf->avail_txqs, &pf->avail_q_mutex,
pf->max_pf_txqs);
}
> >>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-21 8:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 22:44 [PATCH V2 net] ice: Re-organizes reqstd/avail {R,T}XQ check/code for efficiency+readability Salil Mehta
2021-04-20 20:25 ` [Intel-wired-lan] [PATCH V2 net] ice: Re-organizes reqstd/avail {R, T}XQ " Brelinski, TonyX
2021-04-20 21:28 ` Salil Mehta
2021-04-21 5:35 ` Paul Menzel
2021-04-21 7:41 ` Salil Mehta
2021-04-21 7:54 ` Paul Menzel
2021-04-21 8:08 ` Salil Mehta
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).