[PATCH libdrm 1/2] xf86drm: inline drmIoctl()

[PATCH libdrm 1/2] xf86drm: inline drmIoctl()

[Eric Engestrom]

As Chad [1] puts it:
> it's excessive to jump through the PLT into a shared object for a mere
> ioctl wrapper.

[1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/159951.html

Signed-off-by: Eric Engestrom <eric@engestrom.ch>
---
 xf86drm.c | 14 --------------
 xf86drm.h | 18 +++++++++++++++++-
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index 728ac78c..c82a76d2 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -179,20 +179,6 @@ void drmFree(void *pt)
     free(pt);
 }
 
-/**
- * Call ioctl, restarting if it is interupted
- */
-int
-drmIoctl(int fd, unsigned long request, void *arg)
-{
-    int ret;
-
-    do {
-        ret = ioctl(fd, request, arg);
-    } while (ret == -1 && (errno == EINTR || errno == EAGAIN));
-    return ret;
-}
-
 static unsigned long drmGetKeyFromFd(int fd)
 {
     stat_t     st;
diff --git a/xf86drm.h b/xf86drm.h
index 74f54f17..aeed543f 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -38,6 +38,8 @@
 #include <sys/types.h>
 #include <stdint.h>
 #include <drm.h>
+#include <sys/ioctl.h>
+#include <errno.h>
 
 #if defined(__cplusplus)
 extern "C" {
@@ -118,7 +120,21 @@ typedef struct drmHashEntry {
     void     *tagTable;
 } drmHashEntry;
 
-extern int drmIoctl(int fd, unsigned long request, void *arg);
+/**
+ * Call ioctl, restarting if it is interupted
+ */
+static inline int
+drmIoctl(int fd, unsigned long request, void *arg)
+{
+    int ret;
+
+    do {
+        ret = ioctl(fd, request, arg);
+    } while (ret == -1 && (errno == EINTR || errno == EAGAIN));
+
+    return ret;
+}
+
 extern void *drmGetHashTable(void);
 extern drmHashEntry *drmGetEntry(int fd);
 
-- 
Cheers,
  Eric

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 2/2] xf86drm: introduce drmIoctl2()

[Eric Engestrom]

Basically ripped off of [1] by Chris Wilson, which I thought should live
in libdrm too (even though it's needed in Mesa anyway).

[1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/159894.html

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Eric Engestrom <eric@engestrom.ch>
---
 xf86drm.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/xf86drm.h b/xf86drm.h
index aeed543f..eb75b944 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -38,6 +38,9 @@
 #include <sys/types.h>
 #include <stdint.h>
 #include <drm.h>
+#if defined(__linux__) && defined(__GNUC__) && defined (__x86_64__)
+#include <sys/syscall.h>
+#endif
 #include <sys/ioctl.h>
 #include <errno.h>
 
@@ -135,6 +138,30 @@ drmIoctl(int fd, unsigned long request, void *arg)
     return ret;
 }
 
+/**
+ * Call ioctl, restarting if it is interupted, and return the error.
+ */
+static inline int
+drmIoctl2(int fd, unsigned long request, void *arg)
+{
+    int result;
+
+    do {
+#if defined(__linux__) && defined(__GNUC__) && defined (__x86_64__)
+        __asm__("syscall"
+                : "=a" (result)
+                : "0" (__NR_ioctl), "D" (fd), "S" (request), "d" (arg)
+                : "cc", "rcx", "r11", "memory");
+#else
+        result = ioctl(fd, request, arg);
+        if (result == -1)
+            result = -errno;
+#endif
+    } while (result == -EINTR || result == -EAGAIN);
+
+    return result;
+}
+
 extern void *drmGetHashTable(void);
 extern drmHashEntry *drmGetEntry(int fd);
 
-- 
Cheers,
  Eric

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/2] xf86drm: inline drmIoctl()

[Michel Dänzer]

On 22/06/17 08:16 AM, Eric Engestrom wrote:
> As Chad [1] puts it:
>> it's excessive to jump through the PLT into a shared object for a mere
>> ioctl wrapper.
> 
> [1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/159951.html
> 
> Signed-off-by: Eric Engestrom <eric@engestrom.ch>

NAK, at least in this form:


> diff --git a/xf86drm.c b/xf86drm.c
> index 728ac78c..c82a76d2 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -179,20 +179,6 @@ void drmFree(void *pt)
>      free(pt);
>  }
>  
> -/**
> - * Call ioctl, restarting if it is interupted
> - */
> -int
> -drmIoctl(int fd, unsigned long request, void *arg)
> -{
> -    int ret;
> -
> -    do {
> -        ret = ioctl(fd, request, arg);
> -    } while (ret == -1 && (errno == EINTR || errno == EAGAIN));
> -    return ret;
> -}

libdrm.so.2 must continue exporting a working drmIoctl symbol, otherwise
it's an ABI break.


Also, there are downsides to exposing library API calls as inline
functions: E.g. if drmIoctl(2) ever needs a change (worst case a
security bug fix), every user has to be recompiled to get the fix. It's
kind of like static linking through the back door. Is it really worth it
for drmIoctl(2)? Are they ever an actual bottleneck?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/2] xf86drm: inline drmIoctl()

[Eric Engestrom]

On Thursday, 2017-06-22 10:42:21 +0900, Michel Dänzer wrote:
> On 22/06/17 08:16 AM, Eric Engestrom wrote:
> > As Chad [1] puts it:
> >> it's excessive to jump through the PLT into a shared object for a mere
> >> ioctl wrapper.
> > 
> > [1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/159951.html
> > 
> > Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> 
> NAK, at least in this form:
> 
> 
> > diff --git a/xf86drm.c b/xf86drm.c
> > index 728ac78c..c82a76d2 100644
> > --- a/xf86drm.c
> > +++ b/xf86drm.c
> > @@ -179,20 +179,6 @@ void drmFree(void *pt)
> >      free(pt);
> >  }
> >  
> > -/**
> > - * Call ioctl, restarting if it is interupted
> > - */
> > -int
> > -drmIoctl(int fd, unsigned long request, void *arg)
> > -{
> > -    int ret;
> > -
> > -    do {
> > -        ret = ioctl(fd, request, arg);
> > -    } while (ret == -1 && (errno == EINTR || errno == EAGAIN));
> > -    return ret;
> > -}
> 
> libdrm.so.2 must continue exporting a working drmIoctl symbol, otherwise
> it's an ABI break.

Oops, you're absolutely right, my bad :/

> 
> 
> Also, there are downsides to exposing library API calls as inline
> functions: E.g. if drmIoctl(2) ever needs a change (worst case a
> security bug fix), every user has to be recompiled to get the fix. It's
> kind of like static linking through the back door. Is it really worth it
> for drmIoctl(2)? Are they ever an actual bottleneck?

The start of the conversation [1] was that the profiler was spending
more time going through drmIoctl() than the actual syscall, which
prompted Chris to write a local copy that would be inlined, and the fact
some of that time was spend converting the error returned into a `-1`
and set the error in `errno`, which is rather pointless from a caller's
perspective.

I simply took his proposal and applied it to libdrm, although I obviously
did so badly.

Is the updatability of a loop around ioctl() really an issue?
I can drop the first patch and respin the second as a normal dso function,
would that be something people are interested in?

[1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/159894.html

> 
> 
> -- 
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/2] xf86drm: inline drmIoctl()

[Michel Dänzer]

On 22/06/17 11:45 PM, Eric Engestrom wrote:
> On Thursday, 2017-06-22 10:42:21 +0900, Michel Dänzer wrote:
>>
>> Also, there are downsides to exposing library API calls as inline
>> functions: E.g. if drmIoctl(2) ever needs a change (worst case a
>> security bug fix), every user has to be recompiled to get the fix. It's
>> kind of like static linking through the back door. Is it really worth it
>> for drmIoctl(2)? Are they ever an actual bottleneck?
> 
> The start of the conversation [1] was that the profiler was spending
> more time going through drmIoctl() than the actual syscall,

Is that an actual problem, as opposed to a cosmetic issue? I.e. is there
any scenario where a metric is bounded by drmIoctl, and would otherwise
only be bounded by the actual syscall?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/2] xf86drm: inline drmIoctl()

[Eric Engestrom]

On 23 June 2017 03:21:11 BST, "Michel Dänzer" <michel@daenzer.net> wrote:
> On 22/06/17 11:45 PM, Eric Engestrom wrote:> 
> > On Thursday, 2017-06-22 10:42:21 +0900, Michel Dänzer wrote:> 
> >>> 
> >> Also, there are downsides to exposing library API calls as inline> 
> >> functions: E.g. if drmIoctl(2) ever needs a change (worst case a> 
> >> security bug fix), every user has to be recompiled to get the fix.
> It's> 
> >> kind of like static linking through the back door. Is it really
> worth it> 
> >> for drmIoctl(2)? Are they ever an actual bottleneck?> 
> > > 
> > The start of the conversation [1] was that the profiler was
> spending> 
> > more time going through drmIoctl() than the actual syscall,> 
> > 
> Is that an actual problem, as opposed to a cosmetic issue? I.e. is
> there> 
> any scenario where a metric is bounded by drmIoctl, and would
> otherwise> 
> only be bounded by the actual syscall?> 

I think this is a question for Chris (cc'ed)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel