Re: [PATCH v3 01/16] mm: use put_page to free page instead of putback_lru_page

[Naoya Horiguchi]

On Tue, Apr 05, 2016 at 10:20:50AM +0200, Vlastimil Babka wrote:
> On 04/05/2016 03:54 AM, Naoya Horiguchi wrote:
> > On Mon, Apr 04, 2016 at 04:46:31PM +0200, Vlastimil Babka wrote:
> >> On 04/04/2016 06:45 AM, Naoya Horiguchi wrote:
> >>> On Mon, Apr 04, 2016 at 10:39:17AM +0900, Minchan Kim wrote:
> > ...
> >
> > Also (but not your fault) the put_page() preceding
> > test_set_page_hwpoison(page)) IMHO deserves a comment saying which
> > pin we are releasing and which one we still have (hopefully? if I
> > read description of da1b13ccfbebe right) otherwise it looks like
> > doing something with a page that we just potentially freed.
> 
>  Yes, while I read the code, I had same question. I think the releasing
>  refcount is for get_any_page.
> >>>
> >>> As the other callers of page migration do, soft_offline_page expects the
> >>> migration source page to be freed at this put_page() (no pin remains.)
> >>> The refcount released here is from isolate_lru_page() in 
> >>> __soft_offline_page().
> >>> (the pin by get_any_page is released by put_hwpoison_page just after it.)
> >>>
> >>> .. yes, doing something just after freeing page looks weird, but that's
> >>> how PageHWPoison flag works. IOW, many other page flags are maintained
> >>> only during one "allocate-free" life span, but PageHWPoison still does
> >>> its job beyond it.
> >>
> >> But what prevents the page from being allocated again between put_page()
> >> and test_set_page_hwpoison()? In that case we would be marking page
> >> poisoned while still in use, which is the same as marking it while still
> >> in use after a failed migration?
> > 
> > Actually nothing prevents that race. But I think that the result of the race
> > is that the error page can be reused for allocation, which results in 
> > killing
> > processes at page fault time. Soft offline is kind of mild/precautious thing
> > (for correctable errors that don't require immediate handling), so killing
> > processes looks to me an overkill. And marking hwpoison means that we can no
> > longer do retry from userspace.
> 
> So you agree that this race is a bug? It may turn a soft-offline attempt
> into a killed process. In that case we should fix it the same as we are
> fixing the failed migration case.

I agree, it's a bug, although rare and non-critical.

> Maybe it will be just enough to switch
> the test_set_page_hwpoison() and put_page() calls?

Unfortunately that restores the other race with unpoison (described below.)
Sorry for my bad/unclear statements, these races seems exclusive and a 
compatible
solution is not found, so I prioritized fixing the latter one by comparing
severity (the latter causes kernel crash,) which led to the current code.

> > And another practical thing is the race with unpoison_memory() as described
> > in commit da1b13ccfbebe. unpoison_memory() properly works only for properly
> > poisoned pages, so doing unpoison for in-use hwpoisoned pages is fragile.
> > That's why I'd like to avoid setting PageHWPoison for in-use pages if 
> > possible.
> > 
> >> (Also, which part prevents pages with PageHWPoison to be allocated
> >> again, anyway? I can't find it and test_set_page_hwpoison() doesn't
> >> remove from buddy freelists).
> > 
> > check_new_page() in mm/page_alloc.c should prevent reallocation of 
> > PageHWPoison.
> > As you pointed out, memory error handler doens't remove it from buddy 
> > freelists.
> 
> Oh, I see. It's using __PG_HWPOISON wrapper, so I didn't notice it when
> searching. In any case that results in a bad_page() warning, right? Is
> it desirable for a soft-offlined page?

That's right, and the bad_page warning might be too strong for soft offlining.
We can't tell which of memory_failure/soft_offline_page a PageHWPoison came
from, but users can find other lines in dmesg which should tell that.
And memory error events can hit buddy pages directly, in that case we still
need the check in check_new_page().

> If we didn't free poisoned pages
> to buddy system, they wouldn't trigger this warning.

Actually, we didn't free at commit add05cecef80 ("mm: soft-offline: don't free
target page in successful page migration"), but that's was reverted in
commit f4c18e6f7b5b ("mm: check __PG_HWPOISON separately from 
PAGE_FLAGS_CHECK_AT_*").
Now I start thinking the revert was a bad decision, so I'll dig this problem 
again.

> > BTW, it might be a bit off-topic, but recently I felt that check_new_page()
> > might be improvable, because when check_new_page() returns 1, the whole 
> > buddy
> > block (not only the bad page) seems to be leaked from buddy freelist.
> > For example, if thp (order 9) is requested, and PageHWPoison (or any other
> > types of bad pages) is found in an order 9 block, all 512 page are 
> > discarded.
> > Unpoison can't bring it back to buddy.
> > So, some code to split buddy block including bad page (and recovering code 
> > from
> > unpoison) might be helpful, although that's another story 

Re: [PATCH v4 6/6] hwmon: use smp_call_on_cpu() for dell-smm i8k

[Guenter Roeck]

On Tue, Apr 05, 2016 at 07:10:07AM +0200, Juergen Gross wrote:
> Use the smp_call_on_cpu() function to call system management
> mode on cpu 0.
> Make call secure by adding get_online_cpus() to avoid e.g. suspend
> resume cycles in between.
> 
> Signed-off-by: Juergen Gross 
> ---
> V4: add call to get_online_cpus()

Pali, any chance to test this ?

Thanks,
Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 35 ---
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index c43318d..643f3a1 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -21,6 +21,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -35,6 +36,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -130,23 +132,15 @@ static inline const char *i8k_get_dmi_data(int field)
>  /*
>   * Call the System Management Mode BIOS. Code provided by Jonathan Buzzard.
>   */
> -static int i8k_smm(struct smm_regs *regs)
> +static int i8k_smm_func(void *par)
>  {
>   int rc;
> + struct smm_regs *regs = par;
>   int eax = regs->eax;
> - cpumask_var_t old_mask;
>  
>   /* SMM requires CPU 0 */
> - if (!alloc_cpumask_var(_mask, GFP_KERNEL))
> - return -ENOMEM;
> - cpumask_copy(old_mask, >cpus_allowed);
> - rc = set_cpus_allowed_ptr(current, cpumask_of(0));
> - if (rc)
> - goto out;
> - if (smp_processor_id() != 0) {
> - rc = -EBUSY;
> - goto out;
> - }
> + if (smp_processor_id() != 0)
> + return -EBUSY;
>  
>  #if defined(CONFIG_X86_64)
>   asm volatile("pushq %%rax\n\t"
> @@ -204,13 +198,24 @@ static int i8k_smm(struct smm_regs *regs)
>   if (rc != 0 || (regs->eax & 0x) == 0x || regs->eax == eax)
>   rc = -EINVAL;
>  
> -out:
> - set_cpus_allowed_ptr(current, old_mask);
> - free_cpumask_var(old_mask);
>   return rc;
>  }
>  
>  /*
> + * Call the System Management Mode BIOS.
> + */
> +static int i8k_smm(struct smm_regs *regs)
> +{
> + int ret;
> +
> + get_online_cpus();
> + ret = smp_call_on_cpu(0, true, i8k_smm_func, regs);
> + put_online_cpus();
> +
> + return ret;
> +}
> +
> +/*
>   * Read the fan status.
>   */
>  static int i8k_get_fan_status(int fan)
> -- 
> 2.6.2
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 6/6] hwmon: use smp_call_on_cpu() for dell-smm i8k

[Pali Rohár]

On Tuesday 05 April 2016 16:54:14 Guenter Roeck wrote:
> On Tue, Apr 05, 2016 at 07:10:07AM +0200, Juergen Gross wrote:
> > Use the smp_call_on_cpu() function to call system management
> > mode on cpu 0.
> > Make call secure by adding get_online_cpus() to avoid e.g. suspend
> > resume cycles in between.
> > 
> > Signed-off-by: Juergen Gross 
> > ---
> > V4: add call to get_online_cpus()
> 
> Pali, any chance to test this ?

I can test it, but just on machine where (probably) smm calls can be 
send from any cpu... Need some time for testing and I believe I can do 
that at the end of the week.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] drm/virtio: send vblank event after crtc updates

[Gustavo Padovan]

Hi,

Any comment on this?

Gustavo

2016-03-22 Gustavo Padovan :

> From: Gustavo Padovan 
> 
> virtio_gpu was failing to send vblank events when using the atomic IOCTL
> with the DRM_MODE_PAGE_FLIP_EVENT flag set. This patch fixes each and
> enables atomic pageflips updates.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/gpu/drm/virtio/virtgpu_display.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c 
> b/drivers/gpu/drm/virtio/virtgpu_display.c
> index b70bb8b..4f372e0 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -274,12 +274,24 @@ static int virtio_gpu_crtc_atomic_check(struct drm_crtc 
> *crtc,
>   return 0;
>  }
>  
> +static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc,
> +  struct drm_crtc_state *old_state)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(>dev->event_lock, flags);
> + if (crtc->state->event)
> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
> + spin_unlock_irqrestore(>dev->event_lock, flags);
> +}
> +
>  static const struct drm_crtc_helper_funcs virtio_gpu_crtc_helper_funcs = {
>   .enable= virtio_gpu_crtc_enable,
>   .disable   = virtio_gpu_crtc_disable,
>   .mode_fixup= virtio_gpu_crtc_mode_fixup,
>   .mode_set_nofb = virtio_gpu_crtc_mode_set_nofb,
>   .atomic_check  = virtio_gpu_crtc_atomic_check,
> + .atomic_flush  = virtio_gpu_crtc_atomic_flush,
>  };
>  
>  static void virtio_gpu_enc_mode_set(struct drm_encoder *encoder,
> -- 
> 2.5.0
> 

Gustavo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio: fix "warning: ‘queue’ may be used uninitialized"

[Michael S. Tsirkin]

On Tue, Apr 05, 2016 at 09:34:24AM -0400, Jeff Mahoney wrote:
> On 4/5/16 4:04 AM, Michael S. Tsirkin wrote:
> > On Mon, Apr 04, 2016 at 02:14:19PM -0400, Jeff Mahoney wrote:
> >> This fixes the following warning:
> >> drivers/virtio/virtio_ring.c:1032:5: warning: ‘queue’ may be used
> >> uninitialized in this function
> >>
> >> The conditions that govern when queue is set aren't apparent to gcc.
> >>
> >> Setting queue = NULL clears the warning.
> >>
> >> Signed-off-by: Jeff Mahoney 
> > 
> > Which gcc version produces this warning?
> > I do not seem to see it with gcc 5.3.1.
> 
> gcc version 4.8.5 (SUSE Linux)

Looks like a minor compiler bug then, fixed since.
I don't think we need to worry about that this.

> > Also - use uninitialized_var then?
> 
> If it were a fast path, sure, but otherwise the use of uninitialized_var
> just makes similar issues harder to debug if the code changes.
> 
> -Jeff

Same does = NULL though. It's not like things will work with queue ==
NULL, so it'll still crash if it's not inited properly.

> >> ---
> >>
> >>  drivers/virtio/virtio_ring.c |2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> --- a/drivers/virtio/virtio_ring.c
> >> +++ b/drivers/virtio/virtio_ring.c
> >> @@ -1006,7 +1006,7 @@ struct virtqueue *vring_create_virtqueue
> >>const char *name)
> >>  {
> >>struct virtqueue *vq;
> >> -  void *queue;
> >> +  void *queue = NULL;
> >>dma_addr_t dma_addr;
> >>size_t queue_size_in_bytes;
> >>struct vring vring;
> >>
> >> -- 
> >> Jeff Mahoney
> >> SUSE Labs
> > 
> 
> 
> -- 
> Jeff Mahoney
> SUSE Labs
> 



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio: fix "warning: ‘queue’ may be used uninitialized"

[Jeff Mahoney]

On 4/5/16 4:04 AM, Michael S. Tsirkin wrote:
> On Mon, Apr 04, 2016 at 02:14:19PM -0400, Jeff Mahoney wrote:
>> This fixes the following warning:
>> drivers/virtio/virtio_ring.c:1032:5: warning: ‘queue’ may be used
>> uninitialized in this function
>>
>> The conditions that govern when queue is set aren't apparent to gcc.
>>
>> Setting queue = NULL clears the warning.
>>
>> Signed-off-by: Jeff Mahoney 
> 
> Which gcc version produces this warning?
> I do not seem to see it with gcc 5.3.1.

gcc version 4.8.5 (SUSE Linux)

> Also - use uninitialized_var then?

If it were a fast path, sure, but otherwise the use of uninitialized_var
just makes similar issues harder to debug if the code changes.

-Jeff

>> ---
>>
>>  drivers/virtio/virtio_ring.c |2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -1006,7 +1006,7 @@ struct virtqueue *vring_create_virtqueue
>>  const char *name)
>>  {
>>  struct virtqueue *vq;
>> -void *queue;
>> +void *queue = NULL;
>>  dma_addr_t dma_addr;
>>  size_t queue_size_in_bytes;
>>  struct vring vring;
>>
>> -- 
>> Jeff Mahoney
>> SUSE Labs
> 


-- 
Jeff Mahoney
SUSE Labs



signature.asc
Description: OpenPGP digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 04/16] mm/balloon: use general movable page feature into balloon

[Vlastimil Babka]

On 03/30/2016 09:12 AM, Minchan Kim wrote:

Now, VM has a feature to migrate non-lru movable pages so
balloon doesn't need custom migration hooks in migrate.c
and compact.c. Instead, this patch implements page->mapping
->{isolate|migrate|putback} functions.

With that, we could remove hooks for ballooning in general
migration functions and make balloon compaction simple.

Cc: virtualization@lists.linux-foundation.org
Cc: Rafael Aquini 
Cc: Konstantin Khlebnikov 
Signed-off-by: Gioh Kim 
Signed-off-by: Minchan Kim 


I'm not familiar with the inode and pseudofs stuff, so just some things 
I noticed:



-#define PAGE_MOVABLE_MAPCOUNT_VALUE (-255)
+#define PAGE_MOVABLE_MAPCOUNT_VALUE (-256)
+#define PAGE_BALLOON_MAPCOUNT_VALUE PAGE_MOVABLE_MAPCOUNT_VALUE

  static inline int PageMovable(struct page *page)
  {
-   return ((test_bit(PG_movable, &(page)->flags) &&
-   atomic_read(>_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE)
-   || PageBalloon(page));
+   return (test_bit(PG_movable, &(page)->flags) &&
+   atomic_read(>_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE);
  }

  /* Caller should hold a PG_lock */
@@ -645,6 +626,35 @@ static inline void __ClearPageMovable(struct page *page)

  PAGEFLAG(Isolated, isolated, PF_ANY);

+static inline int PageBalloon(struct page *page)
+{
+   return atomic_read(>_mapcount) == PAGE_BALLOON_MAPCOUNT_VALUE
+   && PagePrivate2(page);
+}


Hmm so you are now using PG_private_2 flag here, but it's not 
documented. Also the only caller of PageBalloon() seems to be 
stable_page_flags(). Which will now report all movable pages with 
PG_private_2 as KPF_BALOON. Seems like an overkill and also not 
reliable. Could it test e.g. page->mapping instead?


Or maybe if we manage to get rid of PAGE_MOVABLE_MAPCOUNT_VALUE, we can 
keep PAGE_BALLOON_MAPCOUNT_VALUE to simply distinguish balloon pages for 
stable_page_flags().



@@ -1033,7 +1019,7 @@ static int __unmap_and_move(struct page *page, struct 
page *newpage,
  out:
/* If migration is successful, move newpage to right list */
if (rc == MIGRATEPAGE_SUCCESS) {
-   if (unlikely(__is_movable_balloon_page(newpage)))
+   if (unlikely(PageMovable(newpage)))
put_page(newpage);
else
putback_lru_page(newpage);


Hmm shouldn't the condition have been changed to

if (unlikely(__is_movable_balloon_page(newpage)) || PageMovable(newpage)

by patch 02/16? And this patch should be just removing the 
balloon-specific check? Otherwise it seems like between patches 02 and 
04, other kinds of PageMovable pages were unnecessarily/wrongly routed 
through putback_lru_page()?



diff --git a/mm/vmscan.c b/mm/vmscan.c
index d82196244340..c7696a2e11c7 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1254,7 +1254,7 @@ unsigned long reclaim_clean_pages_from_list(struct zone 
*zone,

list_for_each_entry_safe(page, next, page_list, lru) {
if (page_is_file_cache(page) && !PageDirty(page) &&
-   !isolated_balloon_page(page)) {
+   !PageIsolated(page)) {
ClearPageActive(page);
list_move(>lru, _pages);
}


This looks like the same comment as above at first glance. But looking 
closer, it's even weirder. isolated_balloon_page() was simply 
PageBalloon() after d6d86c0a7f8dd... weird already. You replace it with 
check for !PageIsolated() which looks like a more correct check, so ok. 
Except the potential false positive with PG_owner_priv_1.


Thanks.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 3/6] smp: add function to execute a function synchronously on a cpu

[Juergen Gross]

On 05/04/16 10:11, Peter Zijlstra wrote:
> On Tue, Apr 05, 2016 at 07:10:04AM +0200, Juergen Gross wrote:
>> +int smp_call_on_cpu(unsigned int cpu, bool pin, int (*func)(void *), void 
>> *par)
> 
> Why .pin and not .phys? .pin does not (to me) reflect the
> hypervisor/physical-cpu thing.

I don't mind either way. As you don't like .pin, lets name it .phys.

> Also, as per smp_call_function_single() would it not be more consistent
> to make this the last argument?

Okay, I'll change it.

> 
>> +{
>> +struct smp_call_on_cpu_struct sscs = {
>> +.work = __WORK_INITIALIZER(sscs.work, smp_call_on_cpu_callback),
>> +.done = COMPLETION_INITIALIZER_ONSTACK(sscs.done),
>> +.func = func,
>> +.data = par,
>> +.cpu  = pin ? cpu : -1,
>> +};
>> +
>> +if (cpu >= nr_cpu_ids)
> 
> You might want to also include cpu_online().
> 
>   if (cpu >= nr_cpu_ids || !cpu_online(cpu))

Indeed, good idea.

>> +return -ENXIO;
> 
> Seeing how its fairly hard to schedule work on a cpu that's not actually
> there.

Really? ;-)


Juergen
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Xen-devel] [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu

[David Vrabel]

On 05/04/16 11:01, Juergen Gross wrote:
> 
> No, I don't think this is a good idea. In the EINVAL or EBUSY case a
> simple Xen admin command might be enough to make the next call succeed.
> I don't want to disable pinning in this case.

Ok.

Acked-by: David Vrabel 

David
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Xen-devel] [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu

[Juergen Gross]

On 05/04/16 11:45, David Vrabel wrote:
> On 05/04/16 06:10, Juergen Gross wrote:
>> Some hardware models (e.g. Dell Studio 1555 laptops) require calls to
>> the firmware to be issued on cpu 0 only. As Dom0 might have to use
>> these calls, add xen_pin_vcpu() to achieve this functionality.
>>
>> In case either the domain doesn't have the privilege to make the
>> related hypercall or the hypervisor isn't supporting it, issue a
>> warning once and disable further pinning attempts.
> [...]
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -1885,6 +1885,45 @@ static void xen_set_cpu_features(struct cpuinfo_x86 
>> *c)
>>  }
>>  }
>>  
>> +static void xen_pin_vcpu(int cpu)
>> +{
>> +static bool disable_pinning;
>> +struct sched_pin_override pin_override;
>> +int ret;
>> +
>> +if (disable_pinning)
>> +return;
>> +
>> +pin_override.pcpu = cpu;
>> +ret = HYPERVISOR_sched_op(SCHEDOP_pin_override, _override);
> 
>   /* Ignore errors when removing override. */

Okay.

>> +if (cpu < 0)
>> +return;
>> +
>> +switch (ret) {
>> +case -ENOSYS:
>> +pr_warn("The kernel tried to call a function on physical cpu 
>> %d, but Xen isn't\n"
>> +"supporting this. In case of problems you might 
>> consider vcpu pinning.\n",
>> +cpu);
>> +disable_pinning = true;
>> +break;
>> +case -EPERM:
>> +WARN(1, "Trying to pin vcpu without having privilege to do 
>> so\n");
>> +disable_pinning = true;
>> +break;
>> +case -EINVAL:
>> +case -EBUSY:
>> +pr_warn("The kernel tried to call a function on physical cpu 
>> %d, but this cpu\n"
>> +"seems not to be available. Please check your Xen cpu 
>> configuration.\n",
>> +cpu);
>> +break;
>> +case 0:
>> +break;
>> +default:
>> +WARN(1, "rc %d while trying to pin vcpu\n", ret);
>> +disable_pinning = true;
>> +}
> 
> These messages are a bit wordy for my taste and since they don't say
> what function failed or what driver is affected they're not as useful as

Did you notice I used WARN() for the cases where a usage error is to
be suspected? This will print a stack backtrace helping to identify the
driver.

I can work on the message text, of course.

> they could be.  I'd probably turn these all into:
> 
>   if (cpu >= 0 && ret < 0) {
>   pr_warn("Failed to pin VCPU %d to physical CPU %d: %d",
>   smp_processor_id(), cpu, ret);
>   disable_pinning = true;
>   }

No, I don't think this is a good idea. In the EINVAL or EBUSY case a
simple Xen admin command might be enough to make the next call succeed.
I don't want to disable pinning in this case.

> And look at getting the user of this API to print a more useful error.
> 
> "i8k: unable to call SMM BIOS on physical CPU %d: %d"

TBH: I think this should be done by another patch. This is something
the maintainers of the callers' code should decide.


Juergen
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Xen-devel] [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu

[David Vrabel]

On 05/04/16 06:10, Juergen Gross wrote:
> Some hardware models (e.g. Dell Studio 1555 laptops) require calls to
> the firmware to be issued on cpu 0 only. As Dom0 might have to use
> these calls, add xen_pin_vcpu() to achieve this functionality.
> 
> In case either the domain doesn't have the privilege to make the
> related hypercall or the hypervisor isn't supporting it, issue a
> warning once and disable further pinning attempts.
[...]
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1885,6 +1885,45 @@ static void xen_set_cpu_features(struct cpuinfo_x86 *c)
>   }
>  }
>  
> +static void xen_pin_vcpu(int cpu)
> +{
> + static bool disable_pinning;
> + struct sched_pin_override pin_override;
> + int ret;
> +
> + if (disable_pinning)
> + return;
> +
> + pin_override.pcpu = cpu;
> + ret = HYPERVISOR_sched_op(SCHEDOP_pin_override, _override);

/* Ignore errors when removing override. */
> + if (cpu < 0)
> + return;
> +
> + switch (ret) {
> + case -ENOSYS:
> + pr_warn("The kernel tried to call a function on physical cpu 
> %d, but Xen isn't\n"
> + "supporting this. In case of problems you might 
> consider vcpu pinning.\n",
> + cpu);
> + disable_pinning = true;
> + break;
> + case -EPERM:
> + WARN(1, "Trying to pin vcpu without having privilege to do 
> so\n");
> + disable_pinning = true;
> + break;
> + case -EINVAL:
> + case -EBUSY:
> + pr_warn("The kernel tried to call a function on physical cpu 
> %d, but this cpu\n"
> + "seems not to be available. Please check your Xen cpu 
> configuration.\n",
> + cpu);
> + break;
> + case 0:
> + break;
> + default:
> + WARN(1, "rc %d while trying to pin vcpu\n", ret);
> + disable_pinning = true;
> + }

These messages are a bit wordy for my taste and since they don't say
what function failed or what driver is affected they're not as useful as
they could be.  I'd probably turn these all into:

if (cpu >= 0 && ret < 0) {
pr_warn("Failed to pin VCPU %d to physical CPU %d: %d",
smp_processor_id(), cpu, ret);
disable_pinning = true;
}

And look at getting the user of this API to print a more useful error.

"i8k: unable to call SMM BIOS on physical CPU %d: %d"

Or whatever.

David
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Xen-devel] [PATCH v4 1/6] xen: sync xen header

[David Vrabel]

On 05/04/16 06:10, Juergen Gross wrote:
> Import the actual version of include/xen/interface/sched.h from Xen.

Acked-by: David Vrabel 

David
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] VSOCK: Detach QP check should filter out non matching QPs.

[Jorgen Hansen]

The check in vmci_transport_peer_detach_cb should only allow a
detach when the qp handle of the transport matches the one in
the detach message.

Testing: Before this change, a detach from a peer on a different
socket would cause an active stream socket to register a detach.

Reviewed-by: George Zhang 
Signed-off-by: Jorgen Hansen 
---
 net/vmw_vsock/vmci_transport.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 0a369bb..662bdd2 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -842,7 +842,7 @@ static void vmci_transport_peer_detach_cb(u32 sub_id,
 * qp_handle.
 */
if (vmci_handle_is_invalid(e_payload->handle) ||
-   vmci_handle_is_equal(trans->qp_handle, e_payload->handle))
+   !vmci_handle_is_equal(trans->qp_handle, e_payload->handle))
return;
 
/* We don't ask for delayed CBs when we subscribe to this event (we
@@ -2154,7 +2154,7 @@ module_exit(vmci_transport_exit);
 
 MODULE_AUTHOR("VMware, Inc.");
 MODULE_DESCRIPTION("VMCI transport for Virtual Sockets");
-MODULE_VERSION("1.0.2.0-k");
+MODULE_VERSION("1.0.3.0-k");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("vmware_vsock");
 MODULE_ALIAS_NETPROTO(PF_VSOCK);
-- 
1.7.0

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 01/16] mm: use put_page to free page instead of putback_lru_page

[Vlastimil Babka]

On 04/05/2016 03:54 AM, Naoya Horiguchi wrote:
> On Mon, Apr 04, 2016 at 04:46:31PM +0200, Vlastimil Babka wrote:
>> On 04/04/2016 06:45 AM, Naoya Horiguchi wrote:
>>> On Mon, Apr 04, 2016 at 10:39:17AM +0900, Minchan Kim wrote:
> ...
>
> Also (but not your fault) the put_page() preceding
> test_set_page_hwpoison(page)) IMHO deserves a comment saying which
> pin we are releasing and which one we still have (hopefully? if I
> read description of da1b13ccfbebe right) otherwise it looks like
> doing something with a page that we just potentially freed.

 Yes, while I read the code, I had same question. I think the releasing
 refcount is for get_any_page.
>>>
>>> As the other callers of page migration do, soft_offline_page expects the
>>> migration source page to be freed at this put_page() (no pin remains.)
>>> The refcount released here is from isolate_lru_page() in 
>>> __soft_offline_page().
>>> (the pin by get_any_page is released by put_hwpoison_page just after it.)
>>>
>>> .. yes, doing something just after freeing page looks weird, but that's
>>> how PageHWPoison flag works. IOW, many other page flags are maintained
>>> only during one "allocate-free" life span, but PageHWPoison still does
>>> its job beyond it.
>>
>> But what prevents the page from being allocated again between put_page()
>> and test_set_page_hwpoison()? In that case we would be marking page
>> poisoned while still in use, which is the same as marking it while still
>> in use after a failed migration?
> 
> Actually nothing prevents that race. But I think that the result of the race
> is that the error page can be reused for allocation, which results in killing
> processes at page fault time. Soft offline is kind of mild/precautious thing
> (for correctable errors that don't require immediate handling), so killing
> processes looks to me an overkill. And marking hwpoison means that we can no
> longer do retry from userspace.

So you agree that this race is a bug? It may turn a soft-offline attempt
into a killed process. In that case we should fix it the same as we are
fixing the failed migration case. Maybe it will be just enough to switch
the test_set_page_hwpoison() and put_page() calls?

> And another practical thing is the race with unpoison_memory() as described
> in commit da1b13ccfbebe. unpoison_memory() properly works only for properly
> poisoned pages, so doing unpoison for in-use hwpoisoned pages is fragile.
> That's why I'd like to avoid setting PageHWPoison for in-use pages if 
> possible.
> 
>> (Also, which part prevents pages with PageHWPoison to be allocated
>> again, anyway? I can't find it and test_set_page_hwpoison() doesn't
>> remove from buddy freelists).
> 
> check_new_page() in mm/page_alloc.c should prevent reallocation of 
> PageHWPoison.
> As you pointed out, memory error handler doens't remove it from buddy 
> freelists.

Oh, I see. It's using __PG_HWPOISON wrapper, so I didn't notice it when
searching. In any case that results in a bad_page() warning, right? Is
it desirable for a soft-offlined page? If we didn't free poisoned pages
to buddy system, they wouldn't trigger this warning.

> BTW, it might be a bit off-topic, but recently I felt that check_new_page()
> might be improvable, because when check_new_page() returns 1, the whole buddy
> block (not only the bad page) seems to be leaked from buddy freelist.
> For example, if thp (order 9) is requested, and PageHWPoison (or any other
> types of bad pages) is found in an order 9 block, all 512 page are discarded.
> Unpoison can't bring it back to buddy.
> So, some code to split buddy block including bad page (and recovering code 
> from
> unpoison) might be helpful, although that's another story ...

Hm sounds like another argument for not freeing the page to buddy lists
in the first place. Maybe a hook in free_pages_check()?

> Thanks,
> Naoya Horiguchi
> 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 3/6] smp: add function to execute a function synchronously on a cpu

[Peter Zijlstra]

On Tue, Apr 05, 2016 at 07:10:04AM +0200, Juergen Gross wrote:
> +int smp_call_on_cpu(unsigned int cpu, bool pin, int (*func)(void *), void 
> *par)

Why .pin and not .phys? .pin does not (to me) reflect the
hypervisor/physical-cpu thing.

Also, as per smp_call_function_single() would it not be more consistent
to make this the last argument?

> +{
> + struct smp_call_on_cpu_struct sscs = {
> + .work = __WORK_INITIALIZER(sscs.work, smp_call_on_cpu_callback),
> + .done = COMPLETION_INITIALIZER_ONSTACK(sscs.done),
> + .func = func,
> + .data = par,
> + .cpu  = pin ? cpu : -1,
> + };
> +
> + if (cpu >= nr_cpu_ids)

You might want to also include cpu_online().

if (cpu >= nr_cpu_ids || !cpu_online(cpu))
> + return -ENXIO;

Seeing how its fairly hard to schedule work on a cpu that's not actually
there.

> +
> + queue_work_on(cpu, system_wq, );
> + wait_for_completion();
> +
> + return sscs.ret;
> +}


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio: fix "warning: ‘queue’ may be used uninitialized"

[Michael S. Tsirkin]

On Mon, Apr 04, 2016 at 02:14:19PM -0400, Jeff Mahoney wrote:
> This fixes the following warning:
> drivers/virtio/virtio_ring.c:1032:5: warning: ‘queue’ may be used
> uninitialized in this function
> 
> The conditions that govern when queue is set aren't apparent to gcc.
> 
> Setting queue = NULL clears the warning.
> 
> Signed-off-by: Jeff Mahoney 

Which gcc version produces this warning?
I do not seem to see it with gcc 5.3.1.
Also - use uninitialized_var then?

> ---
> 
>  drivers/virtio/virtio_ring.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1006,7 +1006,7 @@ struct virtqueue *vring_create_virtqueue
>   const char *name)
>  {
>   struct virtqueue *vq;
> - void *queue;
> + void *queue = NULL;
>   dma_addr_t dma_addr;
>   size_t queue_size_in_bytes;
>   struct vring vring;
> 
> -- 
> Jeff Mahoney
> SUSE Labs
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization