The userspace event metadata structure was packed so when sent from a kernel
with a certain set of alignment rules to a userspace listener with a different
set of alignment rules the userspace process would be able to use the
structure. On some arches just using packed, even if it doesn't do anything
to the alignment can cause a severe performance hit. From now on we are
not going to set the packed attribute and will just need to be very careful
to make sure the structure is naturally aligned.
Cc: Andreas Schwab <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
Cc: Andreas Gruenbacher <[email protected]>
Signed-off-by: Eric Paris <[email protected]>
---
include/linux/fanotify.h | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 0535461..b892e46 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -65,20 +65,23 @@
FAN_ALL_PERM_EVENTS |\
FAN_Q_OVERFLOW)
-#define FANOTIFY_METADATA_VERSION 1
-
+#define FANOTIFY_METADATA_VERSION 2
+/*
+ * These structures must be naturally aligned so that a 32 bit userspace process
+ * will find the offsets the same as a 64bit process.
+ */
struct fanotify_event_metadata {
__u32 event_len;
__u32 vers;
- __u64 mask;
+ aligned_u64 mask;
__s32 fd;
__s32 pid;
-} __attribute__ ((packed));
+};
struct fanotify_response {
__s32 fd;
__u32 response;
-} __attribute__ ((packed));
+};
/* Legit userspace responses to a _PERM event */
#define FAN_ALLOW 0x01
--
1.7.1
I liked this version until I realized that userspace doesn't have
aligned_u64 as a valid type.
I'm feeling more like my old version of the patch. Anyone have
thoughts or comments?
-Eric
On Tue, Aug 24, 2010 at 4:43 PM, Eric Paris <[email protected]> wrote:
> The userspace event metadata structure was packed so when sent from a kernel
> with a certain set of alignment rules to a userspace listener with a different
> set of alignment rules the userspace process would be able to use the
> structure. ?On some arches just using packed, even if it doesn't do anything
> to the alignment can cause a severe performance hit. ?From now on we are
> not going to set the packed attribute and will just need to be very careful
> to make sure the structure is naturally aligned.
>
> Cc: Andreas Schwab <[email protected]>
> Cc: Tvrtko Ursulin <[email protected]>
> Cc: Andreas Gruenbacher <[email protected]>
> Signed-off-by: Eric Paris <[email protected]>
> ---
> ?include/linux/fanotify.h | ? 13 ++++++++-----
> ?1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index 0535461..b892e46 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -65,20 +65,23 @@
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? FAN_ALL_PERM_EVENTS |\
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? FAN_Q_OVERFLOW)
>
> -#define FANOTIFY_METADATA_VERSION ? ? ?1
> -
> +#define FANOTIFY_METADATA_VERSION ? ? ?2
> +/*
> + * These structures must be naturally aligned so that a 32 bit userspace process
> + * will find the offsets the same as a 64bit process.
> + */
> ?struct fanotify_event_metadata {
> ? ? ? ?__u32 event_len;
> ? ? ? ?__u32 vers;
> - ? ? ? __u64 mask;
> + ? ? ? aligned_u64 mask;
> ? ? ? ?__s32 fd;
> ? ? ? ?__s32 pid;
> -} __attribute__ ((packed));
> +};
>
> ?struct fanotify_response {
> ? ? ? ?__s32 fd;
> ? ? ? ?__u32 response;
> -} __attribute__ ((packed));
> +};
>
> ?/* Legit userspace responses to a _PERM event */
> ?#define FAN_ALLOW ? ? ?0x01
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Saturday 28 August 2010 01:51:53 Eric Paris wrote:
> I liked this version until I realized that userspace doesn't have
> aligned_u64 as a valid type.
This looks like an error in include/linux/types.h. The aligned types should
probably not be defined inside #ifdef __KERNEL__.
The following other headers expose aligned 64-bit types to user space as well;
copying the netfilter list:
include/linux/if_ppp.h
include/linux/netfilter/nfnetlink_queue.h
include/linux/netfilter/nfnetlink_log.h
include/linux/netfilter/xt_quota.h
include/linux/netfilter/xt_connbytes.h
Otherwise, the definition of those types is really simple, and this would do
in include/linux/fanotify.h until include/linux/types.h is fixed:
#ifndef aligned_u64
# define aligned_u64 __u64 __attribute__((aligned(8)))
#endif
Thanks,
Andreas
From: Andreas Gruenbacher <[email protected]>
Date: Mon, 30 Aug 2010 03:26:29 +0200
> On Saturday 28 August 2010 01:51:53 Eric Paris wrote:
>> I liked this version until I realized that userspace doesn't have
>> aligned_u64 as a valid type.
>
> This looks like an error in include/linux/types.h. The aligned types should
> probably not be defined inside #ifdef __KERNEL__.
You can't do this, as it would pollute the POSIX namespace.
If we want a version of this type visible to userspace, it needs to,
for example, have double underscores prepended to the type name just
as we do for things like __u16 and __u32.
On Monday 30 August 2010 06:21:56 David Miller wrote:
> From: Andreas Gruenbacher <[email protected]>
> Date: Mon, 30 Aug 2010 03:26:29 +0200
>
> > On Saturday 28 August 2010 01:51:53 Eric Paris wrote:
> >> I liked this version until I realized that userspace doesn't have
> >> aligned_u64 as a valid type.
> >
> > This looks like an error in include/linux/types.h. The aligned types
> > should probably not be defined inside #ifdef __KERNEL__.
>
> You can't do this, as it would pollute the POSIX namespace.
Good point.
> If we want a version of this type visible to userspace, it needs to,
> for example, have double underscores prepended to the type name just
> as we do for things like __u16 and __u32.
How about something like this?
Thanks,
Andreas
~
>From 3bec018a4835d4fdbe35595366a51bd09e3cc1d0 Mon Sep 17 00:00:00 2001
From: Andreas Gruenbacher <[email protected]>
Date: Mon, 30 Aug 2010 12:51:01 +0200
Subject: [PATCH] Define __aligned_{u64,le64,be64} types with 8-byte alignment
Convert the existing #defines into typedefs, prepend two underscores to
avoid POSIX namespace pollution, and expose the types to user space.
These types are useful for enforcing the same alignment on 32-bit and
64-bit architectures. (Some 32-bit architectures only align 64-bit
values on 4-byte boundaries by default.)
(The aligned types are used by some netfilter user-space headers
already.)
Signed-off-by: Andreas Gruenbacher <[email protected]>
---
include/linux/if_ppp.h | 16 +++++++-------
include/linux/netfilter/nfnetlink_log.h | 4 +-
include/linux/netfilter/nfnetlink_queue.h | 4 +-
include/linux/netfilter/xt_connbytes.h | 4 +-
include/linux/netfilter/xt_quota.h | 2 +-
include/linux/types.h | 10 ++++----
include/scsi/scsi_tgt_if.h | 30 ++++++++++++++--------------
include/xen/interface/hvm/hvm_op.h | 2 +-
8 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/include/linux/if_ppp.h b/include/linux/if_ppp.h
index fcef103..c9ad383 100644
--- a/include/linux/if_ppp.h
+++ b/include/linux/if_ppp.h
@@ -114,14 +114,14 @@ struct pppol2tp_ioc_stats {
__u16 tunnel_id; /* redundant */
__u16 session_id; /* if zero, get tunnel stats */
__u32 using_ipsec:1; /* valid only for session_id == 0 */
- aligned_u64 tx_packets;
- aligned_u64 tx_bytes;
- aligned_u64 tx_errors;
- aligned_u64 rx_packets;
- aligned_u64 rx_bytes;
- aligned_u64 rx_seq_discards;
- aligned_u64 rx_oos_packets;
- aligned_u64 rx_errors;
+ __aligned_u64 tx_packets;
+ __aligned_u64 tx_bytes;
+ __aligned_u64 tx_errors;
+ __aligned_u64 rx_packets;
+ __aligned_u64 rx_bytes;
+ __aligned_u64 rx_seq_discards;
+ __aligned_u64 rx_oos_packets;
+ __aligned_u64 rx_errors;
};
#define ifr__name b.ifr_ifrn.ifrn_name
diff --git a/include/linux/netfilter/nfnetlink_log.h b/include/linux/netfilter/nfnetlink_log.h
index ea9b8d3..90c2c95 100644
--- a/include/linux/netfilter/nfnetlink_log.h
+++ b/include/linux/netfilter/nfnetlink_log.h
@@ -28,8 +28,8 @@ struct nfulnl_msg_packet_hw {
};
struct nfulnl_msg_packet_timestamp {
- aligned_be64 sec;
- aligned_be64 usec;
+ __aligned_be64 sec;
+ __aligned_be64 usec;
};
enum nfulnl_attr_type {
diff --git a/include/linux/netfilter/nfnetlink_queue.h b/include/linux/netfilter/nfnetlink_queue.h
index 2455fe5..af94e00 100644
--- a/include/linux/netfilter/nfnetlink_queue.h
+++ b/include/linux/netfilter/nfnetlink_queue.h
@@ -25,8 +25,8 @@ struct nfqnl_msg_packet_hw {
};
struct nfqnl_msg_packet_timestamp {
- aligned_be64 sec;
- aligned_be64 usec;
+ __aligned_be64 sec;
+ __aligned_be64 usec;
};
enum nfqnl_attr_type {
diff --git a/include/linux/netfilter/xt_connbytes.h b/include/linux/netfilter/xt_connbytes.h
index 92fcbb0..f1d6c15 100644
--- a/include/linux/netfilter/xt_connbytes.h
+++ b/include/linux/netfilter/xt_connbytes.h
@@ -17,8 +17,8 @@ enum xt_connbytes_direction {
struct xt_connbytes_info {
struct {
- aligned_u64 from; /* count to be matched */
- aligned_u64 to; /* count to be matched */
+ __aligned_u64 from; /* count to be matched */
+ __aligned_u64 to; /* count to be matched */
} count;
__u8 what; /* ipt_connbytes_what */
__u8 direction; /* ipt_connbytes_direction */
diff --git a/include/linux/netfilter/xt_quota.h b/include/linux/netfilter/xt_quota.h
index b0d28c6..468c4e9 100644
--- a/include/linux/netfilter/xt_quota.h
+++ b/include/linux/netfilter/xt_quota.h
@@ -11,7 +11,7 @@ struct xt_quota_priv;
struct xt_quota_info {
u_int32_t flags;
u_int32_t pad;
- aligned_u64 quota;
+ __aligned_u64 quota;
/* Used internally by the kernel */
struct xt_quota_priv *master;
diff --git a/include/linux/types.h b/include/linux/types.h
index 01a082f..1b17aa8 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -121,11 +121,6 @@ typedef __u64 u_int64_t;
typedef __s64 int64_t;
#endif
-/* this is a special 64bit data type that is 8-byte aligned */
-#define aligned_u64 __u64 __attribute__((aligned(8)))
-#define aligned_be64 __be64 __attribute__((aligned(8)))
-#define aligned_le64 __le64 __attribute__((aligned(8)))
-
/**
* The type used for indexing onto a disc or disc partition.
*
@@ -178,6 +173,11 @@ typedef __u64 __bitwise __be64;
typedef __u16 __bitwise __sum16;
typedef __u32 __bitwise __wsum;
+/* special 64bit data types that are 8-byte aligned */
+typedef __u64 __aligned_u64 __attribute__((aligned(8)));
+typedef __be64 __aligned_be64 __attribute__((aligned(8)));
+typedef __le64 __aligned_le64 __attribute__((aligned(8)));
+
#ifdef __KERNEL__
typedef unsigned __bitwise__ gfp_t;
typedef unsigned __bitwise__ fmode_t;
diff --git a/include/scsi/scsi_tgt_if.h b/include/scsi/scsi_tgt_if.h
index f2ee7c2..8f3c5e4 100644
--- a/include/scsi/scsi_tgt_if.h
+++ b/include/scsi/scsi_tgt_if.h
@@ -48,10 +48,10 @@ struct tgt_event {
struct {
int host_no;
int result;
- aligned_u64 itn_id;
- aligned_u64 tag;
- aligned_u64 uaddr;
- aligned_u64 sense_uaddr;
+ __aligned_u64 itn_id;
+ __aligned_u64 tag;
+ __aligned_u64 uaddr;
+ __aligned_u64 sense_uaddr;
uint32_t len;
uint32_t sense_len;
uint8_t rw;
@@ -59,13 +59,13 @@ struct tgt_event {
struct {
int host_no;
int result;
- aligned_u64 itn_id;
- aligned_u64 mid;
+ __aligned_u64 itn_id;
+ __aligned_u64 mid;
} tsk_mgmt_rsp;
struct {
__s32 host_no;
__s32 result;
- aligned_u64 itn_id;
+ __aligned_u64 itn_id;
__u32 function;
} it_nexus_rsp;
@@ -73,30 +73,30 @@ struct tgt_event {
struct {
int host_no;
uint32_t data_len;
- aligned_u64 itn_id;
+ __aligned_u64 itn_id;
uint8_t scb[16];
uint8_t lun[8];
int attribute;
- aligned_u64 tag;
+ __aligned_u64 tag;
} cmd_req;
struct {
int host_no;
int result;
- aligned_u64 itn_id;
- aligned_u64 tag;
+ __aligned_u64 itn_id;
+ __aligned_u64 tag;
} cmd_done;
struct {
int host_no;
int function;
- aligned_u64 itn_id;
- aligned_u64 tag;
+ __aligned_u64 itn_id;
+ __aligned_u64 tag;
uint8_t lun[8];
- aligned_u64 mid;
+ __aligned_u64 mid;
} tsk_mgmt_req;
struct {
__s32 host_no;
__u32 function;
- aligned_u64 itn_id;
+ __aligned_u64 itn_id;
__u32 max_cmds;
__u8 initiator_id[16];
} it_nexus_req;
diff --git a/include/xen/interface/hvm/hvm_op.h b/include/xen/interface/hvm/hvm_op.h
index a4827f4..80c3141 100644
--- a/include/xen/interface/hvm/hvm_op.h
+++ b/include/xen/interface/hvm/hvm_op.h
@@ -38,7 +38,7 @@ struct xen_hvm_pagetable_dying {
/* Domain with a pagetable about to be destroyed. */
domid_t domid;
/* guest physical address of the toplevel pagetable dying */
- aligned_u64 gpa;
+ __aligned_u64 gpa;
};
typedef struct xen_hvm_pagetable_dying xen_hvm_pagetable_dying_t;
DEFINE_GUEST_HANDLE_STRUCT(xen_hvm_pagetable_dying_t);
On Monday 2010-08-30 12:58, Andreas Gruenbacher wrote:
>
>> If we want a version of this type visible to userspace, it needs to,
>> for example, have double underscores prepended to the type name just
>> as we do for things like __u16 and __u32.
>
>How about something like this?
I like that approach:
>From 3bec018a4835d4fdbe35595366a51bd09e3cc1d0 Mon Sep 17 00:00:00 2001
>From: Andreas Gruenbacher <[email protected]>
>Date: Mon, 30 Aug 2010 12:51:01 +0200
>Subject: [PATCH] Define __aligned_{u64,le64,be64} types with 8-byte alignment
>
>Convert the existing #defines into typedefs, prepend two underscores to
>avoid POSIX namespace pollution, and expose the types to user space.
>
>These types are useful for enforcing the same alignment on 32-bit and
>64-bit architectures. (Some 32-bit architectures only align 64-bit
>values on 4-byte boundaries by default.)
>
>(The aligned types are used by some netfilter user-space headers
>already.)
>
>Signed-off-by: Andreas Gruenbacher <[email protected]>
>---
> include/linux/if_ppp.h | 16 +++++++-------
> include/linux/netfilter/nfnetlink_log.h | 4 +-
> include/linux/netfilter/nfnetlink_queue.h | 4 +-
> include/linux/netfilter/xt_connbytes.h | 4 +-
> include/linux/netfilter/xt_quota.h | 2 +-
> include/linux/types.h | 10 ++++----
> include/scsi/scsi_tgt_if.h | 30 ++++++++++++++--------------
> include/xen/interface/hvm/hvm_op.h | 2 +-
> 8 files changed, 36 insertions(+), 36 deletions(-)
>
>diff --git a/include/linux/if_ppp.h b/include/linux/if_ppp.h
>index fcef103..c9ad383 100644
>--- a/include/linux/if_ppp.h
>+++ b/include/linux/if_ppp.h
>@@ -114,14 +114,14 @@ struct pppol2tp_ioc_stats {
> __u16 tunnel_id; /* redundant */
> __u16 session_id; /* if zero, get tunnel stats */
> __u32 using_ipsec:1; /* valid only for session_id == 0 */
>- aligned_u64 tx_packets;
>- aligned_u64 tx_bytes;
>- aligned_u64 tx_errors;
>- aligned_u64 rx_packets;
>- aligned_u64 rx_bytes;
>- aligned_u64 rx_seq_discards;
>- aligned_u64 rx_oos_packets;
>- aligned_u64 rx_errors;
>+ __aligned_u64 tx_packets;
>+ __aligned_u64 tx_bytes;
>+ __aligned_u64 tx_errors;
>+ __aligned_u64 rx_packets;
>+ __aligned_u64 rx_bytes;
>+ __aligned_u64 rx_seq_discards;
>+ __aligned_u64 rx_oos_packets;
>+ __aligned_u64 rx_errors;
From: Jan Engelhardt <[email protected]>
Date: Mon, 30 Aug 2010 15:02:55 +0200 (CEST)
>
> On Monday 2010-08-30 12:58, Andreas Gruenbacher wrote:
>>
>>> If we want a version of this type visible to userspace, it needs to,
>>> for example, have double underscores prepended to the type name just
>>> as we do for things like __u16 and __u32.
>>
>>How about something like this?
>
>
> I like that approach:
>
Me too.
David Miller wrote:
> Someone has to first add the types to linux/types.h, and that doesn't
> go through my tree.
Since the changes to linux/types.h got added in 79b5dc0c64d88cda3da23b2e22a5cec0964372ac is there any chance of the linux/if_ppp.h
changes making it into 2.6.37 ?
Iain