From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751209AbdH1FOx (ORCPT ); Mon, 28 Aug 2017 01:14:53 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:52241 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750825AbdH1FOw (ORCPT ); Mon, 28 Aug 2017 01:14:52 -0400 Date: Sun, 27 Aug 2017 22:14:46 -0700 From: Sukadev Bhattiprolu To: Michael Ellerman Cc: Benjamin Herrenschmidt , mikey@neuling.org, stewart@linux.vnet.ibm.com, apopple@au1.ibm.com, hbabu@us.ibm.com, oohall@gmail.com, linuxppc-dev@ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 10/12] powerpc/vas: Define vas_win_close() interface References: <1503556688-15412-1-git-send-email-sukadev@linux.vnet.ibm.com> <1503556688-15412-11-git-send-email-sukadev@linux.vnet.ibm.com> <87valct177.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87valct177.fsf@concordia.ellerman.id.au> X-Operating-System: Linux 2.0.32 on an i486 User-Agent: Mutt/1.7.1 (2016-10-04) X-TM-AS-GCONF: 00 x-cbid: 17082805-0008-0000-0000-0000087C2507 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007625; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000225; SDB=6.00908658; UDB=6.00455620; IPR=6.00688899; BA=6.00005555; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016893; XFM=3.00000015; UTC=2017-08-28 05:14:50 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17082805-0009-0000-0000-000043BD7085 Message-Id: <20170828051446.GF12907@us.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-28_02:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1708280084 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Michael Ellerman [mpe@ellerman.id.au] wrote: > Hi Suka, > > More comments :) Thanks! > > Sukadev Bhattiprolu writes: > > > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c > > index 2dd4b63..24288dd 100644 > > --- a/arch/powerpc/platforms/powernv/vas-window.c > > +++ b/arch/powerpc/platforms/powernv/vas-window.c > > @@ -879,11 +887,92 @@ struct vas_window *vas_rx_win_open(int vasid, enum vas_cop_type cop, > > } > > EXPORT_SYMBOL_GPL(vas_rx_win_open); > > > > -/* stub for now */ > > +static void poll_window_busy_state(struct vas_window *window) > > +{ > > + int busy; > > + uint64_t val; > > + > > +retry: > > + /* > > + * Poll Window Busy flag > > + */ > > + val = read_hvwc_reg(window, VREG(WIN_STATUS)); > > + busy = GET_FIELD(VAS_WIN_BUSY, val); > > + if (busy) { > > + val = 0; > > + schedule_timeout(2000); > > What's 2000? > > That's in jiffies, so it's not a fixed amount of time. > > But on a typical config that will be 20 _seconds_ ?! Ok. Should I change to that just HZ and > > But you haven't set the task state, so AFAIK it will just return > instantly. call set_current_state(TASK_UNINTERRUPTIBLE) before the schedule_timeout()? > > And if there's a software/hardware bug and it never stops being busy, > then we have a softlockup. The other option would be print a big fat > warning and just not free the window. But maybe that doesn't work for > other reasons. > > > + goto retry; > > + } > > +} > > + > > +static void poll_window_castout(struct vas_window *window) > > +{ > > + int cached; > > + uint64_t val; > > + > > + /* Cast window context out of the cache */ > > +retry: > > + val = read_hvwc_reg(window, VREG(WIN_CTX_CACHING_CTL)); > > + cached = GET_FIELD(VAS_WIN_CACHE_STATUS, val); > > + if (cached) { > > + val = 0ULL; > > + val = SET_FIELD(VAS_CASTOUT_REQ, val, 1); > > + val = SET_FIELD(VAS_PUSH_TO_MEM, val, 0); > > + write_hvwc_reg(window, VREG(WIN_CTX_CACHING_CTL), val); > > Sigh, I still don't like that macro :) :-) For one thing, I have used it a lot now and secondly isn't it easier to know that VAS_CASTOUT_REQ bit is set to 1 without worrying about its bit position? When debugging, yes we have to ensure VAS_CASTOUT_REQ is properly defined and we have to work out value in "val". > > or: > write_hvwc_reg(window, VREG(WIN_CTX_CACHING_CTL), 1ull << 63); > > > + > > + schedule_timeout(2000); > > + goto retry; > > + } > > +} > > + > > +/* > > + * Close a window. > > + * > > + * See Section 1.12.1 of VAS workbook v1.05 for details on closing window: > > + * - Disable new paste operations (unmap paste address) > > + * - Poll for the "Window Busy" bit to be cleared > > + * - Clear the Open/Enable bit for the Window. > > + * - Poll for return of window Credits (implies FIFO empty for Rx win?) > > + * - Unpin and cast window context out of cache > > + * > > + * Besides the hardware, kernel has some bookkeeping of course. > > + */ > > int vas_win_close(struct vas_window *window) > > { > > - return -1; > > + uint64_t val; > > + > > + if (!window) > > + return 0; > > + > > + if (!window->tx_win && atomic_read(&window->num_txwins) != 0) { > > + pr_devel("VAS: Attempting to close an active Rx window!\n"); > > + WARN_ON_ONCE(1); > > + return -EAGAIN; > > EAGAIN means "if you do the same thing again it might work". > > I don't think that's right here. The window is not in a state where it > can be freed, the caller needs to do something to fix that. > > EBUSY would probably be more appropriate. Ok. Should not happen now (or even with the fast thread-wake up code) since only the kernel should be closing the windows - so its really a bug. Will change to EBUSY though. > > > cheers