| Message ID | 20170621231613.20619-1-eric@engestrom.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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?
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)
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);