From patchwork Mon Sep 28 08:52:38 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 523265 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 562B91401AD for ; Mon, 28 Sep 2015 18:52:58 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=aMRsoJ92; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; q=dns; s=default; b=KrVfgEieGufKtirg XUFZxWruECOS+gbgnPO8D+LgNwpqtFXp8Tt6PfX2vt90LfABV+/r6LKBjWoyfznI 1eoCDZHq/VNlnew94pURu8hznEniJdGkNsEv9JAZdrQ5Cc7jeIvQ3gkTpHLbqPiq c+h9QQoFhSNXPanttwi0XBzuzYI= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; s=default; bh=qPuN8kFvD9I+F67GTC16Hk TkeRE=; b=aMRsoJ92J4E8rPVcAxbT3APiA3Iz6WEe9NOjXlXju1Yi7eko8n13HY GthqO60ZCyvHw32DGB18ymFSYvIO4KHn+1mtwtAAdkt5V6G5ZECKZzjr3FJsAOJw oveSxF7kesMwLNeMljDSHhZccty7j/n2RrsdLH08k+4v/KZ8bHtjY= Received: (qmail 41367 invoked by alias); 28 Sep 2015 08:52:51 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 41356 invoked by uid 89); 28 Sep 2015 08:52:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 28 Sep 2015 08:52:49 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-01.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1ZgUAv-0007fx-Cf from Thomas_Schwinge@mentor.com ; Mon, 28 Sep 2015 01:52:45 -0700 Received: from feldtkeller.schwinge.homeip.net (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.3.224.2; Mon, 28 Sep 2015 09:52:43 +0100 From: Thomas Schwinge To: Jakub Jelinek , Ilya Verbin CC: Julian Brown , , Kirill Yukhin Subject: libgomp: Guard all devices/num_devices/num_devices_openmp access by register_lock (was: libgomp: Guard all offload_images/num_offload_images access by register_lock) In-Reply-To: <20150925164950.GB29922@msticlxl57.ims.intel.com> References: <20150204150545.2628ad35@octopus> <20150224112951.363b71fd@octopus> <87a902ib93.fsf@schwinge.name> <20150226172511.GA49293@msticlxl57.ims.intel.com> <20150306140113.GB26588@msticlxl57.ims.intel.com> <20150309144555.3a078f48@octopus> <20150323194439.GA12972@msticlxl57.ims.intel.com> <20150326120919.GZ1746@tucnak.redhat.com> <20150326204130.GA65474@msticlxl57.ims.intel.com> <877fnewjug.fsf@kepler.schwinge.homeip.net> <20150925164950.GB29922@msticlxl57.ims.intel.com> User-Agent: Notmuch/0.9-125-g4686d11 (http://notmuchmail.org) Emacs/24.5.1 (i586-pc-linux-gnu) Date: Mon, 28 Sep 2015 10:52:38 +0200 Message-ID: <87d1x3udrd.fsf@kepler.schwinge.homeip.net> MIME-Version: 1.0 Hi! On Fri, 25 Sep 2015 19:49:50 +0300, Ilya Verbin wrote: > On Fri, Sep 25, 2015 at 18:21:27 +0200, Thomas Schwinge wrote: > > On Thu, 26 Mar 2015 23:41:30 +0300, Ilya Verbin wrote: > > > On Thu, Mar 26, 2015 at 13:09:19 +0100, Jakub Jelinek wrote: > > > > the current code is majorly broken. As I've said earlier, e.g. the lack > > > > of mutex guarding gomp_target_init (which is using pthread_once guaranteed > > > > to be run just once) vs. concurrent GOMP_offload_register calls > > > > (if those are run from ctors, then I guess something like dl_load_lock > > > > ensures at least on glibc that multiple GOMP_offload_register calls aren't > > > > performed at the same time) in accessing/reallocating offload_images > > > > and num_offload_images and the lack of support to register further > > > > images after the gomp_target_init call (if you dlopen further shared > > > > libraries) is really bad. And it would be really nice to support the > > > > unloading. > > > > > Here is the latest patch for libgomp and mic plugin. > > > > What about the scenario where one thread is inside > > GOMP_offload_register_ver/GOMP_offload_register (say, due to opening a > > shared library with such a mkoffload-generated constructor) and is > > modifying offload_images with register_lock held, and another thread is > > inside a GOMP_target* construct -> gomp_init_device and is accessing > > offload_images without register_lock held? Or, why isn't that a > > reachable scenario? > > > > Would the following patch (untested) do the right thing (locking added to > > gomp_init_device and gomp_unload_device)? We can then also remove the > > is_register_lock parameter from gomp_load_image_to_device, and simplify > > the code. > > Looks like you're right, and this scenario is possible. Thanks for your review! Jakub, OK to commit the patch I had posted? Then, in context of a similar scenario, I think we'll also want the following. Please confirm that my reasoning in gomp_get_num_devices and resolve_device is correct. OK for trunk? commit b0cf4dcc588e432c0a0d19d85727a20210b4d837 Author: Thomas Schwinge Date: Sat Sep 26 15:48:09 2015 +0200 libgomp: Guard all devices/num_devices/num_devices_openmp access by register_lock --- libgomp/target.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) Grüße, Thomas diff --git libgomp/target.c libgomp/target.c index 1fbbe31..6f0a339 100644 --- libgomp/target.c +++ libgomp/target.c @@ -49,7 +49,7 @@ static void gomp_target_init (void); /* The whole initialization code for offloading plugins is only run one. */ static pthread_once_t gomp_is_initialized = PTHREAD_ONCE_INIT; -/* Mutex for offload image registration. */ +/* Mutex for offload targets setup and image registration. */ static gomp_mutex_t register_lock; /* This structure describes an offload image. @@ -118,6 +118,8 @@ attribute_hidden int gomp_get_num_devices (void) { gomp_init_targets_once (); + /* As it is immutable once it has been initialized, it's safe to access + num_devices_openmp without register_lock held. */ return num_devices_openmp; } @@ -133,6 +135,8 @@ resolve_device (int device_id) if (device_id < 0 || device_id >= gomp_get_num_devices ()) return NULL; + /* As it is immutable once it has been initialized, it's safe to access + devices without register_lock held. */ return &devices[device_id]; } @@ -1228,6 +1232,8 @@ gomp_target_init (void) char *plugin_name; int i, new_num_devices; + gomp_mutex_lock (®ister_lock); + num_devices = 0; devices = NULL; @@ -1317,6 +1323,8 @@ gomp_target_init (void) if (devices[i].capabilities & GOMP_OFFLOAD_CAP_OPENACC_200) goacc_register (&devices[i]); } + + gomp_mutex_unlock (®ister_lock); } #else /* PLUGIN_SUPPORT */