diff mbox

[2/3] tpm: reduce poll sleep time between send() and recv() in tpm_transmit()

Message ID 20180228191828.20056-2-nayna@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nayna Feb. 28, 2018, 7:18 p.m. UTC
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(-)

Comments

Jarkko Sakkinen March 1, 2018, 9:22 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nayna March 1, 2018, 6:56 p.m. UTC | #2
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
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 5, 2018, 10:56 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 5, 2018, 6:01 p.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar March 5, 2018, 7:07 p.m. UTC | #5
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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 6, 2018, 11:06 a.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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));