Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933150AbbHZLhq (ORCPT ); Wed, 26 Aug 2015 07:37:46 -0400 Received: from mga03.intel.com ([134.134.136.65]:62546 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751859AbbHZLhp (ORCPT ); Wed, 26 Aug 2015 07:37:45 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,415,1437462000"; d="scan'208";a="775951425" From: Alexander Shishkin To: Ingo Molnar , Johannes Berg Cc: Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, adrian.hunter@intel.com, Arnaldo Carvalho de Melo , Vince Weaver , Stephane Eranian , Linus Torvalds , Andrew Morton , Thomas Gleixner , Borislav Petkov , "H. Peter Anvin" Subject: Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting In-Reply-To: <20150825091740.GA23488@gmail.com> References: <1440426780-27227-1-git-send-email-alexander.shishkin@linux.intel.com> <1440492739.2192.7.camel@sipsolutions.net> <20150825090252.GB22414@gmail.com> <20150825091740.GA23488@gmail.com> User-Agent: Notmuch/0.20.2 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Wed, 26 Aug 2015 14:37:40 +0300 Message-ID: <87twrmgu0r.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4463 Lines: 103 Ingo Molnar writes: > * Ingo Molnar wrote: > >> >> * Johannes Berg wrote: >> >> > On Mon, 2015-08-24 at 17:32 +0300, Alexander Shishkin wrote: >> > >> > > This time around, I employed a linker trick to convert the structures >> > > containing extended error information into integers, which are then made to >> > > look just like normal error codes so that IS_ERR_VALUE() and friends would >> > > still work correctly on them. So no extra pointers in the struct perf_event >> > > or anywhere else; the extended error codes are passed around like normal >> > > error codes. They only need to be converted in syscalls' topmost return >> > > statements. This is done in 1/6. >> > >> > For the record, as we discussed separately, I'd love to see this move to more >> > general infrastructure. In wireless (nl80211), for example, we have a few >> > hundred (!) callsites returning -EINVAL, mostly based on malformed netlink >> > attributes, and it can be very difficult to figure out what went wrong; >> > debugging mostly employs a variation of Hugh's trick. >> >> Absolutely, I suggested this as well earlier today, as the scheduler would like >> to make use of it in syscalls with extensible ABIs, such as sched_setattr(). >> >> If people really like this then we could go farther as well and add a standalone >> 'extended errors system call' as well (SyS_errno_extended_get()), which would >> allow the recovery of error strings even for system calls that are not easily >> extensible. We could cache the last error description in the task struct. > > If we do that then we don't even have to introduce per system call error code > conversion, but could unconditionally save the last extended error info in the > task struct and continue - this could be done very cheaply with the linker trick > driven integer ID. > > I.e. system calls could opt in to do: > > return err_str(-EBUSY, "perf/x86: BTS conflicts with active events"); > > and the overhead of this would be minimal, we'd essentially do something like this > to save the error: > > current->err_code = code; > > where 'code' is a build time constant in essence. I'd propose a mixed approach here: err_str() would still return an integer in the [-EXT_ERRNO, -MAX_ERRNO] range which would index the err_site struct and upon returning to userspace we'd do current->err_code = code; return ext_errno(code); /* the traditional errno */ Reason: the lifetime of this extended error code would be exactly the same as that of the traditional error value so that we'd always return the most recent error and wouldn't be prone to something overwriting the error code under us. The problem with code checking for different types of errors has two sides to it: * most of those error codes that are check for shouldn't really be annotated at all and should rather remain like they are; * with the ones that actually do need to be checked for, the checks would change from "if (err == EINTR)" to "if (ext_errno(err) == EINTR)", which doesn't seem like a big deal (with ext_errno() being a O(1) lookup). Side note: we should also make sure that only the userspace-visible errors ever get annotated like that to prevent the error message creep (which would be even a bigger problem if we go ahead to store the extended error code in task_struct right at the topmost return statement). Perf example: pretty much all errors that happen around event scheduling, including stuff that pmu callbacks return, needn't and shouldn't be annotated at all. > We could use this even in system calls where the error path is performance > critical, as all the string recovery and copying overhead would be triggered by > applications that opt in via the new system call: > > struct err_desc { > const char *message; > const char *owner; > const int code; > }; > > SyS_err_get_desc(struct err_desc *err_desc __user); > > [ Which could perhaps be a prctl() extension as well (PR_GET_ERR_DESC): finally > some truly matching functionality for prctl(). ] > > Hm? I like this. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/