From mboxrd@z Thu Jan 1 00:00:00 1970 From: Edward Cree Subject: Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel Date: Fri, 13 Apr 2018 16:59:07 +0100 Message-ID: References: <20180412.113300.475644883265871487.davem@davemloft.net> <97aad935-d5fb-713e-fd0f-d84bbd733a8f@solarflare.com> <20180413.110359.1956367740053085545.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Cc: , To: David Miller Return-path: Received: from dispatch1-us1.ppe-hosted.com ([148.163.129.52]:33100 "EHLO dispatch1-us1.ppe-hosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750824AbeDMP7N (ORCPT ); Fri, 13 Apr 2018 11:59:13 -0400 In-Reply-To: <20180413.110359.1956367740053085545.davem@davemloft.net> Content-Language: en-GB Sender: netdev-owner@vger.kernel.org List-ID: On 13/04/18 16:03, David Miller wrote: > Whilst you may not be able to program the filter into the hardware > synchronously, you should be able to allocate the ID and get all of > the software state setup. That's what we were doing before commit 3af0f34290f6 ("sfc: replace  asynchronous filter operations"), and (as mentioned in that commit)  that leads to (or at least the scheme we used had) race conditions  which I could not see a way to fix.  If the hardware resets (and  thus forgets all its filters) after we've done the software state  setup but before we reach the point of finalising the software state  after the hardware operation, we don't know what operations we need  to do to re-apply the software state to the hardware, because we  don't know whether the reset happened before or after the hardware  operation. Actually, it's not the insertion itself that this can happen to - if  the reset happens first then the insert will fail because other  hardware state we reference isn't set up yet.  However, inserting  one filter can necessitate removing some others (lower-priority  multicast filters for the same flow); the code before 3af0f34290f6  was actually capable of getting so confused that it could double-  free a pointer. This all gets sufficiently complicated that even if I can find a  locking scheme that in theory gets it right, there's pretty much a  100% chance that the actual implementation will contain bugs,  probably very subtle ones that can't reliably be reproduced etc.  All my instincts say to get away from that if at all possible. > People really aren't going to be happy if their performance goes into > the tank because their filter update rate, for whatever reason, hits > this magic backlog limit. Well, the alternative, even if the software state setup part _could_  be made synchronous, is to allow a potentially unbounded queue for  the hardware update part (I think there are even still cases in  which the exponential growth pathology is possible), causing the  filter insertions to be delayed an arbitrarily long time.  Either  the flow is still going by that time (in which case the backlog  limit approach will get a new ndo_rx_flow_steer request and insert  the filter too) or it isn't, in which case getting round to it  eventually is no better than dropping it immediately.  In fact it's  worse because now you waste time inserting a useless filter which  delays new requests even more. Besides, I'm fairly confident that the only cases in which you'll  even come close to hitting the limit are ones where ARFS wouldn't  do you much good anyway, such as: * Misconfigured interrupt affinities where ARFS is entirely pointless * Many short-lived flows (which greatly diminish the utility of ARFS) So for multiple reasons, hitting the limit won't actually make  performance worse, although it will often be a sign that performance  will be bad for other reasons. -Ed