| Message ID | 20170201200248.15137-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-02-01 at 22:02 +0200, Jarkko Sakkinen wrote: > Use wait_event() and with its help clean up all the unnecessary cruft > from the implementation of ksgxswapd. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> Tested-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > drivers/platform/x86/intel_sgx_page_cache.c | 27 ++++----------------------- > 1 file changed, 4 insertions(+), 23 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c > b/drivers/platform/x86/intel_sgx_page_cache.c > index d073057..95949de 100644 > --- a/drivers/platform/x86/intel_sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan) > > int ksgxswapd(void *p) > { > - DEFINE_WAIT(wait); > - unsigned int nr_free; > - unsigned int nr_high; > + while (!kthread_should_stop()) { > + wait_event(ksgxswapd_waitq, kthread_should_stop() || > + sgx_nr_free_pages < sgx_nr_high_pages); > > - for ( ; ; ) { > - if (kthread_should_stop()) > - break; > - > - spin_lock(&sgx_free_list_lock); > - nr_free = sgx_nr_free_pages; > - nr_high = sgx_nr_high_pages; > - spin_unlock(&sgx_free_list_lock); > - > - if (nr_free < nr_high) { > + if (sgx_nr_free_pages < sgx_nr_high_pages) > sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX); > - schedule(); > - } else { > - prepare_to_wait(&ksgxswapd_waitq, > - &wait, TASK_INTERRUPTIBLE); > - > - if (!kthread_should_stop()) > - schedule(); > - > - finish_wait(&ksgxswapd_waitq, &wait); > - } > } > > pr_info("%s: done\n", __func__);
On Wed, Feb 22, 2017 at 01:59:00PM -0800, Sean Christopherson wrote: > On Wed, 2017-02-01 at 22:02 +0200, Jarkko Sakkinen wrote: > > Use wait_event() and with its help clean up all the unnecessary cruft > > from the implementation of ksgxswapd. > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > Tested-by: > Sean Christopherson <sean.j.christopherson@intel.com> Thanks. There were some merge conflicts in the patch that you sent (probably because I've squashed my stuff). I'll fix those and apply it. /Jarkko > > > > --- > > drivers/platform/x86/intel_sgx_page_cache.c | 27 ++++----------------------- > > 1 file changed, 4 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c > > b/drivers/platform/x86/intel_sgx_page_cache.c > > index d073057..95949de 100644 > > --- a/drivers/platform/x86/intel_sgx_page_cache.c > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > > @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan) > > > > int ksgxswapd(void *p) > > { > > - DEFINE_WAIT(wait); > > - unsigned int nr_free; > > - unsigned int nr_high; > > + while (!kthread_should_stop()) { > > + wait_event(ksgxswapd_waitq, kthread_should_stop() || > > + sgx_nr_free_pages < sgx_nr_high_pages); > > > > - for ( ; ; ) { > > - if (kthread_should_stop()) > > - break; > > - > > - spin_lock(&sgx_free_list_lock); > > - nr_free = sgx_nr_free_pages; > > - nr_high = sgx_nr_high_pages; > > - spin_unlock(&sgx_free_list_lock); > > - > > - if (nr_free < nr_high) { > > + if (sgx_nr_free_pages < sgx_nr_high_pages) > > sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX); > > - schedule(); > > - } else { > > - prepare_to_wait(&ksgxswapd_waitq, > > - &wait, TASK_INTERRUPTIBLE); > > - > > - if (!kthread_should_stop()) > > - schedule(); > > - > > - finish_wait(&ksgxswapd_waitq, &wait); > > - } > > } > > > > pr_info("%s: done\n", __func__); >
> On Wed, Feb 22, 2017 at 01:59:00PM -0800, Sean Christopherson wrote: > > On Wed, 2017-02-01 at 22:02 +0200, Jarkko Sakkinen wrote: > > > Use wait_event() and with its help clean up all the unnecessary cruft > > > from the implementation of ksgxswapd. > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Tested-by: > > Sean Christopherson <sean.j.christopherson@intel.com> > > Thanks. There were some merge conflicts in the patch that you sent > (probably because I've squashed my stuff). I'll fix those and apply > it. > > /Jarkko Found an issue with this patch after letting my machine sit idle for a while. The wait_event macro uses TASK_UNINTERRUPTIBLE, which triggers the hung task checks when the thread is idle. The code should instead use wait_event_interruptible. [ 3599.969498] INFO: task ksgxswapd:10754 blocked for more than 120 seconds. [ 3599.976882] Tainted: P OE 4.4.0-64-generic #85-Ubuntu [ 3599.984238] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 3599.998900] ksgxswapd D ffff8808418a3e50 0 10754 2 0x00000000 [ 3599.998904] ffff8808418a3e50 ffffffffc0bee34b ffff880841d23c00 ffff880827533c00 [ 3599.998906] ffff8808418a4000 0000000000000000 ffffffffc0bef0d5 0000000000000000 [ 3599.998907] 0000000000000000 ffff8808418a3e68 ffffffff818384d5 ffff8808276ba300 [ 3599.998909] Call Trace: [ 3599.998915] [<ffffffffc0bee34b>] ? kref_put+0x25/0x27 [isgx] [ 3599.998917] [<ffffffffc0bef0d5>] ? sgx_swap_pages+0xff/0xff [isgx] [ 3599.998920] [<ffffffff818384d5>] schedule+0x35/0x80 [ 3599.998922] [<ffffffffc0bef171>] ksgxswapd+0x9c/0x109 [isgx] [ 3599.998924] [<ffffffff810c41e0>] ? wake_atomic_t_function+0x60/0x60 [ 3599.998926] [<ffffffff810a0ba8>] kthread+0xd8/0xf0 [ 3599.998928] [<ffffffff810a0ad0>] ? kthread_create_on_node+0x1e0/0x1e0 [ 3599.998929] [<ffffffff8183c98f>] ret_from_fork+0x3f/0x70 [ 3599.998931] [<ffffffff810a0ad0>] ? kthread_create_on_node+0x1e0/0x1e0 > > > > > > > --- > > > drivers/platform/x86/intel_sgx_page_cache.c | 27 > > > ++++----------------------- 1 file changed, 4 insertions(+), 23 > > > deletions(-) > > > > > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c > > > b/drivers/platform/x86/intel_sgx_page_cache.c > > > index d073057..95949de 100644 > > > --- a/drivers/platform/x86/intel_sgx_page_cache.c > > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > > > @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan) > > > > > > int ksgxswapd(void *p) > > > { > > > - DEFINE_WAIT(wait); > > > - unsigned int nr_free; > > > - unsigned int nr_high; > > > + while (!kthread_should_stop()) { > > > + wait_event(ksgxswapd_waitq, kthread_should_stop() || > > > + sgx_nr_free_pages < sgx_nr_high_pages); > > > > > > - for ( ; ; ) { > > > - if (kthread_should_stop()) > > > - break; > > > - > > > - spin_lock(&sgx_free_list_lock); > > > - nr_free = sgx_nr_free_pages; > > > - nr_high = sgx_nr_high_pages; > > > - spin_unlock(&sgx_free_list_lock); > > > - > > > - if (nr_free < nr_high) { > > > + if (sgx_nr_free_pages < sgx_nr_high_pages) > > > sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX); > > > - schedule(); > > > - } else { > > > - prepare_to_wait(&ksgxswapd_waitq, > > > - &wait, TASK_INTERRUPTIBLE); > > > - > > > - if (!kthread_should_stop()) > > > - schedule(); > > > - > > > - finish_wait(&ksgxswapd_waitq, &wait); > > > - } > > > } > > > > > > pr_info("%s: done\n", __func__); > >
On Fri, Feb 24, 2017 at 04:55:19PM +0000, Christopherson, Sean J wrote: > > On Wed, Feb 22, 2017 at 01:59:00PM -0800, Sean Christopherson wrote: > > > On Wed, 2017-02-01 at 22:02 +0200, Jarkko Sakkinen wrote: > > > > Use wait_event() and with its help clean up all the unnecessary cruft > > > > from the implementation of ksgxswapd. > > > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > Tested-by: > > > Sean Christopherson <sean.j.christopherson@intel.com> > > > > Thanks. There were some merge conflicts in the patch that you sent > > (probably because I've squashed my stuff). I'll fix those and apply > > it. > > > > /Jarkko > > Found an issue with this patch after letting my machine sit idle for a while. > The wait_event macro uses TASK_UNINTERRUPTIBLE, which triggers the hung task > checks when the thread is idle. The code should instead use wait_event_interruptible. Good catch! I was able to reproduce this too. Thank you /Jarkko > > [ 3599.969498] INFO: task ksgxswapd:10754 blocked for more than 120 seconds. > [ 3599.976882] Tainted: P OE 4.4.0-64-generic #85-Ubuntu > [ 3599.984238] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this > message. [ 3599.998900] ksgxswapd D ffff8808418a3e50 0 10754 2 > 0x00000000 [ 3599.998904] ffff8808418a3e50 ffffffffc0bee34b ffff880841d23c00 > ffff880827533c00 [ 3599.998906] ffff8808418a4000 0000000000000000 > ffffffffc0bef0d5 0000000000000000 [ 3599.998907] 0000000000000000 > ffff8808418a3e68 ffffffff818384d5 ffff8808276ba300 [ 3599.998909] Call Trace: > [ 3599.998915] [<ffffffffc0bee34b>] ? kref_put+0x25/0x27 [isgx] > [ 3599.998917] [<ffffffffc0bef0d5>] ? sgx_swap_pages+0xff/0xff [isgx] > [ 3599.998920] [<ffffffff818384d5>] schedule+0x35/0x80 > [ 3599.998922] [<ffffffffc0bef171>] ksgxswapd+0x9c/0x109 [isgx] > [ 3599.998924] [<ffffffff810c41e0>] ? wake_atomic_t_function+0x60/0x60 > [ 3599.998926] [<ffffffff810a0ba8>] kthread+0xd8/0xf0 > [ 3599.998928] [<ffffffff810a0ad0>] ? kthread_create_on_node+0x1e0/0x1e0 > [ 3599.998929] [<ffffffff8183c98f>] ret_from_fork+0x3f/0x70 > [ 3599.998931] [<ffffffff810a0ad0>] ? kthread_create_on_node+0x1e0/0x1e0 > > > > > > > > > > > > --- > > > > drivers/platform/x86/intel_sgx_page_cache.c | 27 > > > > ++++----------------------- 1 file changed, 4 insertions(+), 23 > > > > deletions(-) > > > > > > > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c > > > > b/drivers/platform/x86/intel_sgx_page_cache.c > > > > index d073057..95949de 100644 > > > > --- a/drivers/platform/x86/intel_sgx_page_cache.c > > > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > > > > @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan) > > > > > > > > int ksgxswapd(void *p) > > > > { > > > > - DEFINE_WAIT(wait); > > > > - unsigned int nr_free; > > > > - unsigned int nr_high; > > > > + while (!kthread_should_stop()) { > > > > + wait_event(ksgxswapd_waitq, kthread_should_stop() || > > > > + sgx_nr_free_pages < sgx_nr_high_pages); > > > > > > > > - for ( ; ; ) { > > > > - if (kthread_should_stop()) > > > > - break; > > > > - > > > > - spin_lock(&sgx_free_list_lock); > > > > - nr_free = sgx_nr_free_pages; > > > > - nr_high = sgx_nr_high_pages; > > > > - spin_unlock(&sgx_free_list_lock); > > > > - > > > > - if (nr_free < nr_high) { > > > > + if (sgx_nr_free_pages < sgx_nr_high_pages) > > > > sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX); > > > > - schedule(); > > > > - } else { > > > > - prepare_to_wait(&ksgxswapd_waitq, > > > > - &wait, TASK_INTERRUPTIBLE); > > > > - > > > > - if (!kthread_should_stop()) > > > > - schedule(); > > > > - > > > > - finish_wait(&ksgxswapd_waitq, &wait); > > > > - } > > > > } > > > > > > > > pr_info("%s: done\n", __func__); > > > > > >
diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c index d073057..95949de 100644 --- a/drivers/platform/x86/intel_sgx_page_cache.c +++ b/drivers/platform/x86/intel_sgx_page_cache.c @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan) int ksgxswapd(void *p) { - DEFINE_WAIT(wait); - unsigned int nr_free; - unsigned int nr_high; + while (!kthread_should_stop()) { + wait_event(ksgxswapd_waitq, kthread_should_stop() || + sgx_nr_free_pages < sgx_nr_high_pages); - for ( ; ; ) { - if (kthread_should_stop()) - break; - - spin_lock(&sgx_free_list_lock); - nr_free = sgx_nr_free_pages; - nr_high = sgx_nr_high_pages; - spin_unlock(&sgx_free_list_lock); - - if (nr_free < nr_high) { + if (sgx_nr_free_pages < sgx_nr_high_pages) sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX); - schedule(); - } else { - prepare_to_wait(&ksgxswapd_waitq, - &wait, TASK_INTERRUPTIBLE); - - if (!kthread_should_stop()) - schedule(); - - finish_wait(&ksgxswapd_waitq, &wait); - } } pr_info("%s: done\n", __func__);
Use wait_event() and with its help clean up all the unnecessary cruft from the implementation of ksgxswapd. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/platform/x86/intel_sgx_page_cache.c | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-)