From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3chas3@gmail.com> Received: from mail-it0-f42.google.com (mail-it0-f42.google.com [209.85.214.42]) by dpdk.org (Postfix) with ESMTP id A1807532C for ; Sun, 16 Sep 2018 18:14:23 +0200 (CEST) Received: by mail-it0-f42.google.com with SMTP id d10-v6so8212873itj.5 for ; Sun, 16 Sep 2018 09:14:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:sender:from:date:message-id :subject:to:cc:content-transfer-encoding; bh=2xIZkvu+QhgUFC68AS1SdYuG8sCvj7EshnIyK2ButwE=; b=rK50ZZe4cKOpwCpoB7ikaxG5j7JS7Wwk9mN2DAllRvYSPixnQlAT5do0j43586tqNC /Py8q3MQTpo5/8cRXujmcCUR6x9/KXvzBGXf6ukvu7o57xJuu/7dCYjMUkmsktYsF/kh /C9r4+KZimPaJDiENA6I4Yz6pil4sy3EHvuN0DYGieun6znj4OuUIHCJfK6Ze43bibK7 3yCSVwKncAJ165Yulin8JaeS3WOc/sJk7UVAKTpWekzD90sI2f5trWNWA3CdLaqL/DQw W9bk2t2dmC5jQNkRwWv08ILJAdzoeoRRsAYe7WgtBbOU/o7qXdLmRUtwCgSdYCVX2/Xw PPJg== 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:sender:from :date:message-id:subject:to:cc:content-transfer-encoding; bh=2xIZkvu+QhgUFC68AS1SdYuG8sCvj7EshnIyK2ButwE=; b=s6Z0nW3Rqb90AQZpBNAU5tnKPqeQ80xtZVUENMN/0v/Q2q5uRI65ogN9gcaqNsNZ3/ pcwm1owQJ/AP19T4JO3sVekA+W9mDcAan0Z8srXYAQWcr0LBYF/V0NJc6QNvQjwdo6cB p5AKQj5mRMuCE7akyykW5Il3yBK37WHh/opKJCgBIl/YD7tjwksXNqeQHgVeuaQD83fL u4LnmsYs9FBzK813UABlOrr5hU+eiwgntcj6peYSJ3h5OhK13sUSdTwlQ96obcN+rhcz GziKXd0yLIEnx8GtRmVhqy3c8nxBAk/SVE+A0bjulQpGJGDlwiFGiKmN2pi+Cr70VVKW LcKA== X-Gm-Message-State: APzg51BBk15V/Y1yxDydtGo0imXrtekID9l+wCmGug8ZraX3kbhzCjnn PFiFNEKqigp8+38n60dyr9rZb3Gz3C3u9X17UGM= X-Google-Smtp-Source: ANB0VdYBx6lQrx/YyXnSCmmoNIPGO4OqfLaFgDoPuGVcqpepSUXiv+Q1EcFenMCOl7Oss2irA61MIDzt4pCu6eztrbI= X-Received: by 2002:a24:cfd7:: with SMTP id y206-v6mr9622258itf.112.1537114462893; Sun, 16 Sep 2018 09:14:22 -0700 (PDT) MIME-Version: 1.0 References: <1533128278-4685-1-git-send-email-radu.nicolau@intel.com> <2eac631f-1402-67b5-04de-1ce161cfcf92@intel.com> <017918fc-70dc-e6d3-6e9f-35bf9bd73fc3@intel.com> In-Reply-To: Sender: chasmosaurus@gmail.com X-Google-Sender-Delegation: chasmosaurus@gmail.com From: Chas Williams <3chas3@gmail.com> Date: Sun, 16 Sep 2018 12:14:11 -0400 X-Google-Sender-Auth: p8Dr85M_CLTmP5EpPATSTJlEQ8c Message-ID: To: Matan Azrad Cc: Declan Doherty , Radu Nicolau , dev@dpdk.org, Chas Williams Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH] net/bonding: propagate promiscous mode in mode 4 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: , X-List-Received-Date: Sun, 16 Sep 2018 16:14:24 -0000 On Thu, Sep 13, 2018 at 11:40 AM Matan Azrad wrote: > > Hi Chas > > From: Chas Williams > > On Wed, Sep 12, 2018 at 1:56 AM Matan Azrad > > wrote: > > > > > > Hi Chas > > > > > > From: Chas Williams > > > > On Mon, Aug 6, 2018 at 3:35 PM Matan Azrad > > > > wrote: > > > > > > > > > > > > > > > Hi Chas > > > > > > > > > > From: Chas Williams > > > > > >On Mon, Aug 6, 2018 at 1:46 PM Matan Azrad > > > > wrote: > > > > > >Hi Chas > > > > > > > > > > > >From: Chas Williams > > > > > >>On Fri, Aug 3, 2018 at 1:47 AM Matan Azrad > > > > wrote: > > > > > >>Hi Chas > > > > > >> > > > > > >> From: Chas Williams > > > > > >> [mailto:mailto:mailto:mailto:3chas3@gmail.com] > > > > > >> On Thu, Aug 2, 2018 at 1:33 > > > > > >>> PM Matan Azrad wrote: > > > > > >>> > > > > > > >>> > > I suggest to do it like next, To add one more parameter > > > > > >>> > > for LACP which means how to configure the > > > > > >>> > LACP MC group - lacp_mc_grp_conf: > > > > > >>> > > 1. rte_flow. > > > > > >>> > > 2. flow director. > > > > > >>> > > 3. add_mac. > > > > > >>> > > 3. set_mc_add_list > > > > > >>> > > 4. allmulti > > > > > >>> > > 5. promiscuous > > > > > >>> > > Maybe more... or less :) > > > > > >>> > > > > > > > >>> > > By this way the user decides how to do it, if it's fail > > > > > >>> > > for a slave, the salve > > > > > >>> > should be rejected. > > > > > >>> > > Conflict with another configuration(for example calling t= o > > > > > >>> > > promiscuous > > > > > >>> > disable while running LACP lacp_mc_grp_conf=3D5) should rai= se > > > > > >>> > an > > > > error. > > > > > >>> > > > > > > > >>> > > What do you think? > > > > > >>> > > > > > > > >>> > > > > > > >>> > Supporting an LACP mc group specific configuration does mak= e > > > > > >>> > sense, but I wonder if this could just be handled by defaul= t > > > > > >>> > during > > > > slave add. > > > > > >>> > > > > > > >>> > > > > > > >>> > 1 and 2 are essentially the same hardware filtering offload > > > > > >>> > mode, and the other modes are irrelevant if this is enabled= , > > > > > >>> > it should not be possible to add the slave if the bond is > > > > > >>> > configured for this mode, or possible to change the bond > > > > > >>> > into this mode if an existing slave doesn't support it. > > > > > >>> > > > > > >>> > > > > > > >>> > 3 should be the default expected behavior, but > > > > > >>> > rte_eth_bond_slave_add() should fail if the slave being > > > > > >>> > added doesn't support either adding the MAC to the slave or > > > > > >>> > adding the > > > > LACP MC address. > > > > > >>> > > > > > > >>> > Then the user could try either rte_eth_allmulticast_enable(= ) > > > > > >>> > on the bond port and then try to add the slave again, which > > > > > >>> > should fail if existing slave didn't support allmulticast o= r > > > > > >>> > the add slave would fail again if the slave didn't support > > > > > >>> > allmulticast and finally just call > > > > > >>> > rte_eth_promiscuous_enable() on the bond and then try to > > > > > >>> > re-add the that slave. > > > > > >>> > > > > > > >>> > but maybe having a explicit configuration parameter would b= e > > > > better. > > > > > >>> > > > > > >>> I don't sure you understand exactly what I=E2=80=99m suggesti= ng here, > > again: > > > > > >>> I suggest to add a new parameter to the LACP mode called > > > > > >>> lacp_mc_grp_conf(or something else). > > > > > >>> So, when the user configures LACP (mode 4) it must to > > > > > >>> configure the lacp_mc_grp_conf parameter to one of the option= s I > > suggested. > > > > > >>> This parameter is not per slave means the bond PMD will use > > > > > >>> the selected option to configure the LACP MC group for all th= e > > slave ports. > > > > > >>> > > > > > >>> If one of the slaves doesn't support the selected option it > > > > > >>> should be > > > > rejected. > > > > > >>> Conflicts should rais an error. > > > > > >>> > > > > > >>> I agree here. Yes, if a slave can't manage to subscribe to > > > > > >>> the multicast group, an error should be raised. The only way > > > > > >>> for this to happen is that you don't have promisc support > > > > > >>> which is the ultimate > > > > fallback. > > > > > >> > > > > > >>> The advantages are: > > > > > >>> The user knows which option is better to synchronize with his > > > > application. > > > > > >>> The user knows better than the bond PMD what is the slaves > > > > capabilities. > > > > > >>> All the slaves are configured by the same way - consistent tr= affic. > > > > > >>> > > > > > >>> > > > > > >>> It would be ideal if all the slaves would have the same > > > > > >>> features and capabilities. There wasn't enforced before, so > > > > > >>> this would be a new restriction that would be less flexible > > > > > >>> than what we currently have. That doesn't seem like an > > improvement. > > > > > >> > > > > > >>> The bonding user probably doesn't care which mode is used. > > > > > >>> The bonding user just wants bonding to work. He doesn't care > > > > > >>> about > > > > the details. If I am writing > > > > > >>> an application with this proposed API, I need to make a list > > > > > >>> of adapters and what they support (and keep this up to date a= s > > > > > >>> DPDK > > > > evolves). Ugh. > > > > > >> > > > > > >>The applications commonly know what are the nics capabilities > > > > > >>they > > > > work with. > > > > > >> > > > > > >>I know at least an one big application which really suffering > > > > > >>because the bond configures promiscuous in mode 4 without the > > > > application asking (it's considered there as a bug in dpdk). > > > > > >>I think that providing another option will be better. > > > > > >> > > > > > >>I think providing another option will be better as well. > > > > > >>However we > > > > disagree on the option. > > > > > >>If the PMD has no other way to subscribe the multicast group, i= t > > > > > >>has to > > > > use promiscuous mode. > > > > > > > > > > > >>Yes, it is true but there are a lot of other and better options= , > > > > promiscuous is greedy! Should be the last alternative to use. > > > > > > > > > > > >Unfortunately, it's the only option implemented. > > > > > > > > > > Yes, I know, I suggest to change it or at least not to make it wo= rst. > > > > > > > > > > >>Providing a list of options only makes life complicated for the > > > > > >>developer and doesn't really make any difference in the end res= ults. > > > > > > > > > > > >>A big different, for example: > > > > > >>Let's say the bonding groups 2 devices that support rte_flow. > > > > > >>The user don't want neither promiscuous nor all multicast, he > > > > > >>just want to get it's mac traffic + LACP MC group traffic,(a > > > > > >>realistic use case) if > > > > he has an option to tell to the bond PMD, please use rte_flow to > > > > configure the specific LACP MC group it will be great. > > > > > >>Think how much work these applications should do in the current > > > > behavior. > > > > > > > > > > > >The bond PMD should already know how to do that itself. > > > > > > > > > > The bond can do it with a lot of complexity, but again the user > > > > > must know > > > > what the bond chose to be synchronized. > > > > > So, I think it's better that the user will define it because it i= s > > > > > a traffic configuration (the same as promiscuous configuration - > > > > > the user configures it) > > > > > > Again, you are forcing more work on the user to ask them to > > > > > > select > > > > between the methods. > > > > > > > > > > We can create a default option as now(promiscuous). > > > > > > > > > > >> For instance, if the least common denominator between the two > > > > > >>PMDs is promiscuous mode, you are going to be forced to run > > > > > >>both in promiscuous mode instead of selecting the best mode for > > each PMD. > > > > > > > > > > > >>In this case promiscuous is better, Using a different > > > > > >>configuration is worst and against the bonding PMD > > > > principle to get a consistent traffic from the slaves. > > > > > >>So, if one uses allmulti and one uses promiscuous the > > > > > >>application may get an inconsistent traffic and it may trigger = a > > > > > >>lot of problems and > > > > complications for some applications. > > > > > > > > > > > >Those applications should already have those problems. > > > > > > I can make the counter > > > > > >argument that there are potentially applications relying on the > > > > > >broken > > > > behavior. > > > > > > > > > > You right. So adding allmulticast will require changes in these > > applications. > > > > > > > > > > >We need to ignore those issues and fix this the "right" way. Th= e > > > > > >"right" way IMHO is the pass the least amount of traffic possibl= e > > > > > >in each > > > > case. > > > > > > > > > > Not in cost of an inconsistency, but looks like we are not agree = here. > > > > > > > > > > > > > I have recently run into this issue again with a device that doesn'= t > > > > support promiscuous, but does let me subscribe to the appropriate > > multicast groups. > > > > At this point, I am leaning toward adding another API call to the > > > > bonding API so that the user can provide a callback to setup > > > > whatever they want on the slaves. > > > > The default setup routine would be enable promiscuous. > > > > > > > > Comments? > > > > > > The bonding already allows to the users to do operations directly to = the > > slaves(it exports the port ids - rte_eth_bond_slaves_get), so I don't > > understand why do you need a new API. > > > The only change you need may be to add parameter to disable the > > promiscuous configuration in mode4. > > > > Changing the API is new API. We should attempt to not break any of the > > existing API. > > > > As for being able to operate on the slaves, yet, you can. But the bond= ing > > PMD also controls the slaves as well. It seems cleaner to make this ex= plcit, > > that the bonding driver calls out to the application to setup the 802.3= ad > > listening when it needs to be done. > > If you want to control it a different way, you simply provide a null ro= utine > > that does nothing and control it however you like. > > The issue is that the bonding PMD cannot be synchronized with such like c= allback, it doesn't know what was done by the application. Exactly. That's why you need to be to change the native behavior of the bonding PMD. > I don't think we should open a direct application calls to the slaves by = an API, if the application really need it, as we said, it has already opti= on to do it without a new API while it is really not recommended by the bon= ding guide. This would be the opposite direction. The slaves would be calling out to the application to ask the application to do something. With the existing bonding PMD it will always attempt to enable promisc and that isn't desirable in all situations. Previously, activate_slave() always happened as part of slave add. That shouldn't be the case now if the bonding PMD isn't running. Adding and removing slaves while the bonding PMD is started shouldn't probably not be done because I am not sure we can ensure that there is race free behavior with the 802.3ad rx/tx routines.