From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3E4F9A04B3; Mon, 10 Feb 2020 17:34:03 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E6E514C90; Mon, 10 Feb 2020 17:34:02 +0100 (CET) Received: from mail-il1-f195.google.com (mail-il1-f195.google.com [209.85.166.195]) by dpdk.org (Postfix) with ESMTP id 76D794C7B for ; Mon, 10 Feb 2020 17:34:01 +0100 (CET) Received: by mail-il1-f195.google.com with SMTP id l4so772793ilj.1 for ; Mon, 10 Feb 2020 08:34:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=F7NdEixsZb3Z7OfUpHcULowLsP04Szzyj5iatp14Yv4=; b=veGfKkaEELG5ztJ8OUOHtASUbHq4uqZRDxT1UIj5tuINK6t88k1PQ07V2gFqoI/ABL LqZSwVSs/K9ij1Evm3fbEjs8uYppop0uZya2gJ2I6v7h6YlQxOmnR7Ltiy8Vf7RcukmY hCRtuNku3PGsoWMOfrJIVx7Dy4BwuPFlR+0Y2oO9Bs+FbRuBVFjUZHiDdRfTpqGZaHR2 NB7M3bEPF+vyP2PnavEOpxvyFZC8Pq6KNGHPEceGEWR5oYIgB6GGzr9QwUe9VaVpgHPt aYZRcuaTOeBI1DCyo4RVTFXGhlBNTva/Bby0vGrJ4E+pEkbj3Q3NW1zrqnYiZ+yb0ehq 54Iw== 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:content-transfer-encoding; bh=F7NdEixsZb3Z7OfUpHcULowLsP04Szzyj5iatp14Yv4=; b=GaImcITDlqjRpXflILCBYbHtJ3pt1tNYjlF99RgVRW6andGVX8yd6rhv14KCZrmt62 59IWU9Sz3h64oSb4tlha1gG+GljMO7HrlZgdsg/2CImelNTE3H68sswVyK5hNt4nUqs9 YB9ZjH2hosmTqLap1MBSP3K/a3nI+9gFc+A+TbonaoAPDF1tgYIkyJQHlFI7hM2w9hp7 VFdaLJx4VoQAzKYBAgLybK0jL5rAwNmFSPdG40uSYmnt7UbKtHzfe4SZff7cM1jUOqdM 07ZoF1YRVoRPo3ByS/hmS/S9s1/E9JlGtMG2VGHptNgsWV4jZgkezBA8cldYUAzruzOD wKOQ== X-Gm-Message-State: APjAAAXBlWEDo4pf2CBkVwvtphjUjGUgv95svGR+b39nw+zXO3OgfJ7V c9/dvxgbLhGFUmctJWQBKp69T4iyqXMkOsv5oXg= X-Google-Smtp-Source: APXvYqyo6CAnbMaklKUuqLrPMJkQCSCnGp/Fqw8j9+IxFBu2rFUmCqRnF8AiSIJIYRo7Enrp0VwPz+qA61+luj47eic= X-Received: by 2002:a92:1906:: with SMTP id 6mr2302023ilz.130.1581352440628; Mon, 10 Feb 2020 08:34:00 -0800 (PST) MIME-Version: 1.0 References: <790778b3-8d6c-0ec2-79dd-7fb254f021cd@u256.net> <2593334.B0Pyx7erxp@xps> In-Reply-To: <2593334.B0Pyx7erxp@xps> From: Jerin Jacob Date: Mon, 10 Feb 2020 22:03:43 +0530 Message-ID: To: Thomas Monjalon Cc: Jerin Jacob Kollanukkaran , Gaetan Rivet , Pavan Nikhilesh Bhagavatula , Vamsi Krishna Attunuru , David Marchand , dpdk-dev , "Richardson, Bruce" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v7] eal: add manual probing option X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Feb 10, 2020 at 8:57 PM Thomas Monjalon wrote= : > > 10/02/2020 15:51, Jerin Jacob: > > On Tue, Feb 4, 2020 at 9:32 PM Gaetan Rivet wrote: > > > > > > On 04/02/2020 16:06, Thomas Monjalon wrote: > > > > 04/02/2020 13:43, Gaetan Rivet: > > > >> On 04/02/2020 12:07, Thomas Monjalon wrote: > > > >>> 04/02/2020 11:03, Gaetan Rivet: > > > >>>> On 03/02/2020 23:21, Thomas Monjalon wrote: > > > >>>>> 03/02/2020 06:16, Pavan Nikhilesh Bhagavatula: > > > >>>>>> @David Marchand @thomas@monjalon.net > > > >>>>>> > > > >>>>>> Ping? > > > >>>>>> > > > >>>>>> Are there any more changes required for this patch? It's been = in queue since last October. > > > >>>>> > > > >>>>> Sorry we have not decided whether it is a good idea or not. > > > >>>>> > > > >>>>> All changes related to probing are very sensitive, > > > >>>>> and we know a big refactoring would be better than stacking > > > >>>>> more and more options and corner cases. > > > >>>>> > > > >>>>> As we are busy with ABI stability stuff, we did not allocate > > > >>>>> enough time to properly think about this feature. > > > >>>>> Please accept our apologies, and let's consider it as > > > >>>>> a high priority for 20.05 cycle. > > > >>>>> > > > >>>> > > > >>>> Hello Thomas, > > > >>>> > > > >>>> This is unfortunate. I pushed Pavan to accept an alternative imp= lementation of this functionality that was less obtrusive, to make the inte= gration smoother. I took care to alleviate those risks from the common path= . > > > >>>> > > > >>>> The big refactoring is needed yes, but considering the current p= ath I'm not seeing it happen in 20.05. If that means taking this patch as-i= s in 20.05 for Marvell users, I'm not sure much is gained from waiting 3 mo= nths, except minimal risk avoidance. > > > >>> > > > >>> > > > >>> Yes, life is full of bad decisions and consequences. > > > >> > > > >> > > > >> Ah, yes, but I stand by my initial opinion, the first implementati= on [1] was riskier and less useful. > > > >> > > > >>> > > > >>> I still think there is a risk in adding new user expectations, > > > >>> and maintaining some code to workaround unknown issues. > > > >>> > > > >>> The real question here is to know why this patch? > > > >>> Is it to workaround a broken driver? > > > >>> Or to workaround a broken design in EAL and bus drivers? > > > >> > > > >> Two birds - one stone here: OVS needed a way to disable automatic = probing cleanly (current workaround seen in multiple deployment is to add a= dummy whitelisted device, which will be ignored by the PCI bus --> it sets= the bus in whitelist mode but avoid probing anything), and as a bonus this= option allows using devices that depends on other devices being probed alr= eady (LAG, representors, failsafe, etc). > > > >> > > > >> I'm not sure having a dependent-probe by default is good, and that= would be a big change. > > > >> > > > >> If we are doing the genesis of this patch, the initial motivation = should be asked for more details from Marvell people and David for the OVS = side. > > > >> > > > >> [1]: First proposal: > > > >> http://mails.dpdk.org/archives/dev/2019-September/144166.= html > > > >> My arguments: > > > >> http://mails.dpdk.org/archives/dev/2019-September/144564.= html > > > > > > > > > > > > OK so there are two needs: > > > > > > > > 1/ Better control whitelist/blacklist mode. > > > > We already know that a rework is needed here. > > > > Unfortunately neither you or me had time to work on it, > > > > and others who were interested disappeared. > > > > > > > > 2/ Associate ports with equivalent properties in applications. > > > > This must be done in applications. > > > > Tweaking the probe order is a hack. > > > > > > > > > > > > > > An application that want to tightly control the port init order, curr= ently (by doing exactly like me here, hotpluging one by one the ports), wou= ld still need the bigger hack that consist in inserting a whitelist PCI dev= args with a dummy address, depending on a undocumented PCI bus feature cons= isting in ignoring matching errors but keeping probing policy from failed d= evargs processing. > > > > > > Instead, with this patch this app can do > > > > > > rte_eal_manual_probe_set(1); > > > rte_eal_init(); > > > > > > to have the same behavior and be able to hotplug ports as it sees fit= . > > > > > > You are worried about creating user expectations about this behavior = (being forced to replicate in some way the functionality during the rewrite= , as I understand it?), but then you are currently forcing users to expect = this workaround to exist in the PCI bus, blocking devs from touching it as = it will thus break current app configurations. I've seen systemd unit file = using this -w dummy flag, as well as the programmatic equivalent. Which is = better, to have to rework it cutting short these configs, or to propose bef= orehand a deprecation path?. > > > > > > This rework won't happen in 20.05, nor in the medium term unless you = decide to drive this change. This workaround serves three needs (PCI normal= ization, port congruence and port dependency) in a low-risk implementation. > > > > Thomas, > > > > What would be the resolution of this? What is your recommendation to > > fix the issue as you have the concern of this patch? > > > > Issue: > > 1) When l2fwd does the forwarding for simplicity and performance > > reason it just xor the port to find the destination port to forward. > > 2) If the adjacent ports are not symmetrical(example: one is 40G and > > other 10G) then forwarding will drop the packets. > > > > So, either > > a) We need to control the probing order > > > > b) Or Application need > > 1) To track the symmetrical ports and maintain the forwarding table OR > > 2) Have the command-line option to specify destination port like l3fwd. > > > > We can fix it in the application, but do we need to complicate l2fwd? > > I am fine with that, if that is consensus. > > You are describing an application issue, > that's why I believe it should be fixed in the application. Thanks for the quick reply and I agree. > > Should we have a command line option to configure the forwarding rules > in the application (2)? I think yes. > Should we implement an application logic to automatically create > the best forwarding rules (1)? It would be nice, but anyway, > I think we need manual config (2) as a fallback. > > > > Thoughts? If you think, there is a rework needed in eal then could you > > enumerate the items for the rework. > > Sorry I don't have time to describe dive into EAL probing and > enumerate the items to rework. > The most important issues I remind are: > - white/blacklist policy is a mess and should be done in a higher= layer > - devargs syntax should allow generic matching (thanks to class a= wareness and generic syntax) > > Starting from these 2 items, we could imagine a generic path to > disable automatic probing, but I think the l2fwd logic should not > rely on probing order anyway. + Bruce as l2fwd maintainer. Since in any case, l2fwd needs to be updated, We will focus on l2fwd change for v20.05 and leaving the fate of this patch to EAL maintainers. Let us know, Are we are OK with below change in l2fwd as - Introduce an array-based port lookup table instead of hardcoding to xor based lookup. - if no argument specified fill dest port as xor of source - If argument is specified override the lookup table with a user-specified destination port. > >