[PATCH RFC] s390/sclp: pass timeout as HZ independent value

[PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Nicholas Mc Guire]

schedule_timeout takes a timeout in jiffies but the code currently is
passing in a constant SDIAS_SLEEP_TICKS which sounds like it should be
in jiffies but it is actually not and thus makes this timeout HZ
dependent, to fix this it is passed through msecs_to_jiffies().

patch was not even compile tested as s390 toolchain from kernel.org 
failed for me (on a debian wheezy x86_64) with:
cc1: error: unrecognized command line option '-mtune=zEC12'

Patch is against 4.0-rc5 (localversion-next is -next-20150527)

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

As there is no documentation of the intended timeout it might be wrong
to convert it with msecs_to_jiffies() since the effective timeout is 
at least a factor 10 smaller than it is now - so someone that knows this
driver needs to check on the actual value - but in any case it needs to
be passed in a HZ independent way.

 drivers/s390/char/sclp_sdias.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/char/sclp_sdias.c b/drivers/s390/char/sclp_sdias.c
index eb7cb07..ab92a75 100644
--- a/drivers/s390/char/sclp_sdias.c
+++ b/drivers/s390/char/sclp_sdias.c
@@ -68,7 +68,7 @@ static int sdias_sclp_send(struct sclp_req *req)
 			/* not initiated, wait some time and retry */
 			set_current_state(TASK_INTERRUPTIBLE);
 			TRACE("add request failed: rc = %i\n",rc);
-			schedule_timeout(SDIAS_SLEEP_TICKS);
+			schedule_timeout(msecs_to_jiffies(SDIAS_SLEEP_TICKS));
 			continue;
 		}
 		/* initiated, wait for completion of service call */
-- 
1.7.10.4

Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Heiko Carstens]

On Wed, May 27, 2015 at 07:04:43PM +0200, Nicholas Mc Guire wrote:
> schedule_timeout takes a timeout in jiffies but the code currently is
> passing in a constant SDIAS_SLEEP_TICKS which sounds like it should be
> in jiffies but it is actually not and thus makes this timeout HZ
> dependent, to fix this it is passed through msecs_to_jiffies().
> 
> patch was not even compile tested as s390 toolchain from kernel.org 
> failed for me (on a debian wheezy x86_64) with:
> cc1: error: unrecognized command line option '-mtune=zEC12'
> 
> Patch is against 4.0-rc5 (localversion-next is -next-20150527)
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> As there is no documentation of the intended timeout it might be wrong
> to convert it with msecs_to_jiffies() since the effective timeout is 
> at least a factor 10 smaller than it is now - so someone that knows this
> driver needs to check on the actual value - but in any case it needs to
> be passed in a HZ independent way.

Yes, the orginal code seems to be broken. Since I've no idea what the intended
timeout value should be, let's simply ask Michael, who wrote this code eight
years ago ;)
While these lines get touched anyway, it would make sense to use
schedule_timeout_interruptible() instead, and get rid of set_current_state().


>  drivers/s390/char/sclp_sdias.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/char/sclp_sdias.c b/drivers/s390/char/sclp_sdias.c
> index eb7cb07..ab92a75 100644
> --- a/drivers/s390/char/sclp_sdias.c
> +++ b/drivers/s390/char/sclp_sdias.c
> @@ -68,7 +68,7 @@ static int sdias_sclp_send(struct sclp_req *req)
>  			/* not initiated, wait some time and retry */
>  			set_current_state(TASK_INTERRUPTIBLE);
>  			TRACE("add request failed: rc = %i\n",rc);
> -			schedule_timeout(SDIAS_SLEEP_TICKS);
> +			schedule_timeout(msecs_to_jiffies(SDIAS_SLEEP_TICKS));
>  			continue;
>  		}
>  		/* initiated, wait for completion of service call */
> -- 

Thanks,
Heiko

Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Nicholas Mc Guire]

On Fri, 29 May 2015, Heiko Carstens wrote:

> On Wed, May 27, 2015 at 07:04:43PM +0200, Nicholas Mc Guire wrote:
> > schedule_timeout takes a timeout in jiffies but the code currently is
> > passing in a constant SDIAS_SLEEP_TICKS which sounds like it should be
> > in jiffies but it is actually not and thus makes this timeout HZ
> > dependent, to fix this it is passed through msecs_to_jiffies().
> > 
> > patch was not even compile tested as s390 toolchain from kernel.org 
> > failed for me (on a debian wheezy x86_64) with:
> > cc1: error: unrecognized command line option '-mtune=zEC12'
> > 
> > Patch is against 4.0-rc5 (localversion-next is -next-20150527)
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > As there is no documentation of the intended timeout it might be wrong
> > to convert it with msecs_to_jiffies() since the effective timeout is 
> > at least a factor 10 smaller than it is now - so someone that knows this
> > driver needs to check on the actual value - but in any case it needs to
> > be passed in a HZ independent way.
> 
> Yes, the orginal code seems to be broken. Since I've no idea what the intended
> timeout value should be, let's simply ask Michael, who wrote this code eight
> years ago ;)
> While these lines get touched anyway, it would make sense to use
> schedule_timeout_interruptible() instead, and get rid of set_current_state().
>
Well that is not really equivalent
schedule_timeout_interruptible() is doing
__set_current_state not set_current_state
so that would drop the mb() and no WRITE_ONCE()

thx!
hofrat
 

Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Heiko Carstens]

On Fri, May 29, 2015 at 11:51:54AM +0200, Nicholas Mc Guire wrote:
> On Fri, 29 May 2015, Heiko Carstens wrote:
> > Yes, the orginal code seems to be broken. Since I've no idea what the intended
> > timeout value should be, let's simply ask Michael, who wrote this code eight
> > years ago ;)
> > While these lines get touched anyway, it would make sense to use
> > schedule_timeout_interruptible() instead, and get rid of set_current_state().
> >
> Well that is not really equivalent
> schedule_timeout_interruptible() is doing
> __set_current_state not set_current_state
> so that would drop the mb() and no WRITE_ONCE()

And how does that matter in this case?

Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Nicholas Mc Guire]

On Fri, 29 May 2015, Heiko Carstens wrote:

> On Fri, May 29, 2015 at 11:51:54AM +0200, Nicholas Mc Guire wrote:
> > On Fri, 29 May 2015, Heiko Carstens wrote:
> > > Yes, the orginal code seems to be broken. Since I've no idea what the intended
> > > timeout value should be, let's simply ask Michael, who wrote this code eight
> > > years ago ;)
> > > While these lines get touched anyway, it would make sense to use
> > > schedule_timeout_interruptible() instead, and get rid of set_current_state().
> > >
> > Well that is not really equivalent
> > schedule_timeout_interruptible() is doing
> > __set_current_state not set_current_state
> > so that would drop the mb() and no WRITE_ONCE()
> 
> And how does that matter in this case?
>
I do not know - did not look into it - in any case
its not a 1:1 API consolidation that all I wanted to point out
before changing anything.

thx!
hofrat

Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Michael Holzheu]

On Fri, 29 May 2015 13:49:36 +0200
Nicholas Mc Guire <der.herr@hofr.at> wrote:

> On Fri, 29 May 2015, Heiko Carstens wrote:
> 
> > On Fri, May 29, 2015 at 11:51:54AM +0200, Nicholas Mc Guire wrote:
> > > On Fri, 29 May 2015, Heiko Carstens wrote:
> > > > Yes, the orginal code seems to be broken. Since I've no idea what the intended
> > > > timeout value should be, let's simply ask Michael, who wrote this code eight
> > > > years ago ;)
> > > > While these lines get touched anyway, it would make sense to use
> > > > schedule_timeout_interruptible() instead, and get rid of set_current_state().
> > > >
> > > Well that is not really equivalent
> > > schedule_timeout_interruptible() is doing
> > > __set_current_state not set_current_state
> > > so that would drop the mb() and no WRITE_ONCE()
> > 
> > And how does that matter in this case?
> >
> I do not know - did not look into it - in any case
> its not a 1:1 API consolidation that all I wanted to point out
> before changing anything.

I agree, 1:1 consolidation is better here.

But I would like to remove the SDIAS_SLEEP_TICKS define and just
use HZ / 2 in schedule_timeout(). Could you please resend the
updated patch? We will then add it to our tree.

Thanks
Michael

Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Nicholas Mc Guire]

On Fri, 29 May 2015, Michael Holzheu wrote:

> On Fri, 29 May 2015 13:49:36 +0200
> Nicholas Mc Guire <der.herr@hofr.at> wrote:
> 
> > On Fri, 29 May 2015, Heiko Carstens wrote:
> > 
> > > On Fri, May 29, 2015 at 11:51:54AM +0200, Nicholas Mc Guire wrote:
> > > > On Fri, 29 May 2015, Heiko Carstens wrote:
> > > > > Yes, the orginal code seems to be broken. Since I've no idea what the intended
> > > > > timeout value should be, let's simply ask Michael, who wrote this code eight
> > > > > years ago ;)
> > > > > While these lines get touched anyway, it would make sense to use
> > > > > schedule_timeout_interruptible() instead, and get rid of set_current_state().
> > > > >
> > > > Well that is not really equivalent
> > > > schedule_timeout_interruptible() is doing
> > > > __set_current_state not set_current_state
> > > > so that would drop the mb() and no WRITE_ONCE()
> > > 
> > > And how does that matter in this case?
> > >
> > I do not know - did not look into it - in any case
> > its not a 1:1 API consolidation that all I wanted to point out
> > before changing anything.
> 
> I agree, 1:1 consolidation is better here.
> 
> But I would like to remove the SDIAS_SLEEP_TICKS define and just
> use HZ / 2 in schedule_timeout(). Could you please resend the
> updated patch? We will then add it to our tree.
>
will do that.

I was compile testing with x86_64-gcc-4.6.3-nolibc_s390x-linux.tar.bz2 
https://www.kernel.org/pub/tools/crosstool/
but that fails for all of the available defconfigs in arch/s390/configs
would you have a pointer to a toolchain that can be used for compile testing ?

thx!
hofrat

Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Heiko Carstens]

On Fri, May 29, 2015 at 04:16:46PM +0200, Nicholas Mc Guire wrote:
> On Fri, 29 May 2015, Michael Holzheu wrote:
> 
> > On Fri, 29 May 2015 13:49:36 +0200
> > Nicholas Mc Guire <der.herr@hofr.at> wrote:
> > 
> > > On Fri, 29 May 2015, Heiko Carstens wrote:
> > > 
> > > > On Fri, May 29, 2015 at 11:51:54AM +0200, Nicholas Mc Guire wrote:
> > > > > On Fri, 29 May 2015, Heiko Carstens wrote:
> > > > > > Yes, the orginal code seems to be broken. Since I've no idea what the intended
> > > > > > timeout value should be, let's simply ask Michael, who wrote this code eight
> > > > > > years ago ;)
> > > > > > While these lines get touched anyway, it would make sense to use
> > > > > > schedule_timeout_interruptible() instead, and get rid of set_current_state().
> > > > > >
> > > > > Well that is not really equivalent
> > > > > schedule_timeout_interruptible() is doing
> > > > > __set_current_state not set_current_state
> > > > > so that would drop the mb() and no WRITE_ONCE()
> > > > 
> > > > And how does that matter in this case?
> > > >
> > > I do not know - did not look into it - in any case
> > > its not a 1:1 API consolidation that all I wanted to point out
> > > before changing anything.
> > 
> > I agree, 1:1 consolidation is better here.
> > 
> > But I would like to remove the SDIAS_SLEEP_TICKS define and just
> > use HZ / 2 in schedule_timeout(). Could you please resend the
> > updated patch? We will then add it to our tree.
> >
> will do that.
> 
> I was compile testing with x86_64-gcc-4.6.3-nolibc_s390x-linux.tar.bz2 
> https://www.kernel.org/pub/tools/crosstool/
> but that fails for all of the available defconfigs in arch/s390/configs
> would you have a pointer to a toolchain that can be used for compile testing ?

Just change processor type to "IBM System z10", or manually edit .config, and
change CONFIG_MARCH_ZEC12 to be disabled and CONFIG_MARCH_Z10 instead to yes.
The same applies to CONFIG_MARCH_ZEC12_TUNE.
However we will accept the patch also without compile test.

Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

[Nicholas Mc Guire]

On Fri, 29 May 2015, Heiko Carstens wrote:

> On Fri, May 29, 2015 at 04:16:46PM +0200, Nicholas Mc Guire wrote:
> > On Fri, 29 May 2015, Michael Holzheu wrote:
> > 
> > > On Fri, 29 May 2015 13:49:36 +0200
> > > Nicholas Mc Guire <der.herr@hofr.at> wrote:
> > > 
> > > > On Fri, 29 May 2015, Heiko Carstens wrote:
> > > > 
> > > > > On Fri, May 29, 2015 at 11:51:54AM +0200, Nicholas Mc Guire wrote:
> > > > > > On Fri, 29 May 2015, Heiko Carstens wrote:
> > > > > > > Yes, the orginal code seems to be broken. Since I've no idea what the intended
> > > > > > > timeout value should be, let's simply ask Michael, who wrote this code eight
> > > > > > > years ago ;)
> > > > > > > While these lines get touched anyway, it would make sense to use
> > > > > > > schedule_timeout_interruptible() instead, and get rid of set_current_state().
> > > > > > >
> > > > > > Well that is not really equivalent
> > > > > > schedule_timeout_interruptible() is doing
> > > > > > __set_current_state not set_current_state
> > > > > > so that would drop the mb() and no WRITE_ONCE()
> > > > > 
> > > > > And how does that matter in this case?
> > > > >
> > > > I do not know - did not look into it - in any case
> > > > its not a 1:1 API consolidation that all I wanted to point out
> > > > before changing anything.
> > > 
> > > I agree, 1:1 consolidation is better here.
> > > 
> > > But I would like to remove the SDIAS_SLEEP_TICKS define and just
> > > use HZ / 2 in schedule_timeout(). Could you please resend the
> > > updated patch? We will then add it to our tree.
> > >
> > will do that.
> > 
> > I was compile testing with x86_64-gcc-4.6.3-nolibc_s390x-linux.tar.bz2 
> > https://www.kernel.org/pub/tools/crosstool/
> > but that fails for all of the available defconfigs in arch/s390/configs
> > would you have a pointer to a toolchain that can be used for compile testing ?
> 
> Just change processor type to "IBM System z10", or manually edit .config, and
> change CONFIG_MARCH_ZEC12 to be disabled and CONFIG_MARCH_Z10 instead to yes.
> The same applies to CONFIG_MARCH_ZEC12_TUNE.
> However we will accept the patch also without compile test.
> 
yup - thats working - thanks !

thx!
hofrat