* Re: [PATCH -next] xen: fix header export to userspace
From: Tony Finch @ 2010-11-15 10:23 UTC (permalink / raw)
To: Randy Dunlap
Cc: Stephen Rothwell, akpm, linux-next, LKML, Jeremy Fitzhardinge,
Konrad Rzeszutek Wilk, xen-devel, virtualization
In-Reply-To: <20101113084439.d21bd0f0.randy.dunlap@oracle.com>
On Sat, 13 Nov 2010, Randy Dunlap wrote:
> From: Randy Dunlap <randy.dunlap@oracle.com>
>
> scripts/headers_install.pl prevents "__user" from being exported
> to userspace headers, so just use compiler.h to make sure that
> __user is defined and avoid the error.
>
> unifdef: linux-next-20101112/xx64/usr/include/xen/privcmd.h.tmp: 79: Premature EOF (#if line 33 depth 1)
I suggest the following slightly more informative message
> scripts/headers_install.pl strips "__user" when exporting headers to
> userspace and can introduce a syntax error (detected by unifdef) if
> "__user" appears in an unexpected place. Simplify by using compiler.h to
> make sure that __user is defined and avoid the error.
>
> unifdef: linux-next-20101112/xx64/usr/include/xen/privcmd.h.tmp: 79: Premature EOF (#if line 33 depth 1)
In any case,
Acked-By: Tony Finch <dot@dotat.at>
Tony.
--
f.anthony.n.finch <dot@dotat.at> http://dotat.at/
HUMBER THAMES DOVER WIGHT PORTLAND: NORTH BACKING WEST OR NORTHWEST, 5 TO 7,
DECREASING 4 OR 5, OCCASIONALLY 6 LATER IN HUMBER AND THAMES. MODERATE OR
ROUGH. RAIN THEN FAIR. GOOD.
^ permalink raw reply
* [PATCH 31/44] drivers/xen: Remove unnecessary semicolons
From: Joe Perches @ 2010-11-15 3:04 UTC (permalink / raw)
To: Jiri Kosina
Cc: virtualization, Jeremy Fitzhardinge, xen-devel, linux-kernel,
Konrad Rzeszutek Wilk
In-Reply-To: <cover.1289789604.git.joe@perches.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/xen/swiotlb-xen.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 54469c3..65ea21a 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -54,7 +54,7 @@ u64 start_dma_addr;
static dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
{
- return phys_to_machine(XPADDR(paddr)).maddr;;
+ return phys_to_machine(XPADDR(paddr)).maddr;
}
static phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
--
1.7.3.1.g432b3.dirty
^ permalink raw reply related
* [PATCH 00/44] remove unnecessary semicolons
From: Joe Perches @ 2010-11-15 3:04 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-arm-kernel, linux-kernel, linux-tegra, microblaze-uclinux,
user-mode-linux-devel, user-mode-linux-user, cpufreq, linux-i2c,
netdev, linux-media, linux-mmc, e1000-devel, linux-wireless,
ath9k-devel, platform-driver-x86, ibm-acpi-devel, linux-s390,
linux-scsi, spi-devel-general, devel, linux-usb, xen-devel,
virtualization, v9fs-developer, ceph-devel, logfs, linux-nfs,
ocfs2-devel
ya trivial series...
Joe Perches (44):
arch/arm: Remove unnecessary semicolons
arch/microblaze: Remove unnecessary semicolons
arch/um: Remove unnecessary semicolons
drivers/cpufreq: Remove unnecessary semicolons
drivers/gpio: Remove unnecessary semicolons
drivers/i2c: Remove unnecessary semicolons
drivers/isdn: Remove unnecessary semicolons
drivers/leds: Remove unnecessary semicolons
drivers/media/video: Remove unnecessary semicolons
drivers/misc: Remove unnecessary semicolons
drivers/mmc: Remove unnecessary semicolons
drivers/net/bnx2x: Remove unnecessary semicolons
drivers/net/e1000e: Remove unnecessary semicolons
drivers/net/ixgbe: Remove unnecessary semicolons
drivers/net/vxge: Remove unnecessary semicolons
drivers/net/wireless/ath: Remove unnecessary semicolons
drivers/net/wireless/iwlwifi: Remove unnecessary semicolons
drivers/net/cnic.c: Remove unnecessary semicolons
drivers/platform/x86: Remove unnecessary semicolons
drivers/power: Remove unnecessary semicolons
drivers/s390/net: Remove unnecessary semicolons
drivers/scsi/be2iscsi: Remove unnecessary semicolons
drivers/scsi/bfa: Remove unnecessary semicolons
drivers/scsi/lpfc: Remove unnecessary semicolons
drivers/scsi/pm8001: Remove unnecessary semicolons
drivers/scsi/qla2xxx: Remove unnecessary semicolons
drivers/serial: Remove unnecessary semicolons
drivers/spi: Remove unnecessary semicolons
drivers/staging: Remove unnecessary semicolons
drivers/usb/gadget: Remove unnecessary semicolons
drivers/xen: Remove unnecessary semicolons
fs/9p: Remove unnecessary semicolons
fs/ceph: Remove unnecessary semicolons
fs/logfs: Remove unnecessary semicolons
fs/nfs: Remove unnecessary semicolons
fs/ocfs2: Remove unnecessary semicolons
fs/ubifs: Remove unnecessary semicolons
include/linux/if_macvlan.h: Remove unnecessary semicolons
include/net/caif/cfctrl.h: Remove unnecessary semicolons
mm/hugetlb.c: Remove unnecessary semicolons
net/ipv6/mcast.c: Remove unnecessary semicolons
net/sunrpc/addr.c: Remove unnecessary semicolons
sound/core/pcm_lib.c: Remove unnecessary semicolons
sound/soc/codecs: Remove unnecessary semicolons
arch/arm/mach-at91/at91cap9_devices.c | 2 +-
arch/arm/mach-at91/at91sam9g45_devices.c | 2 +-
arch/arm/mach-at91/at91sam9rl_devices.c | 2 +-
arch/arm/mach-nuc93x/time.c | 2 +-
arch/arm/mach-tegra/tegra2_clocks.c | 2 +-
arch/arm/mach-w90x900/cpu.c | 2 +-
arch/arm/plat-mxc/irq.c | 2 +-
arch/microblaze/lib/memmove.c | 2 +-
arch/um/drivers/mmapper_kern.c | 2 +-
drivers/cpufreq/cpufreq_conservative.c | 2 +-
drivers/gpio/langwell_gpio.c | 2 +-
drivers/i2c/busses/i2c-designware.c | 2 +-
drivers/isdn/hardware/mISDN/mISDNinfineon.c | 4 ++--
drivers/isdn/hardware/mISDN/mISDNisar.c | 2 +-
drivers/leds/leds-mc13783.c | 2 +-
drivers/media/video/cx88/cx88-blackbird.c | 2 +-
drivers/media/video/davinci/vpfe_capture.c | 2 +-
drivers/media/video/em28xx/em28xx-cards.c | 2 +-
drivers/misc/bmp085.c | 2 +-
drivers/misc/isl29020.c | 2 +-
drivers/mmc/host/davinci_mmc.c | 2 +-
drivers/net/bnx2x/bnx2x_link.c | 4 ++--
drivers/net/bnx2x/bnx2x_main.c | 2 +-
drivers/net/cnic.c | 2 +-
drivers/net/e1000e/netdev.c | 2 +-
drivers/net/ixgbe/ixgbe_sriov.c | 2 +-
drivers/net/vxge/vxge-main.c | 2 +-
drivers/net/wireless/ath/ath9k/htc.h | 2 +-
drivers/net/wireless/iwlwifi/iwl-agn.c | 2 +-
drivers/platform/x86/classmate-laptop.c | 2 +-
drivers/platform/x86/thinkpad_acpi.c | 2 +-
drivers/power/intel_mid_battery.c | 2 +-
drivers/s390/net/qeth_core_sys.c | 2 +-
drivers/scsi/be2iscsi/be_main.c | 4 ++--
drivers/scsi/bfa/bfa_fcs_lport.c | 2 +-
drivers/scsi/lpfc/lpfc_bsg.c | 2 +-
drivers/scsi/pm8001/pm8001_init.c | 2 +-
drivers/scsi/qla2xxx/qla_isr.c | 4 ++--
drivers/scsi/qla2xxx/qla_nx.c | 2 +-
drivers/serial/mrst_max3110.c | 2 +-
drivers/spi/amba-pl022.c | 2 +-
drivers/spi/spi_nuc900.c | 2 +-
.../staging/ath6kl/hif/sdio/linux_sdio/src/hif.c | 2 +-
drivers/staging/ath6kl/os/linux/ar6000_drv.c | 2 +-
drivers/staging/bcm/InterfaceInit.c | 2 +-
drivers/staging/bcm/InterfaceIsr.c | 2 +-
drivers/staging/bcm/Misc.c | 4 ++--
.../comedi/drivers/addi-data/APCI1710_Tor.c | 2 +-
.../comedi/drivers/addi-data/hwdrv_apci1500.c | 2 +-
.../comedi/drivers/addi-data/hwdrv_apci1516.c | 2 +-
.../comedi/drivers/addi-data/hwdrv_apci3501.c | 2 +-
drivers/staging/comedi/drivers/amplc_pci230.c | 2 +-
drivers/staging/comedi/drivers/cb_das16_cs.c | 2 +-
drivers/staging/comedi/drivers/comedi_bond.c | 2 +-
drivers/staging/crystalhd/crystalhd_hw.c | 2 +-
drivers/staging/go7007/go7007-driver.c | 2 +-
drivers/staging/iio/accel/lis3l02dq_ring.c | 2 +-
.../staging/intel_sst/intel_sst_drv_interface.c | 4 ++--
drivers/staging/keucr/smilmain.c | 4 ++--
drivers/staging/keucr/smilsub.c | 4 ++--
drivers/staging/msm/lcdc_toshiba_wvga_pt.c | 2 +-
drivers/staging/rt2860/common/cmm_data_pci.c | 4 ++--
drivers/staging/rt2860/rt_linux.c | 2 +-
drivers/staging/rt2860/rtmp.h | 2 +-
drivers/staging/rtl8192e/ieee80211/ieee80211_tx.c | 2 +-
drivers/staging/rtl8192e/r819xE_phy.c | 2 +-
drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c | 2 +-
drivers/staging/rtl8192u/r8192U_core.c | 2 +-
drivers/staging/rtl8192u/r819xU_phy.c | 2 +-
drivers/staging/rtl8712/rtl8712_efuse.c | 2 +-
drivers/staging/rtl8712/rtl8712_xmit.c | 2 +-
drivers/staging/rtl8712/rtl871x_xmit.c | 2 +-
drivers/staging/tidspbridge/core/tiomap3430.c | 4 ++--
drivers/staging/tidspbridge/rmgr/nldr.c | 2 +-
drivers/staging/vt6655/card.c | 2 +-
drivers/staging/vt6655/iwctl.c | 2 +-
drivers/staging/vt6655/wpa2.c | 4 ++--
drivers/staging/vt6656/baseband.c | 2 +-
drivers/staging/vt6656/iwctl.c | 2 +-
drivers/staging/vt6656/power.c | 2 +-
drivers/staging/vt6656/wpa2.c | 4 ++--
drivers/usb/gadget/f_fs.c | 2 +-
drivers/xen/swiotlb-xen.c | 2 +-
fs/9p/acl.c | 2 +-
fs/9p/xattr.c | 2 +-
fs/ceph/mds_client.c | 2 +-
fs/logfs/readwrite.c | 2 +-
fs/nfs/getroot.c | 2 +-
fs/ocfs2/refcounttree.c | 2 +-
fs/ubifs/scan.c | 2 +-
include/linux/if_macvlan.h | 2 +-
include/net/caif/cfctrl.h | 2 +-
mm/hugetlb.c | 2 +-
net/ipv6/mcast.c | 2 +-
net/sunrpc/addr.c | 2 +-
sound/core/pcm_lib.c | 2 +-
sound/soc/codecs/wm8904.c | 2 +-
sound/soc/codecs/wm8940.c | 1 -
sound/soc/codecs/wm8993.c | 2 +-
sound/soc/codecs/wm_hubs.c | 2 +-
100 files changed, 111 insertions(+), 112 deletions(-)
--
1.7.3.1.g432b3.dirty
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Dor Laor @ 2010-11-14 10:46 UTC (permalink / raw)
To: Ky Srinivasan; +Cc: devel, Virtualization, Haiyang Zhang, Greg KH
In-Reply-To: <4CDBE98E02000030000902C9@novprvoes0310.provo.novell.com>
On 11/11/2010 10:03 PM, Ky Srinivasan wrote:
> + * An implementation of key value pair (KVP) functionality for Linux.
> + *
> + *
> + * Copyright (C) 2010, Novell, Inc.
> + * Author : K. Y. Srinivasan<ksrinivasan@novell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT. See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +
> +
> +#include<linux/net.h>
> +#include<linux/nls.h>
> +#include<linux/connector.h>
> +
> +#include "logging.h"
> +#include "osd.h"
> +#include "vmbus.h"
> +#include "vmbus_packet_format.h"
> +#include "vmbus_channel_interface.h"
> +#include "version_info.h"
> +#include "channel.h"
> +#include "vmbus_private.h"
> +#include "vmbus_api.h"
> +#include "utils.h"
> +#include "kvp.h"
> +
> +
> +/*
> + *
> + * The following definitions are shared with the user-mode component; do not
> + * change any of this without making the corresponding changes in
> + * the KVP user-mode component.
> + */
> +
> +#define CN_KVP_VAL 0x1 /* This supports queries from the kernel */
> +#define CN_KVP_USER_VAL 0x2 /* This supports queries from the user */
> +
> +
> +/*
> + * KVP protocol: The user mode component first registers with the
Is there a spec that describes all of the possible messages?
For Linux virtio we use it in parallel to the code to make sure all
potential guest OS will follow the same standard.
What about live migration semantics like migrate while transaction is
on? and having the guest aware of the migration so the host data will be
updated?
..
> main(void)
> {
> int fd, len, sock_opt;
> int error;
> struct cn_msg *message;
> struct pollfd pfd;
> struct nlmsghdr *incoming_msg;
> struct cn_msg *incoming_cn_msg;
> char *key_value;
> kvp_key_name_t key_name;
>
> daemon(1, 0);
> openlog("KVP", 0, LOG_USER);
> syslog(LOG_INFO, "KVP starting; pid is:%d", getpid());
> /*
> * Retrieve OS release information.
> */
> kvp_get_os_info();
>
> fd = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);
This is nice. One of the first tried for virtio-serial was to use
netlink. We even wanted unix_domain socket for AF_VIRT that will reach
the host but davem nacked it.
^ permalink raw reply
* Re: [PATCH 03/20] x86/ticketlock: Use C for __ticket_spin_unlock
From: Paolo Bonzini @ 2010-11-13 22:34 UTC (permalink / raw)
To: Américo Wang
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Srivatsa Vaddagiri,
Linux Kernel Mailing List, Jan Beulich, Linux Virtualization,
Jeremy Fitzhardinge, Avi Kivity, H. Peter Anvin
In-Reply-To: <20101113100527.GG3837@hack>
On 11/13/2010 11:05 AM, Américo Wang wrote:
> On Wed, Nov 03, 2010 at 10:59:44AM -0400, Jeremy Fitzhardinge wrote:
>> * On PPro SMP or if we are using OOSTORE, we use a locked operation to unlock
>> * (PPro errata 66, 92)
>> */
>> -# define UNLOCK_LOCK_PREFIX LOCK_PREFIX
>> +static __always_inline void __ticket_unlock_release(struct arch_spinlock *lock)
>> +{
>> + if (sizeof(lock->tickets.head) == sizeof(u8))
>> + asm (LOCK_PREFIX "incb %0"
>> + : "+m" (lock->tickets.head) : : "memory");
>> + else
>> + asm (LOCK_PREFIX "incw %0"
>> + : "+m" (lock->tickets.head) : : "memory");
>
> This 'if/else' really should be done with #ifdef, even though
> the compiler may be smart enough to remove it.
That's a sure path to bitrot.
Paolo
^ permalink raw reply
* [PATCH -next] xen: fix header export to userspace
From: Randy Dunlap @ 2010-11-13 16:44 UTC (permalink / raw)
To: Stephen Rothwell, akpm
Cc: xen-devel, Jeremy, Fitzhardinge, Konrad Rzeszutek Wilk,
Tony Finch, LKML, virtualization, linux-next
In-Reply-To: <20101112130547.c2a3c2c1.sfr@canb.auug.org.au>
From: Randy Dunlap <randy.dunlap@oracle.com>
scripts/headers_install.pl prevents "__user" from being exported
to userspace headers, so just use compiler.h to make sure that
__user is defined and avoid the error.
unifdef: linux-next-20101112/xx64/usr/include/xen/privcmd.h.tmp: 79: Premature EOF (#if line 33 depth 1)
Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: xen-devel@lists.xensource.com (moderated for non-subscribers)
Cc: virtualization@lists.osdl.org
Cc: Tony Finch <dot@dotat.at>
---
include/xen/privcmd.h | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
--- linux-next-20101112.orig/include/xen/privcmd.h
+++ linux-next-20101112/include/xen/privcmd.h
@@ -34,13 +34,10 @@
#define __LINUX_PUBLIC_PRIVCMD_H__
#include <linux/types.h>
+#include <linux/compiler.h>
typedef unsigned long xen_pfn_t;
-#ifndef __user
-#define __user
-#endif
-
struct privcmd_hypercall {
__u64 op;
__u64 arg[5];
^ permalink raw reply
* Re: [PATCH 06/20] x86/ticketlock: make __ticket_spin_trylock common
From: Eric Dumazet @ 2010-11-13 10:48 UTC (permalink / raw)
To: Américo Wang
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Srivatsa, Linux,
Linux Kernel Mailing List, Jan Beulich, Virtualization,
Jeremy Fitzhardinge, Vaddagiri, Avi Kivity, H. Peter Anvin
In-Reply-To: <20101113101732.GH3837@hack>
Le samedi 13 novembre 2010 à 18:17 +0800, Américo Wang a écrit :
> On Wed, Nov 03, 2010 at 10:59:47AM -0400, Jeremy Fitzhardinge wrote:
>
> >+ union {
> >+ struct __raw_tickets tickets;
> >+ __ticketpair_t slock;
> >+ } tmp, new;
> >+ int ret;
> >+
> >+ tmp.tickets = ACCESS_ONCE(lock->tickets);
> >+ if (tmp.tickets.head != tmp.tickets.tail)
> >+ return 0;
> >+
> >+ new.slock = tmp.slock + (1 << TICKET_SHIFT);
> >+
> >+ ret = cmpxchg(&lock->ticketpair, tmp.slock, new.slock) == tmp.slock;
> >+ barrier(); /* just make nothing creeps before lock is claimed */
>
> This one should be smp_wmb(), right? No CONFIG_X86_OOSTORE protected.
cmpxchg() is a full memory barrier, no need for smp_wmb() or barrier()
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 06/20] x86/ticketlock: make __ticket_spin_trylock common
From: Américo Wang @ 2010-11-13 10:17 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Srivatsa Vaddagiri,
Linux Kernel Mailing List, Jan Beulich, Linux Virtualization,
Jeremy Fitzhardinge, Avi Kivity, H. Peter Anvin
In-Reply-To: <4cea7b0804de5a2c1249034fc5c4840ade043ff4.1288794124.git.jeremy.fitzhardinge@citrix.com>
On Wed, Nov 03, 2010 at 10:59:47AM -0400, Jeremy Fitzhardinge wrote:
>+ union {
>+ struct __raw_tickets tickets;
>+ __ticketpair_t slock;
>+ } tmp, new;
>+ int ret;
>+
>+ tmp.tickets = ACCESS_ONCE(lock->tickets);
>+ if (tmp.tickets.head != tmp.tickets.tail)
>+ return 0;
>+
>+ new.slock = tmp.slock + (1 << TICKET_SHIFT);
>+
>+ ret = cmpxchg(&lock->ticketpair, tmp.slock, new.slock) == tmp.slock;
>+ barrier(); /* just make nothing creeps before lock is claimed */
This one should be smp_wmb(), right? No CONFIG_X86_OOSTORE protected.
^ permalink raw reply
* Re: [PATCH 03/20] x86/ticketlock: Use C for __ticket_spin_unlock
From: Américo Wang @ 2010-11-13 10:05 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Srivatsa Vaddagiri,
Linux Kernel Mailing List, Jan Beulich, Linux Virtualization,
Jeremy Fitzhardinge, Avi Kivity, H. Peter Anvin
In-Reply-To: <20092775a9df07a5a75820ac250194b535279d51.1288794124.git.jeremy.fitzhardinge@citrix.com>
On Wed, Nov 03, 2010 at 10:59:44AM -0400, Jeremy Fitzhardinge wrote:
> * On PPro SMP or if we are using OOSTORE, we use a locked operation to unlock
> * (PPro errata 66, 92)
> */
>-# define UNLOCK_LOCK_PREFIX LOCK_PREFIX
>+static __always_inline void __ticket_unlock_release(struct arch_spinlock *lock)
>+{
>+ if (sizeof(lock->tickets.head) == sizeof(u8))
>+ asm (LOCK_PREFIX "incb %0"
>+ : "+m" (lock->tickets.head) : : "memory");
>+ else
>+ asm (LOCK_PREFIX "incw %0"
>+ : "+m" (lock->tickets.head) : : "memory");
This 'if/else' really should be done with #ifdef, even though
the compiler may be smart enough to remove it.
>+
>+}
> #else
>-# define UNLOCK_LOCK_PREFIX
>+static __always_inline void __ticket_unlock_release(struct arch_spinlock *lock)
>+{
>+ barrier();
>+ lock->tickets.head++;
>+ barrier();
The second barrier() is not needed.
Thanks.
^ permalink raw reply
* Re: [PATCH 01/20] x86/ticketlock: clean up types and accessors
From: Américo Wang @ 2010-11-13 9:57 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Srivatsa Vaddagiri,
Linux Kernel Mailing List, Jan Beulich, Linux Virtualization,
Jeremy Fitzhardinge, Avi Kivity, H. Peter Anvin
In-Reply-To: <742be99eb6d64784dbc3e446d22df78b7a55fc0c.1288794124.git.jeremy.fitzhardinge@citrix.com>
On Wed, Nov 03, 2010 at 10:59:42AM -0400, Jeremy Fitzhardinge wrote:
...
> static __always_inline void __ticket_spin_lock(arch_spinlock_t *lock)
> {
>- int inc = 0x00010000;
>- int tmp;
>+ unsigned inc = 1 << TICKET_SHIFT;
>+ unsigned tmp;
Please don't use old implicit-int.
>
>- return (((tmp >> TICKET_SHIFT) - tmp) & ((1 << TICKET_SHIFT) - 1)) > 1;
>+ return ((tmp.tail - tmp.head) & TICKET_MASK) > 1;
There is a type promotion here.
> }
>
> #ifndef CONFIG_PARAVIRT_SPINLOCKS
>diff --git a/arch/x86/include/asm/spinlock_types.h b/arch/x86/include/asm/spinlock_types.h
>index dcb48b2..4582640 100644
>--- a/arch/x86/include/asm/spinlock_types.h
>+++ b/arch/x86/include/asm/spinlock_types.h
>@@ -5,11 +5,27 @@
> # error "please don't include this file directly"
> #endif
>
>+#include <linux/types.h>
>+
>+#if (CONFIG_NR_CPUS < 256)
>+typedef u8 __ticket_t;
>+#else
>+typedef u16 __ticket_t;
>+#endif
>+
>+#define TICKET_SHIFT (sizeof(__ticket_t) * 8)
>+#define TICKET_MASK ((1 << TICKET_SHIFT) - 1)
So here you may need to cast the result to __ticket_t.
Thanks.
^ permalink raw reply
* Re: [PATCH 00/20] x86: ticket lock rewrite and paravirtualization
From: H. Peter Anvin @ 2010-11-12 22:20 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Srivatsa Vaddagiri,
Linux Kernel Mailing List, Jan Beulich, Linux Virtualization,
Avi Kivity
In-Reply-To: <4CDDBCE4.80906@goop.org>
On 11/12/2010 02:17 PM, Jeremy Fitzhardinge wrote:
> On 11/12/2010 02:12 PM, H. Peter Anvin wrote:
>> On 11/03/2010 07:59 AM, Jeremy Fitzhardinge wrote:
>>> - with an unmodified struct spinlock, it can check to see if
>>> head == tail after unlock; if not, then there's someone else
>>> trying to lock, and we can do a kick. Unfortunately this
>>> generates very high level of redundant kicks, because the
>>> waiting CPU might not have blocked yet (which is the common
>>> case)
>>>
>> How high is "very high" here -- most of the time (so that any mitigation
>> on the slow patch is useless)?
>
> I'll need to remeasure, but I think around 90% of the slowpath entries
> were spurious without this. In other words, when spinlocks do contend,
> most of the time it isn't very serious and the other cpu doesn't spend
> much time spinning.
>
90% of the slowpath entries is one thing, my real question is the
fraction of fastpath entries that get diverted to the slowpath. It
affects where mitigation needs to happen.
-hpa
^ permalink raw reply
* Re: [PATCH 00/20] x86: ticket lock rewrite and paravirtualization
From: Jeremy Fitzhardinge @ 2010-11-12 22:17 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Srivatsa Vaddagiri,
Linux Kernel Mailing List, Jan Beulich, Linux Virtualization,
Avi Kivity
In-Reply-To: <4CDDBBD3.5050903@zytor.com>
On 11/12/2010 02:12 PM, H. Peter Anvin wrote:
> On 11/03/2010 07:59 AM, Jeremy Fitzhardinge wrote:
>> - with an unmodified struct spinlock, it can check to see if
>> head == tail after unlock; if not, then there's someone else
>> trying to lock, and we can do a kick. Unfortunately this
>> generates very high level of redundant kicks, because the
>> waiting CPU might not have blocked yet (which is the common
>> case)
>>
> How high is "very high" here -- most of the time (so that any mitigation
> on the slow patch is useless)?
I'll need to remeasure, but I think around 90% of the slowpath entries
were spurious without this. In other words, when spinlocks do contend,
most of the time it isn't very serious and the other cpu doesn't spend
much time spinning.
J
^ permalink raw reply
* Re: [PATCH 00/20] x86: ticket lock rewrite and paravirtualization
From: H. Peter Anvin @ 2010-11-12 22:12 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Srivatsa Vaddagiri,
Linux Kernel Mailing List, Jan Beulich, Linux Virtualization,
Jeremy Fitzhardinge, Avi Kivity
In-Reply-To: <cover.1288794124.git.jeremy.fitzhardinge@citrix.com>
On 11/03/2010 07:59 AM, Jeremy Fitzhardinge wrote:
>
> - with an unmodified struct spinlock, it can check to see if
> head == tail after unlock; if not, then there's someone else
> trying to lock, and we can do a kick. Unfortunately this
> generates very high level of redundant kicks, because the
> waiting CPU might not have blocked yet (which is the common
> case)
>
How high is "very high" here -- most of the time (so that any mitigation
on the slow patch is useless)?
-hpa
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Greg KH @ 2010-11-12 21:38 UTC (permalink / raw)
To: Ky Srinivasan; +Cc: devel, Virtualization, Haiyang Zhang, Greg KH
In-Reply-To: <4CDD484E02000030000903E9@novprvoes0310.provo.novell.com>
On Fri, Nov 12, 2010 at 01:59:42PM -0700, Ky Srinivasan wrote:
>
>
> >>> On 11/12/2010 at 1:47 PM, in message <20101112184753.GA20893@kroah.com>, Greg
> KH <greg@kroah.com> wrote:
> > On Fri, Nov 12, 2010 at 11:06:18AM -0700, Ky Srinivasan wrote:
> >> >> +typedef struct kvp_msg {
> >> >> + __u32 kvp_key; /* Key */
> >> >> + __u8 kvp_value[0]; /* Corresponding value */
> >> >> +} kvp_msg_t;
> >> >
> >> > I thought that kvp_value was really KVP_VALUE_SIZE?
> >>
> >> kvp_value is typed information and KVP_VALUE_SIZE specifies the
> >> maximum size of the supported value. For instance if kvp_value is a
> >> string (which is the case for all the values currently supported),
> >> KVP_VALUE_SIZE specifies the maximum size of the string that will be
> >> supported.
> >
> > So it's a variable length structure? How do you konw how long the
> > structure is, does that depend on the key?
>
> kvp_value is a null terminated string. In the current implementation;
> the kernel component sends an index (key) to the user mode and
> receives the corresponding value - a string.
Why does the kernel care about the string at all?
> >> > And no typedefs, you did run your code through checkpatch.pl, right?
> >> > Why ignore the stuff it spits back at you?
> >> I will fix this.
> >> >
> >> >
> >> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> >> >> + "IntegrationServicesVersion",
> >> >> + "NetworkAddressIPv4",
> >> >> + "NetworkAddressIPv6",
> >> >> + "OSBuildNumber",
> >> >> + "OSName",
> >> >> + "OSMajorVersion",
> >> >> + "OSMinorVersion",
> >> >> + "OSVersion",
> >> >> + "ProcessorArchitecture",
> >> >> + };
> >> >
> >> > Why list these at all, as more might come in the future, and the kernel
> >> > really doesn't care about them, right?
> >> The core HyperV KVP protocol is such that the host iterates through an
> >> index (starting with 0); the guest is free to associate a key with the
> >> index and return the associated value. The host side iteration is
> >> stopped when the guest returns a failure on an index. MSFT has
> >> specified the keys and their ordinal value in their KVP specification.
> >> The array you see above is part of that specification.
> >
> > So you match on the string name of the key? I'm confused, as I didn't
> > think I saw you matching on the string name, only the key value.
> >
> > Also, as the kernel isn't handling the key type (with one exception),
> > why even list these in the kernel at all? I'm all for documenting
> > stuff, but don't use code memory for documentation :)
>
> When we respond back to the host, we need to specify the key for
> which the value is being returned. Note that the host only iterates
> over an integer key space while the guest is expected to return both
> the key name (guest is free to associate the key name with the integer
> index) and the corresponding value.
Ah, to verify that the guest really did know what the key value that was
being used was for? Odd way of verification, but I guess it works.
> I use, the array of strings kvp_keys[] to implement the mapping
> between the integer index and the key name. Look at the function
> kvp_respond_to_host() where we lookup the kvp_keys[] array prior to
> responding back to the host.
>
> In the next iteration of this patch, I am thinking of moving index to
> key mapping code into the user-level daemon, so the kernel side will
> not have to be modified as the key-space expands (in the future).
Yes, I would recommend that so the kernel would not have to be modified
for every time the hyperv kernel is also changed :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Ky Srinivasan @ 2010-11-12 20:59 UTC (permalink / raw)
To: Greg KH; +Cc: devel, Virtualization, Haiyang Zhang, Greg KH
In-Reply-To: <20101112184753.GA20893@kroah.com>
>>> On 11/12/2010 at 1:47 PM, in message <20101112184753.GA20893@kroah.com>, Greg
KH <greg@kroah.com> wrote:
> On Fri, Nov 12, 2010 at 11:06:18AM -0700, Ky Srinivasan wrote:
>> >> +typedef struct kvp_msg {
>> >> + __u32 kvp_key; /* Key */
>> >> + __u8 kvp_value[0]; /* Corresponding value */
>> >> +} kvp_msg_t;
>> >
>> > I thought that kvp_value was really KVP_VALUE_SIZE?
>>
>> kvp_value is typed information and KVP_VALUE_SIZE specifies the
>> maximum size of the supported value. For instance if kvp_value is a
>> string (which is the case for all the values currently supported),
>> KVP_VALUE_SIZE specifies the maximum size of the string that will be
>> supported.
>
> So it's a variable length structure? How do you konw how long the
> structure is, does that depend on the key?
kvp_value is a null terminated string. In the current implementation; the kernel component sends an index (key) to the user mode and receives the corresponding value - a string.
>
>> > And no typedefs, you did run your code through checkpatch.pl, right?
>> > Why ignore the stuff it spits back at you?
>> I will fix this.
>> >
>> >
>> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> >> + "IntegrationServicesVersion",
>> >> + "NetworkAddressIPv4",
>> >> + "NetworkAddressIPv6",
>> >> + "OSBuildNumber",
>> >> + "OSName",
>> >> + "OSMajorVersion",
>> >> + "OSMinorVersion",
>> >> + "OSVersion",
>> >> + "ProcessorArchitecture",
>> >> + };
>> >
>> > Why list these at all, as more might come in the future, and the kernel
>> > really doesn't care about them, right?
>> The core HyperV KVP protocol is such that the host iterates through an
>> index (starting with 0); the guest is free to associate a key with the
>> index and return the associated value. The host side iteration is
>> stopped when the guest returns a failure on an index. MSFT has
>> specified the keys and their ordinal value in their KVP specification.
>> The array you see above is part of that specification.
>
> So you match on the string name of the key? I'm confused, as I didn't
> think I saw you matching on the string name, only the key value.
>
> Also, as the kernel isn't handling the key type (with one exception),
> why even list these in the kernel at all? I'm all for documenting
> stuff, but don't use code memory for documentation :)
When we respond back to the host, we need to specify the key for which the value is being returned. Note that the host only iterates over an integer key space while the guest is expected to return both the key name (guest is free to associate the key name with the integer index) and the corresponding value. I use, the array of strings kvp_keys[] to implement the mapping between the integer index and the key name. Look at the function kvp_respond_to_host() where we lookup the kvp_keys[] array prior to responding back to the host.
In the next iteration of this patch, I am thinking of moving index to key mapping code into the user-level daemon, so the kernel side will not have to be modified as the key-space expands (in the future).
>
>> >> --- linux.trees.git.orig/include/linux/connector.h 2010-11-09 17:22:15.000000000
>> > -0500
>> >> +++ linux.trees.git/include/linux/connector.h 2010-11-11 13:14:52.000000000
>> > -0500
>> >> @@ -42,8 +42,9 @@
>> >> #define CN_VAL_DM_USERSPACE_LOG 0x1
>> >> #define CN_IDX_DRBD 0x8
>> >> #define CN_VAL_DRBD 0x1
>> >> +#define CN_KVP_IDX 0x9 /* MSFT KVP functionality */
>> >
>> > Did you reserve this number with anyone? Who?
>>
>> I sent an email to Evgeniy Polyakov (Johnpol@2ka.mipt). The mail
>> bounced back. I was hoping you would help me register this index.
>> Would it make sense to have a separate patch to deal with registering
>> this index?
>
> Yes, as I can not modify non-staging code without an ack from that
> maintainer. And I'm pretty sure that Evgeniy's address has just
> changed, use "Evgeniy Polyakov <zbr@ioremap.net>" instead.
Thanks I will do just that.
K. Y
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Greg KH @ 2010-11-12 19:22 UTC (permalink / raw)
To: Ky Srinivasan; +Cc: devel, Virtualization, Haiyang Zhang, Greg KH
In-Reply-To: <4CDD253602000030000903C9@novprvoes0310.provo.novell.com>
On Fri, Nov 12, 2010 at 11:29:58AM -0700, Ky Srinivasan wrote:
>
>
> >>> On 11/11/2010 at 4:19 PM, in message <20101111211904.GB31373@kroah.com>, Greg
> KH <greg@kroah.com> wrote:
> > On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
> >> +/*
> >> + * Array of keys we support in Linux.
> >
> > Not really, you can support "any" number of keys as the kernel shouldn't
> > care, or did I get it wrong?
> We currently support only the keys that have been specified in the KVP specification. I have a more detailed response on the core KVP protocol in response to your other email on this topic.
> >
> >> + *
> >> + */
> >> +#define KVP_MAX_KEY 10
> >> +#define KVP_LIC_VERSION 1
> >
> > Um, this is a nice magic number, care to explain it a bit more?
> As I noted in an earlier email, the KVP specification currently requires that we support 10 keys and it also specifies the ordering of these keys. The information for the key "IntegrationServicesVersion", is only available in the kernel (one of the other LIC drivers defines this information).
>
> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> >> + "IntegrationServicesVersion",
> >
> > Looks like it matches up with this, right? You might want to make that
> > a bit more "tied" together.
> >
> Yes; I will fix this.
> >> + case (KVP_LIC_VERSION):
> >> + kvp_transaction_active = true;
> >> + kvp_respond_to_host(kvp_data->index,
> >> + HV_DRV_VERSION);
> >
> > Why are you doing this in the kernel? Why not do it from userspace like
> > all other messages?
> This information is only available in the kernel (defined by another LIC driver).
I thought it was exported through a modinfo parameter? If not, you
could set the MODULE_VERSION field be the as this which then is exported
to userspace so you would not have to do any thing like this in the
kernel.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Greg KH @ 2010-11-12 18:47 UTC (permalink / raw)
To: Ky Srinivasan; +Cc: devel, Virtualization, Haiyang Zhang, Greg KH
In-Reply-To: <4CDD1FAA02000030000903B8@novprvoes0310.provo.novell.com>
On Fri, Nov 12, 2010 at 11:06:18AM -0700, Ky Srinivasan wrote:
> >> +typedef struct kvp_msg {
> >> + __u32 kvp_key; /* Key */
> >> + __u8 kvp_value[0]; /* Corresponding value */
> >> +} kvp_msg_t;
> >
> > I thought that kvp_value was really KVP_VALUE_SIZE?
>
> kvp_value is typed information and KVP_VALUE_SIZE specifies the
> maximum size of the supported value. For instance if kvp_value is a
> string (which is the case for all the values currently supported),
> KVP_VALUE_SIZE specifies the maximum size of the string that will be
> supported.
So it's a variable length structure? How do you konw how long the
structure is, does that depend on the key?
> > And no typedefs, you did run your code through checkpatch.pl, right?
> > Why ignore the stuff it spits back at you?
> I will fix this.
> >
> >
> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> >> + "IntegrationServicesVersion",
> >> + "NetworkAddressIPv4",
> >> + "NetworkAddressIPv6",
> >> + "OSBuildNumber",
> >> + "OSName",
> >> + "OSMajorVersion",
> >> + "OSMinorVersion",
> >> + "OSVersion",
> >> + "ProcessorArchitecture",
> >> + };
> >
> > Why list these at all, as more might come in the future, and the kernel
> > really doesn't care about them, right?
> The core HyperV KVP protocol is such that the host iterates through an
> index (starting with 0); the guest is free to associate a key with the
> index and return the associated value. The host side iteration is
> stopped when the guest returns a failure on an index. MSFT has
> specified the keys and their ordinal value in their KVP specification.
> The array you see above is part of that specification.
So you match on the string name of the key? I'm confused, as I didn't
think I saw you matching on the string name, only the key value.
Also, as the kernel isn't handling the key type (with one exception),
why even list these in the kernel at all? I'm all for documenting
stuff, but don't use code memory for documentation :)
> >> --- linux.trees.git.orig/include/linux/connector.h 2010-11-09 17:22:15.000000000
> > -0500
> >> +++ linux.trees.git/include/linux/connector.h 2010-11-11 13:14:52.000000000
> > -0500
> >> @@ -42,8 +42,9 @@
> >> #define CN_VAL_DM_USERSPACE_LOG 0x1
> >> #define CN_IDX_DRBD 0x8
> >> #define CN_VAL_DRBD 0x1
> >> +#define CN_KVP_IDX 0x9 /* MSFT KVP functionality */
> >
> > Did you reserve this number with anyone? Who?
>
> I sent an email to Evgeniy Polyakov (Johnpol@2ka.mipt). The mail
> bounced back. I was hoping you would help me register this index.
> Would it make sense to have a separate patch to deal with registering
> this index?
Yes, as I can not modify non-staging code without an ack from that
maintainer. And I'm pretty sure that Evgeniy's address has just
changed, use "Evgeniy Polyakov <zbr@ioremap.net>" instead.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Ky Srinivasan @ 2010-11-12 18:29 UTC (permalink / raw)
To: Greg KH; +Cc: devel, Virtualization, Haiyang Zhang, Greg KH
In-Reply-To: <20101111211904.GB31373@kroah.com>
>>> On 11/11/2010 at 4:19 PM, in message <20101111211904.GB31373@kroah.com>, Greg
KH <greg@kroah.com> wrote:
> On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
>> +/*
>> + * Array of keys we support in Linux.
>
> Not really, you can support "any" number of keys as the kernel shouldn't
> care, or did I get it wrong?
We currently support only the keys that have been specified in the KVP specification. I have a more detailed response on the core KVP protocol in response to your other email on this topic.
>
>> + *
>> + */
>> +#define KVP_MAX_KEY 10
>> +#define KVP_LIC_VERSION 1
>
> Um, this is a nice magic number, care to explain it a bit more?
As I noted in an earlier email, the KVP specification currently requires that we support 10 keys and it also specifies the ordering of these keys. The information for the key "IntegrationServicesVersion", is only available in the kernel (one of the other LIC drivers defines this information).
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> + "IntegrationServicesVersion",
>
> Looks like it matches up with this, right? You might want to make that
> a bit more "tied" together.
>
Yes; I will fix this.
>> + case (KVP_LIC_VERSION):
>> + kvp_transaction_active = true;
>> + kvp_respond_to_host(kvp_data->index,
>> + HV_DRV_VERSION);
>
> Why are you doing this in the kernel? Why not do it from userspace like
> all other messages?
This information is only available in the kernel (defined by another LIC driver).
Regards,
K. Y
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Ky Srinivasan @ 2010-11-12 18:06 UTC (permalink / raw)
To: Greg KH; +Cc: devel, Virtualization, Haiyang Zhang, Greg KH
In-Reply-To: <20101111211548.GA31373@kroah.com>
>>> On 11/11/2010 at 4:15 PM, in message <20101111211548.GA31373@kroah.com>, Greg
KH <greg@kroah.com> wrote:
> On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
>> +/*
>> + * An implementation of key value pair (KVP) functionality for Linux.
>> + *
>> + *
>> + * Copyright (C) 2010, Novell, Inc.
>> + * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation.
>
> This is all that is needed.
>
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
>> + * NON INFRINGEMENT. See the GNU General Public License for more
>> + * details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>
> You can delete this chunk.
Will do.
>
>> +/*
>> + * KVP protocol: The user mode component first registers with the
>> + * the kernel component. Subsequently, the kernel component requests, data
>> + * for the specified keys. In response to this message the user mode
> component
>> + * fills in the value corresponding to the specified key. We overload the
>> + * sequence field in the cn_msg header to define our KVP message types.
>> + *
>> + * XXXKYS: Have a shared header file between the user and kernel (TODO)
>> + */
>> +
>> +enum kvp_op {
>> + KVP_REGISTER = 0, /* Register the user mode component */
>> + KVP_KERNEL_GET,/*Kernel is requesting the value for the specified key*/
>> + KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
>> + KVP_USER_GET, /*User is requesting the value for the specified key*/
>> + KVP_USER_SET /*User is providing the value for the specified key*/
>> +};
>
> As these values are shared between the kernel and userspace, you should
> specifically define them.
>
> Also, your spaces with the /* stuff is incorrect.
>
> And, "KVP"? That's very generic, how about, "HYPERV_KVP_..." instead?
> That fits the global namespace much better.
I will change the names; deal with the comments etc.
>
> s/kvp_op/hyperv_kvp_op/ as well.
>
>> +#define KVP_KEY_SIZE 512
>> +#define KVP_VALUE_SIZE 2048
>> +
>> +
>> +typedef struct kvp_msg {
>> + __u32 kvp_key; /* Key */
>> + __u8 kvp_value[0]; /* Corresponding value */
>> +} kvp_msg_t;
>
> I thought that kvp_value was really KVP_VALUE_SIZE?
kvp_value is typed information and KVP_VALUE_SIZE specifies the maximum size of the supported value. For instance if kvp_value is a string (which is the case for all the values currently supported), KVP_VALUE_SIZE specifies the maximum size of the string that will be supported.
>
> And no typedefs, you did run your code through checkpatch.pl, right?
> Why ignore the stuff it spits back at you?
I will fix this.
>
>
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> + "IntegrationServicesVersion",
>> + "NetworkAddressIPv4",
>> + "NetworkAddressIPv6",
>> + "OSBuildNumber",
>> + "OSName",
>> + "OSMajorVersion",
>> + "OSMinorVersion",
>> + "OSVersion",
>> + "ProcessorArchitecture",
>> + };
>
> Why list these at all, as more might come in the future, and the kernel
> really doesn't care about them, right?
The core HyperV KVP protocol is such that the host iterates through an index (starting with 0); the guest is free to associate a key with the index and return the associated value. The host side iteration is stopped when the guest returns a failure on an index. MSFT has specified the keys and their ordinal value in their KVP specification. The array you see above is part of that specification.
>
>> +int
>> +kvp_init(void)
>
> All of your global symbols should have "hyperv_" on the front of them.
Will do.
>
>> --- linux.trees.git.orig/drivers/staging/hv/Makefile 2010-11-10 14:01:55.000000000
> -0500
>> +++ linux.trees.git/drivers/staging/hv/Makefile 2010-11-11 11:24:54.000000000
> -0500
>> @@ -2,7 +2,7 @@ obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_t
>> obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
>> obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
>> obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
>> -obj-$(CONFIG_HYPERV_UTILS) += hv_utils.o
>> +obj-$(CONFIG_HYPERV_UTILS) += hv_util.o
>
> Ick, you just renamed the kernel module. Did you really mean to do
> this? What tools are now going to break because you did this? (I'm
> thinking installers here...)
Oops! I will fix that.
>
>>
>> hv_vmbus-y := vmbus_drv.o osd.o \
>> vmbus.o hv.o connection.o channel.o \
>> @@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
>> hv_storvsc-y := storvsc_drv.o storvsc.o
>> hv_blkvsc-y := blkvsc_drv.o blkvsc.o
>> hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
>> +hv_util-y := hv_utils.o kvp.o
>> Index: linux.trees.git/drivers/staging/hv/kvp.h
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> +++ linux.trees.git/drivers/staging/hv/kvp.h 2010-11-10 14:03:47.000000000 -0500
>
> hyperv_kvp.h as this is going to be a global header file, right?
>
>> +typedef enum {
>> + ICKvpExchangeOperationGet = 0,
>> + ICKvpExchangeOperationSet,
>> + ICKvpExchangeOperationDelete,
>> + ICKvpExchangeOperationEnumerate,
>> + ICKvpExchangeOperationCount /* Number of operations, must be last. */
>> +} IC_KVP_EXCHANGE_OPERATION;
>> +
>> +typedef enum {
>> + ICKvpExchangePoolExternal = 0,
>> + ICKvpExchangePoolGuest,
>> + ICKvpExchangePoolAuto,
>> + ICKvpExchangePoolAutoExternal,
>> + ICKvpExchangePoolInternal,
>> + ICKvpExchangePoolCount /* Number of pools, must be last. */
>> +} IC_KVP_EXCHANGE_POOL;
>> +
>> +typedef struct ic_kvp_hdr {
>> + u8 operation;
>> + u8 pool;
>> +} ic_kvp_hdr_t;
>> +
>> +typedef struct ic_kvp_exchg_msg_value {
>> + u32 value_type;
>> + u32 key_size;
>> + u32 value_size;
>> + u8 key[IC_KVP_EXCHANGE_MAX_KEY_SIZE];
>> + u8 value[IC_KVP_EXCHANGE_MAX_VALUE_SIZE];
>> +} ic_kvp_exchg_msg_value_t;
>> +
>> +typedef struct ic_kvp__msg_enumerate {
>> + u32 index;
>> + ic_kvp_exchg_msg_value_t data;
>> +} ic_kvp_msg_enumerate_t;
>> +
>> +typedef struct ic_kvp_msg {
>> + ic_kvp_hdr_t kvp_hdr;
>> + ic_kvp_msg_enumerate_t kvp_data;
>> +} ic_kvp_msg_t;
>
> Again, no typedefs, and fix up the names of these structures to be
> understandable :)
Will do. With regards to making them understandable, I will try!
>
>> --- linux.trees.git.orig/include/linux/connector.h 2010-11-09 17:22:15.000000000
> -0500
>> +++ linux.trees.git/include/linux/connector.h 2010-11-11 13:14:52.000000000
> -0500
>> @@ -42,8 +42,9 @@
>> #define CN_VAL_DM_USERSPACE_LOG 0x1
>> #define CN_IDX_DRBD 0x8
>> #define CN_VAL_DRBD 0x1
>> +#define CN_KVP_IDX 0x9 /* MSFT KVP functionality */
>
> Did you reserve this number with anyone? Who?
I sent an email to Evgeniy Polyakov (Johnpol@2ka.mipt). The mail bounced back. I was hoping you would help me register this index. Would it make sense to have a separate patch to deal with registering this index?
>
>> -#define CN_NETLINK_USERS 8
>> +#define CN_NETLINK_USERS 10
>
> Are you sure you incremented this properly?
Oops! I will fix this.
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Ky Srinivasan @ 2010-11-12 16:57 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: devel, Virtualization, HaiyangZhang, Greg KH
In-Reply-To: <20101111124904.24010ee5@nehalam>
>>> On 11/11/2010 at 3:49 PM, in message <20101111124904.24010ee5@nehalam>,
Stephen Hemminger <shemminger@vyatta.com> wrote:
> On Thu, 11 Nov 2010 13:03:10 -0700
> "Ky Srinivasan" <ksrinivasan@novell.com> wrote:
>
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> + "IntegrationServicesVersion",
>> + "NetworkAddressIPv4",
>> + "NetworkAddressIPv6",
>> + "OSBuildNumber",
>> + "OSName",
>> + "OSMajorVersion",
>> + "OSMinorVersion",
>> + "OSVersion",
>> + "ProcessorArchitecture",
>> + };
>
> Minor nit:
> static const char *kvp_keys[KVP_MAX_KEY] = {
> "FullQualifiedDomainName",
Will do.
> ...
>
> +/*
> + * Global state maintained for transaction that is being processed.
> + * Note that only one transaction can be active at any point in time.
> + *
> + * This state is set when we receive a request from the host; we
> + * cleanup this state when the transaction is completed - when we respond
> + * to the host with the key value.
> + */
> +
> +static u8 *recv_buffer; /* the receive buffer that we allocated */
> +static int recv_len; /* number of bytes received. */
> +static struct vmbus_channel *recv_channel; /*chn on which we got the
> request*/
> +static u64 recv_req_id; /* request ID. */
> +static int kvp_current_index;
> +
>
> I would put all the state variables for the transaction in one
> structure,
Will do.
>
> +static void kvp_timer_func(unsigned long __data)
> +{
> + u32 key = *((u32 *)__data);
> + /*
> + * If the timer fires, the user-mode component has not responded;
> + * process the pending transaction.
> + */
> + kvp_respond_to_host(key, "Guest timed out");
> +}
>
> delayed_work is sometimes better for things like this, since it
> runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you having a full context is preferable. I will make the appropriate changes.
>
> + case (KVP_MAX_KEY):
> + /*
> + * We don't support this key
> + * and any key beyond this.
> + */
> + icmsghdrp->status = HV_E_FAIL;
> + goto callback_done;
> +
>
> case labels do not need parens
Thank you; my next version of this patch will incorporate your feedback.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Ky Srinivasan @ 2010-11-12 16:57 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: devel, Virtualization, HaiyangZhang, Greg KH
In-Reply-To: <20101111124904.24010ee5@nehalam>
>>> On 11/11/2010 at 3:49 PM, in message <20101111124904.24010ee5@nehalam>,
Stephen Hemminger <shemminger@vyatta.com> wrote:
> On Thu, 11 Nov 2010 13:03:10 -0700
> "Ky Srinivasan" <ksrinivasan@novell.com> wrote:
>
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> + "IntegrationServicesVersion",
>> + "NetworkAddressIPv4",
>> + "NetworkAddressIPv6",
>> + "OSBuildNumber",
>> + "OSName",
>> + "OSMajorVersion",
>> + "OSMinorVersion",
>> + "OSVersion",
>> + "ProcessorArchitecture",
>> + };
>
> Minor nit:
> static const char *kvp_keys[KVP_MAX_KEY] = {
> "FullQualifiedDomainName",
Will do.
> ...
>
> +/*
> + * Global state maintained for transaction that is being processed.
> + * Note that only one transaction can be active at any point in time.
> + *
> + * This state is set when we receive a request from the host; we
> + * cleanup this state when the transaction is completed - when we respond
> + * to the host with the key value.
> + */
> +
> +static u8 *recv_buffer; /* the receive buffer that we allocated */
> +static int recv_len; /* number of bytes received. */
> +static struct vmbus_channel *recv_channel; /*chn on which we got the
> request*/
> +static u64 recv_req_id; /* request ID. */
> +static int kvp_current_index;
> +
>
> I would put all the state variables for the transaction in one
> structure,
Will do.
>
> +static void kvp_timer_func(unsigned long __data)
> +{
> + u32 key = *((u32 *)__data);
> + /*
> + * If the timer fires, the user-mode component has not responded;
> + * process the pending transaction.
> + */
> + kvp_respond_to_host(key, "Guest timed out");
> +}
>
> delayed_work is sometimes better for things like this, since it
> runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you having a full context is preferable. I will make the appropriate changes.
>
> + case (KVP_MAX_KEY):
> + /*
> + * We don't support this key
> + * and any key beyond this.
> + */
> + icmsghdrp->status = HV_E_FAIL;
> + goto callback_done;
> +
>
> case labels do not need parens
Thank you; my next version of this patch will incorporate your feedback.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH]: An implementation of HyperV KVP functionality
From: Ky Srinivasan @ 2010-11-12 16:57 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: devel, Virtualization, HaiyangZhang, Greg KH
In-Reply-To: <20101111124904.24010ee5@nehalam>
>>> On 11/11/2010 at 3:49 PM, in message <20101111124904.24010ee5@nehalam>,
Stephen Hemminger <shemminger@vyatta.com> wrote:
> On Thu, 11 Nov 2010 13:03:10 -0700
> "Ky Srinivasan" <ksrinivasan@novell.com> wrote:
>
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> + "IntegrationServicesVersion",
>> + "NetworkAddressIPv4",
>> + "NetworkAddressIPv6",
>> + "OSBuildNumber",
>> + "OSName",
>> + "OSMajorVersion",
>> + "OSMinorVersion",
>> + "OSVersion",
>> + "ProcessorArchitecture",
>> + };
>
> Minor nit:
> static const char *kvp_keys[KVP_MAX_KEY] = {
> "FullQualifiedDomainName",
Will do.
> ...
>
> +/*
> + * Global state maintained for transaction that is being processed.
> + * Note that only one transaction can be active at any point in time.
> + *
> + * This state is set when we receive a request from the host; we
> + * cleanup this state when the transaction is completed - when we respond
> + * to the host with the key value.
> + */
> +
> +static u8 *recv_buffer; /* the receive buffer that we allocated */
> +static int recv_len; /* number of bytes received. */
> +static struct vmbus_channel *recv_channel; /*chn on which we got the
> request*/
> +static u64 recv_req_id; /* request ID. */
> +static int kvp_current_index;
> +
>
> I would put all the state variables for the transaction in one
> structure,
Will do.
>
> +static void kvp_timer_func(unsigned long __data)
> +{
> + u32 key = *((u32 *)__data);
> + /*
> + * If the timer fires, the user-mode component has not responded;
> + * process the pending transaction.
> + */
> + kvp_respond_to_host(key, "Guest timed out");
> +}
>
> delayed_work is sometimes better for things like this, since it
> runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you having a full context is preferable. I will make the appropriate changes.
>
> + case (KVP_MAX_KEY):
> + /*
> + * We don't support this key
> + * and any key beyond this.
> + */
> + icmsghdrp->status = HV_E_FAIL;
> + goto callback_done;
> +
>
> case labels do not need parens
Thank you; my next version of this patch will incorporate your feedback.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 04/20] x86/ticketlock: make large and small ticket versions of spin_lock the same
From: Jeremy Fitzhardinge @ 2010-11-12 16:27 UTC (permalink / raw)
To: vatsa
Cc: Nick Piggin, Xen-devel, Peter Zijlstra, Linux Kernel Mailing List,
Jan Beulich, Linux Virtualization, Jeremy Fitzhardinge,
Avi Kivity, H. Peter Anvin
In-Reply-To: <20101112121932.GA30016@linux.vnet.ibm.com>
On 11/12/2010 04:19 AM, Srivatsa Vaddagiri wrote:
> On Wed, Nov 03, 2010 at 10:59:45AM -0400, Jeremy Fitzhardinge wrote:
>> Make the bulk of __ticket_spin_lock look identical for large and small
>> number of cpus.
> [snip]
>
>> #if (NR_CPUS < 256)
>> static __always_inline void __ticket_spin_lock(arch_spinlock_t *lock)
>> {
>> - register union {
>> - struct __raw_tickets tickets;
>> - unsigned short slock;
>> - } inc = { .slock = 1 << TICKET_SHIFT };
>> + register struct __raw_tickets inc = { .tail = 1 };
> [snip]
>
>> #else
>> static __always_inline void __ticket_spin_lock(arch_spinlock_t *lock)
>> {
>> - unsigned inc = 1 << TICKET_SHIFT;
>> - __ticket_t tmp;
>> + register struct __raw_tickets inc = { .tickets.tail = 1 };
> s/.tickets//?
>
> Otherwise I get a compile error for NR_CPUS > 256, with just 4 patches applied.
Yeah, likely. That's precisely why I wanted to make them the same ;).
J
^ permalink raw reply
* Announce: Auto/Lazy-migration Patches RFC on linux-numa list
From: Lee Schermerhorn @ 2010-11-12 16:15 UTC (permalink / raw)
To: qemu-devel, linux-mm, linux-kernel, kvm, virtualization
At last weeks' LPC, there was some interest in my patches for Auto/Lazy
Migration to improve locality and possibly performance of unpinned guest
VMs on a NUMA platform. As a result of these conversations I have reposted
the patches [4 series, ~40 patches] as RFCs to the linux-numa list. Links
to threads given below.
I have rebased the patches atop 3Nov10 mmotm series [2.6.36 + 3nov mmotm].
The patched kernel builds, boots and survives some fairly heavy testing on
an 8 node istanbul x86_64. Under heavy load, I do encounter a race in the
somewhat optional migration cache. Currently this generates a warning and
carries on, but the one migration cache entry and related page is then
wedged. This would need to be resolved.
The series/threads in the order applied:
[PATCH/RFC 0/14] Shared Policy Overview
http://markmail.org/message/trvpl3t7gimvwht6
[PATCH/RFC 0/8] numa - Migrate-on-Fault
http://markmail.org/message/mdwbcitql5ka4uws
[PATCH/RFC 0/11] numa - Automatic-migration
http://markmail.org/message/zik3itmqed65mol2
[PATCH/RFC 1/5] numa - migration cache - core implementation
http://markmail.org/message/xvck7enyezx6chyi
RESEND: [PATCH/RFC 1/5] numa - migration cache - core implementation
http://markmail.org/message/xgvvrnn2nk4nsn2e
resend to add back the patch description missing from 1st attempt.
^ permalink raw reply
* Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport
From: Anthony Liguori @ 2010-11-12 13:21 UTC (permalink / raw)
To: Ian Molton
Cc: QEMU Developers, Rusty Russell, linux-kernel, virtualization,
virtualization, Alon Levy, Avi Kivity
In-Reply-To: <4CDD2F9A.7000702@collabora.co.uk>
On 11/12/2010 06:14 AM, Ian Molton wrote:
> On 10/11/10 17:47, Anthony Liguori wrote:
>> On 11/10/2010 11:22 AM, Ian Molton wrote:
>>> Ping ?
>>
>> I think the best way forward is to post patches.
>
> I posted links to the git trees. I can post patches, but they are
> *large*. Do you really want me to post them?
Yes, and they have to be split up into something reviewable.
>
>> To summarize what I was trying to express in the thread, I think this is
>> not the right long term architecture but am not opposed to it as a short
>> term solution. I think having a new virtio device is a bad design choice
>> but am not totally opposed to it.
>
> Ok! (I agree (that this should be a short term solution) :) )
>
>> you want to go for the path of integration, you're going to have to fix
>> all of the coding style issues and make the code fit into QEMU. Dropping
>> a bunch of junk into target-i386/ is not making the code fit into QEMU.
>
> I agree. how about hw/gl for the renderer and hw/ for the virtio module?
That would be fine.
>> If you post just what you have now in patch form, I can try to provide
>> more concrete advice ignoring the coding style problems.
>
> I can post patches, although I dont think LKML would appreciate the
> volume! I can post them to the qemu list if you do.
Yes, qemu is where I was suggesting you post them.
Regards,
Anthony Liguori
> -Ian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox