RE: [Intel-gfx] [PATCH] Make is_signed_type() simpler

From: Ruhl, Michael J
Date: Mon Sep 30 2019 - 16:40:27 EST

Next message: Pavel Begunkov (Silence): "[PATCH v2 0/2] Simplify blk_mq_in_flight*"
Previous message: Liang, Kan: "Re: [PATCH V4 08/14] perf/x86/intel: Support per thread RDPMC TopDown metrics"
In reply to: Rasmus Villemoes: "Re: [PATCH] Make is_signed_type() simpler"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of
>Alexey Dobriyan
>Sent: Sunday, September 29, 2019 4:06 PM
>To: akpm@xxxxxxxxxxxxxxxxxxxx
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux@xxxxxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; rostedt@xxxxxxxxxxx; mingo@xxxxxxxxxx
>Subject: [Intel-gfx] [PATCH] Make is_signed_type() simpler
>
>* Simply compare -1 with 0,
>* Drop unnecessary parenthesis sets
>
>New macro leaves pointer as "unsigned type" but gives a warning,
>which should be fine because asking whether a pointer is signed is
>strange question.
>
>I'm not sure what's going on in the i915 driver, it is shipping kernel
>pointers to userspace.
>
>Signed-off-by: Alexey Dobriyan <adobriyan@xxxxxxxxx>
>---
>
> drivers/gpu/drm/i915/i915_trace.h | 86 +++++++++++++++++++--------------
>-----
> include/linux/overflow.h | 2
> include/linux/trace_events.h | 2
> tools/include/linux/overflow.h | 2
> 4 files changed, 46 insertions(+), 46 deletions(-)
>
>--- a/drivers/gpu/drm/i915/i915_trace.h
>+++ b/drivers/gpu/drm/i915/i915_trace.h
>@@ -419,16 +419,16 @@ TRACE_EVENT(i915_gem_object_create,
> TP_ARGS(obj),
>
> TP_STRUCT__entry(
>- __field(struct drm_i915_gem_object *, obj)
>+ __field(unsigned long, obj)
> __field(u64, size)
> ),
>
> TP_fast_assign(
>- __entry->obj = obj;
>+ __entry->obj = (unsigned long)obj;
> __entry->size = obj->base.size;
> ),
>
>- TP_printk("obj=%p, size=0x%llx", __entry->obj, __entry->size)
>+ TP_printk("obj=%p, size=0x%llx", (void *)__entry->obj, __entry-

Looking up the printk format specifiers, I see:

%p

Pointers printed without a specifier extension (i.e unadorned %p) are
hashed to give a unique identifier without leaking kernel addresses to user
space.

This would seem to me that we this code is correct. Am I misreading this?

Mike

>>size)
> );
>
> TRACE_EVENT(i915_gem_shrink,
>@@ -456,25 +456,25 @@ TRACE_EVENT(i915_vma_bind,
> TP_ARGS(vma, flags),
>
> TP_STRUCT__entry(
>- __field(struct drm_i915_gem_object *, obj)
>- __field(struct i915_address_space *, vm)
>+ __field(unsigned long, obj)
>+ __field(unsigned long, vm)
> __field(u64, offset)
> __field(u64, size)
> __field(unsigned, flags)
> ),
>
> TP_fast_assign(
>- __entry->obj = vma->obj;
>- __entry->vm = vma->vm;
>+ __entry->obj = (unsigned long)vma->obj;
>+ __entry->vm = (unsigned long)vma->vm;
> __entry->offset = vma->node.start;
> __entry->size = vma->node.size;
> __entry->flags = flags;
> ),
>
> TP_printk("obj=%p, offset=0x%016llx size=0x%llx%s vm=%p",
>- __entry->obj, __entry->offset, __entry->size,
>+ (void *)__entry->obj, __entry->offset, __entry->size,
> __entry->flags & PIN_MAPPABLE ? ", mappable" : "",
>- __entry->vm)
>+ (void *)__entry->vm)
> );
>
> TRACE_EVENT(i915_vma_unbind,
>@@ -482,21 +482,21 @@ TRACE_EVENT(i915_vma_unbind,
> TP_ARGS(vma),
>
> TP_STRUCT__entry(
>- __field(struct drm_i915_gem_object *, obj)
>- __field(struct i915_address_space *, vm)
>+ __field(unsigned long, obj)
>+ __field(unsigned long, vm)
> __field(u64, offset)
> __field(u64, size)
> ),
>
> TP_fast_assign(
>- __entry->obj = vma->obj;
>- __entry->vm = vma->vm;
>+ __entry->obj = (unsigned long)vma->obj;
>+ __entry->vm = (unsigned long)vma->vm;
> __entry->offset = vma->node.start;
> __entry->size = vma->node.size;
> ),
>
> TP_printk("obj=%p, offset=0x%016llx size=0x%llx vm=%p",
>- __entry->obj, __entry->offset, __entry->size, __entry-
>>vm)
>+ (void *)__entry->obj, __entry->offset, __entry->size,
>(void *)__entry->vm)
> );
>
> TRACE_EVENT(i915_gem_object_pwrite,
>@@ -504,19 +504,19 @@ TRACE_EVENT(i915_gem_object_pwrite,
> TP_ARGS(obj, offset, len),
>
> TP_STRUCT__entry(
>- __field(struct drm_i915_gem_object *, obj)
>+ __field(unsigned long, obj)
> __field(u64, offset)
> __field(u64, len)
> ),
>
> TP_fast_assign(
>- __entry->obj = obj;
>+ __entry->obj = (unsigned long)obj;
> __entry->offset = offset;
> __entry->len = len;
> ),
>
> TP_printk("obj=%p, offset=0x%llx, len=0x%llx",
>- __entry->obj, __entry->offset, __entry->len)
>+ (void *)__entry->obj, __entry->offset, __entry->len)
> );
>
> TRACE_EVENT(i915_gem_object_pread,
>@@ -524,19 +524,19 @@ TRACE_EVENT(i915_gem_object_pread,
> TP_ARGS(obj, offset, len),
>
> TP_STRUCT__entry(
>- __field(struct drm_i915_gem_object *, obj)
>+ __field(unsigned long, obj)
> __field(u64, offset)
> __field(u64, len)
> ),
>
> TP_fast_assign(
>- __entry->obj = obj;
>+ __entry->obj = (unsigned long)obj;
> __entry->offset = offset;
> __entry->len = len;
> ),
>
> TP_printk("obj=%p, offset=0x%llx, len=0x%llx",
>- __entry->obj, __entry->offset, __entry->len)
>+ (void *)__entry->obj, __entry->offset, __entry->len)
> );
>
> TRACE_EVENT(i915_gem_object_fault,
>@@ -544,21 +544,21 @@ TRACE_EVENT(i915_gem_object_fault,
> TP_ARGS(obj, index, gtt, write),
>
> TP_STRUCT__entry(
>- __field(struct drm_i915_gem_object *, obj)
>+ __field(unsigned long, obj)
> __field(u64, index)
> __field(bool, gtt)
> __field(bool, write)
> ),
>
> TP_fast_assign(
>- __entry->obj = obj;
>+ __entry->obj = (unsigned long)obj;
> __entry->index = index;
> __entry->gtt = gtt;
> __entry->write = write;
> ),
>
> TP_printk("obj=%p, %s index=%llu %s",
>- __entry->obj,
>+ (void *)__entry->obj,
> __entry->gtt ? "GTT" : "CPU",
> __entry->index,
> __entry->write ? ", writable" : "")
>@@ -569,14 +569,14 @@ DECLARE_EVENT_CLASS(i915_gem_object,
> TP_ARGS(obj),
>
> TP_STRUCT__entry(
>- __field(struct drm_i915_gem_object *, obj)
>+ __field(unsigned long, obj)
> ),
>
> TP_fast_assign(
>- __entry->obj = obj;
>+ __entry->obj = (unsigned long)obj;
> ),
>
>- TP_printk("obj=%p", __entry->obj)
>+ TP_printk("obj=%p", (void *)__entry->obj)
> );
>
> DEFINE_EVENT(i915_gem_object, i915_gem_object_clflush,
>@@ -595,7 +595,7 @@ TRACE_EVENT(i915_gem_evict,
>
> TP_STRUCT__entry(
> __field(u32, dev)
>- __field(struct i915_address_space *, vm)
>+ __field(unsigned long, vm)
> __field(u64, size)
> __field(u64, align)
> __field(unsigned int, flags)
>@@ -603,14 +603,14 @@ TRACE_EVENT(i915_gem_evict,
>
> TP_fast_assign(
> __entry->dev = vm->i915->drm.primary->index;
>- __entry->vm = vm;
>+ __entry->vm = (unsigned long)vm;
> __entry->size = size;
> __entry->align = align;
> __entry->flags = flags;
> ),
>
> TP_printk("dev=%d, vm=%p, size=0x%llx, align=0x%llx %s",
>- __entry->dev, __entry->vm, __entry->size, __entry-
>>align,
>+ __entry->dev, (void *)__entry->vm, __entry->size,
>__entry->align,
> __entry->flags & PIN_MAPPABLE ? ", mappable" : "")
> );
>
>@@ -620,7 +620,7 @@ TRACE_EVENT(i915_gem_evict_node,
>
> TP_STRUCT__entry(
> __field(u32, dev)
>- __field(struct i915_address_space *, vm)
>+ __field(unsigned long, vm)
> __field(u64, start)
> __field(u64, size)
> __field(unsigned long, color)
>@@ -629,7 +629,7 @@ TRACE_EVENT(i915_gem_evict_node,
>
> TP_fast_assign(
> __entry->dev = vm->i915->drm.primary->index;
>- __entry->vm = vm;
>+ __entry->vm = (unsigned long)vm;
> __entry->start = node->start;
> __entry->size = node->size;
> __entry->color = node->color;
>@@ -637,7 +637,7 @@ TRACE_EVENT(i915_gem_evict_node,
> ),
>
> TP_printk("dev=%d, vm=%p, start=0x%llx size=0x%llx, color=0x%lx,
>flags=%x",
>- __entry->dev, __entry->vm,
>+ __entry->dev, (void *)__entry->vm,
> __entry->start, __entry->size,
> __entry->color, __entry->flags)
> );
>@@ -648,15 +648,15 @@ TRACE_EVENT(i915_gem_evict_vm,
>
> TP_STRUCT__entry(
> __field(u32, dev)
>- __field(struct i915_address_space *, vm)
>+ __field(unsigned long, vm)
> ),
>
> TP_fast_assign(
> __entry->dev = vm->i915->drm.primary->index;
>- __entry->vm = vm;
>+ __entry->vm = (unsigned long)vm;
> ),
>
>- TP_printk("dev=%d, vm=%p", __entry->dev, __entry->vm)
>+ TP_printk("dev=%d, vm=%p", __entry->dev, (void *)__entry->vm)
> );
>
> TRACE_EVENT(i915_request_queue,
>@@ -922,16 +922,16 @@ DECLARE_EVENT_CLASS(i915_ppgtt,
> TP_ARGS(vm),
>
> TP_STRUCT__entry(
>- __field(struct i915_address_space *, vm)
>+ __field(unsigned long, vm)
> __field(u32, dev)
> ),
>
> TP_fast_assign(
>- __entry->vm = vm;
>+ __entry->vm = (unsigned long)vm;
> __entry->dev = vm->i915->drm.primary->index;
> ),
>
>- TP_printk("dev=%u, vm=%p", __entry->dev, __entry->vm)
>+ TP_printk("dev=%u, vm=%p", __entry->dev, (void *)__entry->vm)
> )
>
> DEFINE_EVENT(i915_ppgtt, i915_ppgtt_create,
>@@ -957,20 +957,20 @@ DECLARE_EVENT_CLASS(i915_context,
>
> TP_STRUCT__entry(
> __field(u32, dev)
>- __field(struct i915_gem_context *, ctx)
>+ __field(unsigned long, ctx)
> __field(u32, hw_id)
>- __field(struct i915_address_space *, vm)
>+ __field(unsigned long, vm)
> ),
>
> TP_fast_assign(
> __entry->dev = ctx->i915->drm.primary->index;
>- __entry->ctx = ctx;
>+ __entry->ctx = (unsigned long)ctx;
> __entry->hw_id = ctx->hw_id;
>- __entry->vm = ctx->vm;
>+ __entry->vm = (unsigned long)ctx->vm;
> ),
>
> TP_printk("dev=%u, ctx=%p, ctx_vm=%p, hw_id=%u",
>- __entry->dev, __entry->ctx, __entry->vm, __entry->hw_id)
>+ __entry->dev, (void *)__entry->ctx, (void *)__entry->vm,
>__entry->hw_id)
> )
>
> DEFINE_EVENT(i915_context, i915_context_create,
>--- a/include/linux/overflow.h
>+++ b/include/linux/overflow.h
>@@ -31,7 +31,7 @@
> * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
> * credit to Christian Biere.
> */
>-#define is_signed_type(type) (((type)(-1)) < (type)1)
>+#define is_signed_type(type) ((type)-1 < 0)
> #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 -
>is_signed_type(type)))
> #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
> #define type_min(T) ((T)((T)-type_max(T)-(T)1))
>--- a/include/linux/trace_events.h
>+++ b/include/linux/trace_events.h
>@@ -546,7 +546,7 @@ extern int trace_add_event_call(struct
>trace_event_call *call);
> extern int trace_remove_event_call(struct trace_event_call *call);
> extern int trace_event_get_offsets(struct trace_event_call *call);
>
>-#define is_signed_type(type) (((type)(-1)) < (type)1)
>+#define is_signed_type(type) ((type)-1 < 0)
>
> int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
> int trace_set_clr_event(const char *system, const char *event, int set);
>--- a/tools/include/linux/overflow.h
>+++ b/tools/include/linux/overflow.h
>@@ -31,7 +31,7 @@
> * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
> * credit to Christian Biere.
> */
>-#define is_signed_type(type) (((type)(-1)) < (type)1)
>+#define is_signed_type(type) ((type)-1 < 0)
> #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 -
>is_signed_type(type)))
> #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
> #define type_min(T) ((T)((T)-type_max(T)-(T)1))
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Next message: Pavel Begunkov (Silence): "[PATCH v2 0/2] Simplify blk_mq_in_flight*"
Previous message: Liang, Kan: "Re: [PATCH V4 08/14] perf/x86/intel: Support per thread RDPMC TopDown metrics"
In reply to: Rasmus Villemoes: "Re: [PATCH] Make is_signed_type() simpler"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]