From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D45B2C76194 for ; Thu, 25 Jul 2019 15:39:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A491122BF5 for ; Thu, 25 Jul 2019 15:39:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="nmCIp8hq" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728768AbfGYPjy (ORCPT ); Thu, 25 Jul 2019 11:39:54 -0400 Received: from mail-ua1-f66.google.com ([209.85.222.66]:42490 "EHLO mail-ua1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727081AbfGYPjy (ORCPT ); Thu, 25 Jul 2019 11:39:54 -0400 Received: by mail-ua1-f66.google.com with SMTP id a97so19991406uaa.9 for ; Thu, 25 Jul 2019 08:39:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=zUSY0yNX7mjAe5hgCsuzMt6JoI+T3veQET0Qe3Sg7bs=; b=nmCIp8hqFv2u364QTeF1hoE1hDHQBhUlMI7TLjTtUqlM+25nPEpVMXgqAJLBrL8W0E KIj2AH3i+7XfPuNPCPGSvPkgR0tOh0YPzOZGrx8Hwrh+I0lAOo36NobtMFOnjg6kl3YJ Qk+Lxh1O2J1WrPHMivAvH/rBYsTe2Iw+lYFPQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zUSY0yNX7mjAe5hgCsuzMt6JoI+T3veQET0Qe3Sg7bs=; b=ZxQSDf5oxUYeP7LC7D7IyI9G2IyWIpQTWLB2wPiYQ6Z4dOtFWBjLJ7DrR0QViUmAA1 3HPb6yKAIWRm0Sf//oDHCOZgcNmC1OKUT8dMS0Az0Xf6neGiVCVJWkoudcCAYsPLVNEg /4ScA02zSxYQLkwxwN1cAkv2syUvc9YJUuq3pnMsDruZl2vJrA5is7PMpg9yN7RX608y 7O+EHJ9OVu4HkVGLT775LvXwWTQKDP8euLHSp+xwS5PXXAte+mXBp4Ry9um5zpeYJeFn 4KVZEOWoPaHazlnd9CWhZsUvHkmSPkfsVRu9/6/szwOiUabSmo50gOhkqb49Ua70Sjf3 y/rg== X-Gm-Message-State: APjAAAVLXka1nI2fzolDeLUftbz9K5s+M0HStGiYkbkDi6YRRpi6c0V8 1625Xz8UcqK4bN+EGPi2vuIF6nTo6zc= X-Google-Smtp-Source: APXvYqxl+SClCjVT57OoIYDP54zYmK/X3HAfMpuGzmpzEOMqx3OB6uZNjkLesuEz6+N5Ja7NsbYCCQ== X-Received: by 2002:ab0:2906:: with SMTP id v6mr39544860uap.96.1564069192281; Thu, 25 Jul 2019 08:39:52 -0700 (PDT) Received: from mail-ua1-f48.google.com (mail-ua1-f48.google.com. [209.85.222.48]) by smtp.gmail.com with ESMTPSA id u19sm12756460uau.7.2019.07.25.08.39.51 for (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Thu, 25 Jul 2019 08:39:51 -0700 (PDT) Received: by mail-ua1-f48.google.com with SMTP id c4so20013062uad.1 for ; Thu, 25 Jul 2019 08:39:51 -0700 (PDT) X-Received: by 2002:ab0:1391:: with SMTP id m17mr52819855uae.90.1564069190798; Thu, 25 Jul 2019 08:39:50 -0700 (PDT) MIME-Version: 1.0 References: <20190722215340.3071-1-ilina@codeaurora.org> <20190722215340.3071-2-ilina@codeaurora.org> <5d3769df.1c69fb81.55d03.aa33@mx.google.com> <20190724145251.GB18620@codeaurora.org> <5d38b38e.1c69fb81.e8e5d.035b@mx.google.com> <20190724203610.GE18620@codeaurora.org> <20190725151851.GG18620@codeaurora.org> In-Reply-To: <20190725151851.GG18620@codeaurora.org> From: Doug Anderson Date: Thu, 25 Jul 2019 08:39:30 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler To: Lina Iyer Cc: Stephen Boyd , Andy Gross , Bjorn Andersson , linux-arm-msm , "open list:ARM/QUALCOMM SUPPORT" , Rajendra Nayak , LKML , Linux PM , mkshah@codeaurora.org Content-Type: text/plain; charset="UTF-8" Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org Hi, On Thu, Jul 25, 2019 at 8:18 AM Lina Iyer wrote: > > On Wed, Jul 24 2019 at 17:28 -0600, Doug Anderson wrote: > >Hi, > > > >On Wed, Jul 24, 2019 at 1:36 PM Lina Iyer wrote: > >> > >> On Wed, Jul 24 2019 at 13:38 -0600, Stephen Boyd wrote: > >> >Quoting Lina Iyer (2019-07-24 07:52:51) > >> >> On Tue, Jul 23 2019 at 14:11 -0600, Stephen Boyd wrote: > >> >> >Quoting Lina Iyer (2019-07-22 14:53:38) > >> >> >> Avoid locking in the interrupt context to improve latency. Since we > >> >> >> don't lock in the interrupt context, it is possible that we now could > >> >> >> race with the DRV_CONTROL register that writes the enable register and > >> >> >> cleared by the interrupt handler. For fire-n-forget requests, the > >> >> >> interrupt may be raised as soon as the TCS is triggered and the IRQ > >> >> >> handler may clear the enable bit before the DRV_CONTROL is read back. > >> >> >> > >> >> >> Use the non-sync variant when enabling the TCS register to avoid reading > >> >> >> back a value that may been cleared because the interrupt handler ran > >> >> >> immediately after triggering the TCS. > >> >> >> > >> >> >> Signed-off-by: Lina Iyer > >> >> >> --- > >> >> > > >> >> >I have to read this patch carefully. The commit text isn't convincing me > >> >> >that it is actually safe to make this change. It mostly talks about the > >> >> >performance improvements and how we need to fix __tcs_trigger(), which > >> >> >is good, but I was hoping to be convinced that not grabbing the lock > >> >> >here is safe. > >> >> > > >> >> >How do we ensure that drv->tcs_in_use is cleared before we call > >> >> >tcs_write() and try to look for a free bit? Isn't it possible that we'll > >> >> >get into a situation where the bitmap is all used up but the hardware > >> >> >has just received an interrupt and is going to clear out a bit and then > >> >> >an rpmh write fails with -EBUSY? > >> >> > > >> >> If we have a situation where there are no available free bits, we retry > >> >> and that is part of the function. Since we have only 2 TCSes avaialble > >> >> to write to the hardware and there could be multiple requests coming in, > >> >> it is a very common situation. We try and acquire the drv->lock and if > >> >> there are free TCS available and if available mark them busy and send > >> >> our requests. If there are none available, we keep retrying. > >> >> > >> > > >> >Ok. I wonder if we need some sort of barriers here too, like an > >> >smp_mb__after_atomic()? That way we can make sure that the write to > >> >clear the bit is seen by another CPU that could be spinning forever > >> >waiting for that bit to be cleared? Before this change the spinlock > >> >would be guaranteed to make these barriers for us, but now that doesn't > >> >seem to be the case. I really hope that this whole thing can be changed > >> >to be a mutex though, in which case we can use the bit_wait() API, etc. > >> >to put tasks to sleep while RPMh is processing things. > >> > > >> We have drivers that want to send requests in atomic contexts and > >> therefore mutex locks would not work. > > > >Jumping in without reading all the context, but I saw this fly by and > >it seemed odd. If I'm way off base then please ignore... > > > >Can you give more details? Why are these drivers in atomic contexts? > >If they are in atomic contexts because they are running in the context > >of an interrupt then your next patch in the series isn't so correct. > > > >Also: when people submit requests in atomic context are they always > >submitting an asynchronous request? In that case we could > >(presumably) just use a spinlock to protect the queue of async > >requests and a mutex for everything else? > Yes, drivers only make async requests in interrupt contexts. So correct me if I'm off base, but you're saying that drivers make requests in interrupt contexts even after your whole series and that's why you're using spinlocks instead of mutexes. ...but then in patch #3 in your series you say: > Switch over from using _irqsave/_irqrestore variants since we no longer > race with a lock from the interrupt handler. Those seem like contradictions. What happens if someone is holding the lock, then an interrupt fires, then the interrupt routine wants to do an async request. Boom, right? > They cannot > use the sync variants. The async and sync variants are streamlined into > the same code path. Hence the use of spinlocks instead of mutexes > through the critical path. I will perhaps defer to Stephen who was the one thinking that a mutex would be a big win here. ...but if a mutex truly is a big win then it doesn't seem like it'd be that hard to have a linked list (protected by a spinlock) and then some type of async worker that: 1. Grab the spinlock, pops one element off the linked list, release the spinlock 2. Grab the mutex, send the one element, release the mutex 3. Go back to step #1. This will keep the spinlock held for as little time as possible. -Doug