| Message ID | 20180228191828.20056-2-nayna@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote: > In tpm_transmit, after send(), the code checks for status in a loop Maybe cutting hairs now but please just use the actual function name instead of send(). Just makes the commit log more useful asset. > - tpm_msleep(TPM_TIMEOUT); > + tpm_msleep(TPM_TIMEOUT_POLL); What about just calling schedule()? /Jarkko
On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote: > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote: >> In tpm_transmit, after send(), the code checks for status in a loop > Maybe cutting hairs now but please just use the actual function name > instead of send(). Just makes the commit log more useful asset. Sure, will do. > >> - tpm_msleep(TPM_TIMEOUT); >> + tpm_msleep(TPM_TIMEOUT_POLL); > What about just calling schedule()? I'm not sure what you mean by "schedule()". Are you suggesting instead of using usleep_range(), using something with an even finer grain construct? Thanks & Regards, - Nayna > > /Jarkko >
On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote: > > > On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote: > > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote: > > > In tpm_transmit, after send(), the code checks for status in a loop > > Maybe cutting hairs now but please just use the actual function name > > instead of send(). Just makes the commit log more useful asset. > Sure, will do. > > > > > - tpm_msleep(TPM_TIMEOUT); > > > + tpm_msleep(TPM_TIMEOUT_POLL); > > What about just calling schedule()? > I'm not sure what you mean by "schedule()". Are you suggesting instead of > using usleep_range(), using something with an even finer grain construct? > > Thanks & Regards, > - Nayna kernel/sched/core.c /Jarkko
On Mon, Mar 05, 2018 at 12:56:33PM +0200, Jarkko Sakkinen wrote: > On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote: > > > > > > On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote: > > > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote: > > > > In tpm_transmit, after send(), the code checks for status in a loop > > > Maybe cutting hairs now but please just use the actual function name > > > instead of send(). Just makes the commit log more useful asset. > > Sure, will do. > > > > > > > - tpm_msleep(TPM_TIMEOUT); > > > > + tpm_msleep(TPM_TIMEOUT_POLL); > > > What about just calling schedule()? > > I'm not sure what you mean by "schedule()". Are you suggesting instead of > > using usleep_range(), using something with an even finer grain construct? > > > > Thanks & Regards, > > - Nayna > > kernel/sched/core.c The question I'm trying ask to is: is it better to sleep such a short time or just ask scheduler to schedule something else after each iteration? /Jarkko
On Mon, 2018-03-05 at 20:01 +0200, Jarkko Sakkinen wrote: > On Mon, Mar 05, 2018 at 12:56:33PM +0200, Jarkko Sakkinen wrote: > > On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote: > > > > > > > > > On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote: > > > > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote: > > > > > In tpm_transmit, after send(), the code checks for status in a loop > > > > Maybe cutting hairs now but please just use the actual function name > > > > instead of send(). Just makes the commit log more useful asset. > > > Sure, will do. > > > > > > > > > - tpm_msleep(TPM_TIMEOUT); > > > > > + tpm_msleep(TPM_TIMEOUT_POLL); > > > > What about just calling schedule()? > > > I'm not sure what you mean by "schedule()". Are you suggesting instead of > > > using usleep_range(), using something with an even finer grain construct? > > > > > > Thanks & Regards, > > > - Nayna > > > > kernel/sched/core.c > > The question I'm trying ask to is: is it better to sleep such a short > time or just ask scheduler to schedule something else after each > iteration? I still don't understand why scheduling some work would be an improvement. We still need to loop, testing for the TPM command to complete. According to the schedule_hrtimeout_range() function comment, schedule_hrtimeout_range() is both power and performance friendly. What we didn't realize is that the hrtimer *uses* the maximum range to calculate the sleep time, but *may* return earlier based on the minimum time. This patch minimizes the tpm_msleep(). The subsequent patch in this patch set shows that 1 msec isn't fine enough granularity. I still think calling usleep_range() is the right solution, but it needs to be at a finer granularity than tpm_msleep() provides. Mimi
On Mon, 2018-03-05 at 14:07 -0500, Mimi Zohar wrote: > On Mon, 2018-03-05 at 20:01 +0200, Jarkko Sakkinen wrote: > > On Mon, Mar 05, 2018 at 12:56:33PM +0200, Jarkko Sakkinen wrote: > > > On Fri, Mar 02, 2018 at 12:26:35AM +0530, Nayna Jain wrote: > > > > > > > > > > > > On 03/01/2018 02:52 PM, Jarkko Sakkinen wrote: > > > > > On Wed, Feb 28, 2018 at 02:18:27PM -0500, Nayna Jain wrote: > > > > > > In tpm_transmit, after send(), the code checks for status in a loop > > > > > > > > > > Maybe cutting hairs now but please just use the actual function name > > > > > instead of send(). Just makes the commit log more useful asset. > > > > > > > > Sure, will do. > > > > > > > > > > > - tpm_msleep(TPM_TIMEOUT); > > > > > > + tpm_msleep(TPM_TIMEOUT_POLL); > > > > > > > > > > What about just calling schedule()? > > > > > > > > I'm not sure what you mean by "schedule()". Are you suggesting instead > > > > of > > > > using usleep_range(), using something with an even finer grain > > > > construct? > > > > > > > > Thanks & Regards, > > > > - Nayna > > > > > > kernel/sched/core.c > > > > The question I'm trying ask to is: is it better to sleep such a short > > time or just ask scheduler to schedule something else after each > > iteration? > > I still don't understand why scheduling some work would be an > improvement. We still need to loop, testing for the TPM command to > complete. > > According to the schedule_hrtimeout_range() function comment, > schedule_hrtimeout_range() is both power and performance friendly. > What we didn't realize is that the hrtimer *uses* the maximum range > to calculate the sleep time, but *may* return earlier based on the > minimum time. > > This patch minimizes the tpm_msleep(). The subsequent patch in this > patch set shows that 1 msec isn't fine enough granularity. I still > think calling usleep_range() is the right solution, but it needs to be > at a finer granularity than tpm_msleep() provides. > > Mimi We can move to usleep_range() in call sites where it makes sense instead of adjusting tpm_msleep() implementation... /Jarkko
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 76df4fbcf089..2d22f981f0c9 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -470,7 +470,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, goto out; } - tpm_msleep(TPM_TIMEOUT); + tpm_msleep(TPM_TIMEOUT_POLL); rmb(); } while (time_before(jiffies, stop));
In tpm_transmit, after send(), the code checks for status in a loop with polling every 5msec. It is expected that the tpm might return earlier than 5msec, so it might be adding to unnecessary delay. This patch reduces the polling sleep time from 5msec to 1msec. After this change, performance on a TPM 1.2 with an 8 byte burstcount for 1000 extends improved from ~14sec to ~10.7sec. Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> --- drivers/char/tpm/tpm-interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)