| Message ID | 1415350670-15333-2-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jeff Kirsher > From: Kamil Krawczyk <kamil.krawczyk@intel.com> > > The code was polling the firmware tail register for completion every > 10 microseconds, which is way faster than the firmware can respond. > This changes the poll interval to 1ms, which reduces polling CPU > utilization, and the number of times we loop. Are you sure the code path is allowed to sleep? > The maximum delay is still 100ms. Actually it is now likely to be up to 200ms or more. You could convert the maximum delay check to one that looks at jiffies - but maybe it doesn't matter. > Change-ID: I4bbfa6b66d802890baf8b4154061e55942b90958 > Signed-off-by: Kamil Krawczyk <kamil.krawczyk@intel.com> > Acked-by: Shannon Nelson <shannon.nelson@intel.com> > Tested-by: Jim Young <jamesx.m.young@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_adminq.c | 5 ++--- > drivers/net/ethernet/intel/i40e/i40e_adminq.h | 2 +- > drivers/net/ethernet/intel/i40evf/i40e_adminq.c | 5 ++--- > drivers/net/ethernet/intel/i40evf/i40e_adminq.h | 2 +- > 4 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c > b/drivers/net/ethernet/intel/i40e/i40e_adminq.c > index 72f5d25..057b7bf 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c > @@ -853,7 +853,6 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw, > */ > if (!details->async && !details->postpone) { > u32 total_delay = 0; > - u32 delay_len = 10; > > do { > /* AQ designers suggest use of head for better > @@ -862,8 +861,8 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw, > if (i40e_asq_done(hw)) > break; > /* ugh! delay while spin_lock */ The comment is not right any more. > - udelay(delay_len); > - total_delay += delay_len; > + usleep_range(1000, 2000); > + total_delay++; > } while (total_delay < hw->aq.asq_cmd_timeout); > } > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.h > b/drivers/net/ethernet/intel/i40e/i40e_adminq.h > index ba38a89..df0bd09 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.h > +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.h > @@ -141,7 +141,7 @@ static inline int i40e_aq_rc_to_posix(u16 aq_rc) > > /* general information */ > #define I40E_AQ_LARGE_BUF 512 > -#define I40E_ASQ_CMD_TIMEOUT 100000 /* usecs */ > +#define I40E_ASQ_CMD_TIMEOUT 100 /* msecs */ It looks like this value is written to asq_cmd_timeout, that makes be wonder whether anything else can change it - otherwise the compile time constant would be used. Changing the units has broken anything else that modifies the value. > > void i40e_fill_default_direct_cmd_desc(struct i40e_aq_desc *desc, > u16 opcode); > diff --git a/drivers/net/ethernet/intel/i40evf/i40e_adminq.c > b/drivers/net/ethernet/intel/i40evf/i40e_adminq.c > index f206be9..25c846b 100644 > --- a/drivers/net/ethernet/intel/i40evf/i40e_adminq.c > +++ b/drivers/net/ethernet/intel/i40evf/i40e_adminq.c > @@ -801,7 +801,6 @@ i40e_status i40evf_asq_send_command(struct i40e_hw *hw, > */ > if (!details->async && !details->postpone) { > u32 total_delay = 0; > - u32 delay_len = 10; > > do { > /* AQ designers suggest use of head for better > @@ -810,8 +809,8 @@ i40e_status i40evf_asq_send_command(struct i40e_hw *hw, > if (i40evf_asq_done(hw)) > break; > /* ugh! delay while spin_lock */ > - udelay(delay_len); > - total_delay += delay_len; > + usleep_range(1000, 2000); > + total_delay++; > } while (total_delay < hw->aq.asq_cmd_timeout); > } > > diff --git a/drivers/net/ethernet/intel/i40evf/i40e_adminq.h > b/drivers/net/ethernet/intel/i40evf/i40e_adminq.h > index 91a5c5b..f40cfac 100644 > --- a/drivers/net/ethernet/intel/i40evf/i40e_adminq.h > +++ b/drivers/net/ethernet/intel/i40evf/i40e_adminq.h > @@ -141,7 +141,7 @@ static inline int i40e_aq_rc_to_posix(u16 aq_rc) > > /* general information */ > #define I40E_AQ_LARGE_BUF 512 > -#define I40E_ASQ_CMD_TIMEOUT 100000 /* usecs */ > +#define I40E_ASQ_CMD_TIMEOUT 100 /* msecs */ > > void i40evf_fill_default_direct_cmd_desc(struct i40e_aq_desc *desc, > u16 opcode); > -- > 1.9.3 Hmmm.... two drivers containing the same code.... David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 7, 2014 at 10:57 AM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> The code was polling the firmware tail register for completion
any reason not to sleep while waiting for this completion? can the
firmware generate an interrupt?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review David, comments follow. On Fri, 7 Nov 2014 09:40:08 +0000 David Laight <David.Laight@ACULAB.COM> wrote: > From: Jeff Kirsher > > From: Kamil Krawczyk <kamil.krawczyk@intel.com> > > > > The code was polling the firmware tail register for completion every > > 10 microseconds, which is way faster than the firmware can respond. > > This changes the poll interval to 1ms, which reduces polling CPU > > utilization, and the number of times we loop. > > Are you sure the code path is allowed to sleep? Yes, we are never (should never be) in interrupt context when calling these routines. > > > The maximum delay is still 100ms. > > Actually it is now likely to be up to 200ms or more. > You could convert the maximum delay check to one that > looks at jiffies - but maybe it doesn't matter. Thats okay, this is all init or reset or shutdown level code. If the delay goes up it won't hurt anything. > > > --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c > > @@ -853,7 +853,6 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw, > > */ > > if (!details->async && !details->postpone) { > > u32 total_delay = 0; > > - u32 delay_len = 10; > > > > do { > > /* AQ designers suggest use of head for better > > @@ -862,8 +861,8 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw, > > if (i40e_asq_done(hw)) > > break; > > /* ugh! delay while spin_lock */ > > The comment is not right any more. yes it should have been removed. > > > - udelay(delay_len); > > - total_delay += delay_len; > > + usleep_range(1000, 2000); > > + total_delay++; > > } while (total_delay < hw->aq.asq_cmd_timeout); > > } > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.h > > b/drivers/net/ethernet/intel/i40e/i40e_adminq.h > > index ba38a89..df0bd09 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.h > > +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.h > > @@ -141,7 +141,7 @@ static inline int i40e_aq_rc_to_posix(u16 aq_rc) > > > > /* general information */ > > #define I40E_AQ_LARGE_BUF 512 > > -#define I40E_ASQ_CMD_TIMEOUT 100000 /* usecs */ > > +#define I40E_ASQ_CMD_TIMEOUT 100 /* msecs */ > > It looks like this value is written to asq_cmd_timeout, that makes > be wonder whether anything else can change it - otherwise the compile > time constant would be used. > Changing the units has broken anything else that modifies the value. I pretty much agree with you, but I can tell you why it's there. Currently nothing in the code changes it. The code was designed such that it can run on hardware requiring different timeouts. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 7 Nov 2014 15:29:15 +0200 Or Gerlitz <gerlitz.or@gmail.com> wrote: > On Fri, Nov 7, 2014 at 10:57 AM, Jeff Kirsher > <jeffrey.t.kirsher@intel.com> wrote: > > The code was polling the firmware tail register for completion > > any reason not to sleep while waiting for this completion? can the > firmware generate an interrupt? The code path in question is called from the drivers when they are in a synchronous context and can sleep or be rescheduled without issue. An async mode is possible, but not here, because the code complexity required to change to async mode is unmaintainable (if even doable, how do you handle an interrupt while you're in probe?). We use async when it is practical. Thanks for your comments, Jesse -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c b/drivers/net/ethernet/intel/i40e/i40e_adminq.c index 72f5d25..057b7bf 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c @@ -853,7 +853,6 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw, */ if (!details->async && !details->postpone) { u32 total_delay = 0; - u32 delay_len = 10; do { /* AQ designers suggest use of head for better @@ -862,8 +861,8 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw, if (i40e_asq_done(hw)) break; /* ugh! delay while spin_lock */ - udelay(delay_len); - total_delay += delay_len; + usleep_range(1000, 2000); + total_delay++; } while (total_delay < hw->aq.asq_cmd_timeout); } diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.h b/drivers/net/ethernet/intel/i40e/i40e_adminq.h index ba38a89..df0bd09 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.h +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.h @@ -141,7 +141,7 @@ static inline int i40e_aq_rc_to_posix(u16 aq_rc) /* general information */ #define I40E_AQ_LARGE_BUF 512 -#define I40E_ASQ_CMD_TIMEOUT 100000 /* usecs */ +#define I40E_ASQ_CMD_TIMEOUT 100 /* msecs */ void i40e_fill_default_direct_cmd_desc(struct i40e_aq_desc *desc, u16 opcode); diff --git a/drivers/net/ethernet/intel/i40evf/i40e_adminq.c b/drivers/net/ethernet/intel/i40evf/i40e_adminq.c index f206be9..25c846b 100644 --- a/drivers/net/ethernet/intel/i40evf/i40e_adminq.c +++ b/drivers/net/ethernet/intel/i40evf/i40e_adminq.c @@ -801,7 +801,6 @@ i40e_status i40evf_asq_send_command(struct i40e_hw *hw, */ if (!details->async && !details->postpone) { u32 total_delay = 0; - u32 delay_len = 10; do { /* AQ designers suggest use of head for better @@ -810,8 +809,8 @@ i40e_status i40evf_asq_send_command(struct i40e_hw *hw, if (i40evf_asq_done(hw)) break; /* ugh! delay while spin_lock */ - udelay(delay_len); - total_delay += delay_len; + usleep_range(1000, 2000); + total_delay++; } while (total_delay < hw->aq.asq_cmd_timeout); } diff --git a/drivers/net/ethernet/intel/i40evf/i40e_adminq.h b/drivers/net/ethernet/intel/i40evf/i40e_adminq.h index 91a5c5b..f40cfac 100644 --- a/drivers/net/ethernet/intel/i40evf/i40e_adminq.h +++ b/drivers/net/ethernet/intel/i40evf/i40e_adminq.h @@ -141,7 +141,7 @@ static inline int i40e_aq_rc_to_posix(u16 aq_rc) /* general information */ #define I40E_AQ_LARGE_BUF 512 -#define I40E_ASQ_CMD_TIMEOUT 100000 /* usecs */ +#define I40E_ASQ_CMD_TIMEOUT 100 /* msecs */ void i40evf_fill_default_direct_cmd_desc(struct i40e_aq_desc *desc, u16 opcode);