Re: [patch] HWBKPT: Make bp_len type to u64 generic across the arch

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sat, Jan 30, 2010 at 10:25:18AM +0530, Mahesh Salgaonkar wrote:
> 
> Change 'bp_len' type to __u64 to make it work across the arch.
> The s390 architecture watch point length can be upto 2^64.
> 
> reference:
> 	http://lkml.org/lkml/2010/1/25/212
> 
> Based on commit 6aa41f8b01301199af6c9febb24f3c1f5a0bc9d5
> 
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> ---
> 
>  include/linux/hw_breakpoint.h |    2 +-
>  include/linux/perf_event.h    |    6 ++----
>  kernel/hw_breakpoint.c        |    2 +-
>  kernel/perf_event.c           |    2 +-
>  4 files changed, 5 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
> index 41235c9..76e7427 100644
> --- a/include/linux/hw_breakpoint.h
> +++ b/include/linux/hw_breakpoint.h
> @@ -44,7 +44,7 @@ static inline int hw_breakpoint_type(struct perf_event *bp)
>  	return bp->attr.bp_type;
>  }
>  
> -static inline int hw_breakpoint_len(struct perf_event *bp)
> +static inline unsigned long hw_breakpoint_len(struct perf_event *bp)
>  {
>  	return bp->attr.bp_len;
>  }



This should return a u64, or gcc will warn us about loosing
informations in 32 bits arch?



> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 1438463..30c78bd 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -211,11 +211,9 @@ struct perf_event_attr {
>  		__u32		wakeup_watermark; /* bytes before wakeup   */
>  	};
>  
> -	__u32			__reserved_2;
> -
> -	__u64			bp_addr;
>  	__u32			bp_type;
> -	__u32			bp_len;
> +	__u64			bp_addr;
> +	__u64			bp_len;
>  };



Peter, what do you think about this new layout?
Putting the bp_type right after the wakeup_* fields
is going to remove the padding difference between
64 and 32 archs. That looks better than the __reserved_2
we had.

If this patch can make it for .33, it would be nice.



>  
>  /*
> diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
> index 50dbd59..a1f511f 100644
> --- a/kernel/hw_breakpoint.c
> +++ b/kernel/hw_breakpoint.c
> @@ -324,8 +324,8 @@ EXPORT_SYMBOL_GPL(register_user_hw_breakpoint);
>  int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
>  {
>  	u64 old_addr = bp->attr.bp_addr;
> +	u64 old_len = bp->attr.bp_len;
>  	int old_type = bp->attr.bp_type;
> -	int old_len = bp->attr.bp_len;
>  	int err = 0;
>  
>  	perf_event_disable(bp);
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 8f8f7aa..1473cc9 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -4720,7 +4720,7 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
>  	if (attr->type >= PERF_TYPE_MAX)
>  		return -EINVAL;
>  
> -	if (attr->__reserved_1 || attr->__reserved_2)
> +	if (attr->__reserved_1)
>  		return -EINVAL;
>  
>  	if (attr->sample_type & ~(PERF_SAMPLE_MAX-1))
> 


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>