From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3chas3@gmail.com> Received: from mail-io0-f196.google.com (mail-io0-f196.google.com [209.85.223.196]) by dpdk.org (Postfix) with ESMTP id 8BC8B1B50B for ; Thu, 2 Aug 2018 23:10:50 +0200 (CEST) Received: by mail-io0-f196.google.com with SMTP id q9-v6so3257235ioj.8 for ; Thu, 02 Aug 2018 14:10:50 -0700 (PDT) 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; bh=0elktd7Vdq5cazh3mgkecsDFDvo2pmnKoIQ9eqls1PQ=; b=hhlR+3jGVm1AF3F4amofOTXNMKdri15p8davB2PYXcvOJZ0S0nOnwBwPAprkK/pB/F uVpq7HLb7QqCJFg/JipLQtFRb9kEcB+Z1tMHstbxChSf5QaCNaLWmxE1Ht0WPyVxeaLt XvDGziesJ6GzU2QYnVYSBlryIcNPyLT07pGLIJcSbASlOcbykhfDcY6PNZ+DQ0qo+TFx erIwscxsUbFJU8RwK9itrh3gmhJwSmqrKTrphAEq92EUPFIGQFbZ4Mqk4wsPY5fxBfnU xGZiGNM6Ryr/WaKwN0dBsW0tiP564ZAF9BBKnumzveHQ3TpcqH5RdCvzV2Y4UvP7sIeQ Hftw== 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=0elktd7Vdq5cazh3mgkecsDFDvo2pmnKoIQ9eqls1PQ=; b=bFLOGBhshWfyvUQ22BewOGWvUxpdcC1YQ4AvrVvKqZBrOxogrNNQCXBEnyMumMs+QM vJGxwca2THK3Bgtn+EVaiROarhYZx13n4Pki1F5XipFiyopzLZ/t99O4S9k7KYxD72U6 Qgw27d9hcciOMNzFq8Shy9vNU7GXYW1+kNPe3SXNlJiJbtHHYUGYelk5yR5unH5g335D HR0mY1chweeD2488OMaHLXuf/GFa6KbfZZDcD/pxQp6GOGqX6MDI2zmVhPSv1mxrTZfz XtTzqz33cxjpR9APe8eBbk4jpBV33eSOC0fvTIyyOL0AM22CjfdVsTU1RarLm6Qc0KG7 tZ+g== X-Gm-Message-State: AOUpUlG6aKHZ+LLTjRHcQKjxAvj6r0nvjn9I8dHu8EJR7KHSq8TwWLQ7 S+bZ7ZiBI0K4eAa2QCVBkaP1X0HBdX4uUMYV2Ic= X-Google-Smtp-Source: AAOMgpeXKErG+kctbotVbdQ741nIPfb/TmCHdqEBauffgXYJFj/iIg0mDYpCKiUhO/eYVbkk55ixBgRfeWd0f2D1wxY= X-Received: by 2002:a5e:da45:: with SMTP id o5-v6mr3872387iop.215.1533244249936; Thu, 02 Aug 2018 14:10:49 -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: From: Chas Williams <3chas3@gmail.com> Date: Thu, 2 Aug 2018 17:10:38 -0400 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 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: Thu, 02 Aug 2018 21:10:51 -0000 On Thu, Aug 2, 2018 at 1:33 PM Matan Azrad wrote: > Hi Declan > > From: Doherty, Declan > > On 02/08/2018 3:24 PM, Matan Azrad wrote: > > > Hi > > > > > > From: Doherty, Declan > > >> On 02/08/2018 7:35 AM, Matan Azrad wrote: > > >>> Hi Chas, Radu > > >>> > > >>> From: Chas Williams > > >>>> On Wed, Aug 1, 2018 at 9:48 AM Radu Nicolau > > > >>>> wrote: > > >>>> > > >>>>> > > >>>>> > > >>>>> On 8/1/2018 2:34 PM, Chas Williams wrote: > > >>>>> > > >>>>> > > >>>>> > > >>>>> On Wed, Aug 1, 2018 at 9:04 AM Radu Nicolau > > > > >>>>> wrote: > > >>>>> > > >>>>>> Update the bonding promiscuous mode enable/disable functions as = to > > >>>>>> propagate the change to all slaves instead of doing nothing; thi= s > > >>>>>> seems to be the correct behaviour according to the standard, and > > >>>>>> also implemented in the linux network stack. > > >>>>>> > > >>>>>> Signed-off-by: Radu Nicolau > > >>>>>> --- > > >>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 8 ++------ > > >>>>>> 1 file changed, 2 insertions(+), 6 deletions(-) > > >>>>>> > > >>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c > > >>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c > > >>>>>> index ad6e33f..16105cb 100644 > > >>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c > > >>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c > > >>>>>> @@ -2617,12 +2617,10 @@ bond_ethdev_promiscuous_enable(struct > > >>>>>> rte_eth_dev > > >>>>>> *eth_dev) > > >>>>>> case BONDING_MODE_ROUND_ROBIN: > > >>>>>> case BONDING_MODE_BALANCE: > > >>>>>> case BONDING_MODE_BROADCAST: > > >>>>>> + case BONDING_MODE_8023AD: > > >>>>>> for (i =3D 0; i < internals->slave_count; i++) > > >>>>>> > > >>>>>> rte_eth_promiscuous_enable(internals->slaves[i].port_id); > > >>>>>> break; > > >>>>>> - /* In mode4 promiscus mode is managed when slave is > > >>>> added/removed > > >>>>>> */ > > >>>>>> > > >>>>> > > >>>>> This comment is true (and it appears it is always on in 802.3ad > mode): > > >>>>> > > >>>>> /* use this port as agregator */ > > >>>>> port->aggregator_port_id =3D slave_id; > > >>>>> rte_eth_promiscuous_enable(slave_id); > > >>>>> > > >>>>> If we are going to do this here, we should probably get rid of it > in > > >>>>> the other location so that future readers aren't confused about > > >>>>> which is the one doing the work. > > >>>>> > > >>>>> Since some adapters don't have group multicast support, we might > > >>>>> already be in promiscuous anyway. Turning off promiscuous for th= e > > >>>>> bonding master might turn it off in the slaves where an applicati= on > > >>>>> has already enabled it. > > >>>>> > > >>>>> > > >>>>> The idea was to preserve the current behavior except for the > > >>>>> explicit promiscuous disable/enable APIs; an application may > disable > > >>>>> the promiscuous mode on the bonding port and then enable it back, > > >>>>> expecting it to propagate to the slaves. > > >>>>> > > >>>> > > >>>> Yes, but an application doing that will break 802.3ad because > > >>>> promiscuous mode is used to receive the LAG PDUs which are on a > > multicast > > >> group. > > >>>> That's why this code doesn't let you disable promiscuous when you > are > > >>>> in 802.3ad mode. > > >>>> > > >>>> If you want to do this it needs to be more complicated. In 802.3a= d, > > >>>> you should try to add the multicast group to the slave interface. > If > > >>>> that fails, turn on promisc mode for the slave. Make note of it. > > >>>> Later if bonding wants to enabled/disable promisc mode for the > > >>>> slaves, it needs to check if that slaves needs to remain in promis= c > to > > >> continue to get the LAG PDUs. > > >>> > > >>> I agree with Chas that this commit will hurt current LACP logic, bu= t > maybe > > >> this is the time to open discussion about it: > > >>> The current bonding implementation is greedy while it setting > > >>> promiscuous automatically for LACP, The user asks LACP and he gets > > >> promiscuous by the way. > > >>> > > >>> So if the user don't want promiscuous he must to disable it directl= y > via > > slaves > > >> ports and to allow LACP using rte_flow\flow > > >> director\set_mc_addr_list\allmulti... > > >>> > > >>> I think the best way is to let the user to enable LACP as he wants= , > directly > > via > > >> slaves or by the bond promiscuous_enable API. > > >>> For sure, it must be documented well. > > >>> > > >>> Matan. > > >>> > > >> > > >> I'm thinking that default behavior should be that promiscuous mode > should > > be > > >> disabled by default, and that the bond port should fail to start if > any of the > > slave > > >> ports can't support subscription to the LACP multicast group. At thi= s > point > > the > > >> user can decided to enable promiscuous mode on the bond port (and > > therefore > > >> on all the slaves) and then start the bond. If we have slaves with > different > > >> configurations for multicast subscriptions or promiscuous mode > enablement, > > >> then there is potentially the opportunity for inconsistency in traff= ic > > depending > > >> on which slaves are active. > > > > > >> Personally I would prefer that all configuration if possible is > propagated > > >> through the bond port. So if a user wants to use a port which doesn'= t > support > > >> multicast subscription then all ports in the bond need to be in > promiscuous > > >> mode, and the user needs to explicitly enable it through the bond > port, that > > way > > >> at least we can guarantee consist traffic irrespective of which port= s > in the > > bond > > >> are active at any one time. > > > > > > That's exactly what I said :) > > > > > > > :) > > > > I guess so, but it was the configuration directly via the slave port bi= t > > which had me concerned, I think this needs to be managed directly from > > the bond port, ideally they > > Yes, but you know that bond is far from supporting all the ethdev > configuration > and there is not a clear limitation in bond documentation to do > configuration via slaves. > > I agree that it's better to use some bond API to configure the LACP mc > group. > > > > > > > > > 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 to promiscuou= s > > disable while running LACP lacp_mc_grp_conf=3D5) should raise an error. > > > > > > What do you think? > > > > > > > Supporting an LACP mc group specific configuration does make sense, but > > I wonder if this could just be handled by default 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 bon= d > > port and then try to add the slave again, which should fail if existing > > slave didn't support allmulticast or 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 tha= t > > slave. > > > > but maybe having a explicit configuration parameter would be better. > > I don't sure you understand exactly what I=E2=80=99m suggesting here, aga= in: > 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 options 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 the 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 traffic. > > > 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 as DPDK evolves). Ugh.