From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f176.google.com (mail-wr0-f176.google.com [209.85.128.176]) by dpdk.org (Postfix) with ESMTP id B1B5E2BC7 for ; Mon, 23 Apr 2018 11:33:36 +0200 (CEST) Received: by mail-wr0-f176.google.com with SMTP id f14-v6so39281377wre.4 for ; Mon, 23 Apr 2018 02:33:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=msKYDJZmExSDOC5O1DyckhiBmHE1fyIvag86XazgZr4=; b=O5PSSABWgmkKAAWPtxG+g4orkk8FDCIiW9sadDHG+Jd4FoDwwq3J2fwoV8ibawzHGz pM8Rph44wRRRVYurChu64LPLYxzH74bKRALEv738ApiNQJ7zxMDJkQsVncnKlaCcKA64 ZUV4NypnVjEEZOzKsQqnQ4uzJU6N1tVbCf6+NQkTcX/wl158MSmbTkCuMyD+kevqanHb OqEhQG3S4o6LhBW5cS1O3YjuqV7hlOSICuDw8bqLhPQ6s6nIGhyeBy5yM7Ure83EP6xI lV8qFoXwoeZPgBzXkmVU9WADJvcpx/kreWR/5qvOzb42REQAXnx7TXI+m2akjua/ftXm /kLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=msKYDJZmExSDOC5O1DyckhiBmHE1fyIvag86XazgZr4=; b=FVBE7YpwaFdzrWI2tEPFGW9yszV2cLl94oMOk2C5lAJ9nVSLpF3BD8sM7ihRqT4i8N Vff4IWRrIHSMx2lunpvSaqb6ztRQyRs+KTlfB0gAwvCy0wI0SQAHjYcMc+b1979nT1VY Hd/fjrSPwIuCX80pPeLi8cpcWBAX8un41QIpa368UaT9V2k8fcX0wzVnILOgJZMdnhyw s+nkGp4SeXKzRDhtYqZ6ZRU6Gh3BX/uS9GGrtHfpQIw0woOVmmfow1EMqKGUWAzD74LF 8Smvx7SCxHZjmMybj+jS8Op4zYbm10GclaHXauj77dev29js7nu/h4CDw2Si6n+h4kFu e9tg== X-Gm-Message-State: ALQs6tDWrvNE9IOfo6bHw06YGZqSmSrtv5aoqr1Iko7ywf7DTJTWiias wRtKF9xQE+ywzMFkePAPT7Ie X-Google-Smtp-Source: AIpwx4/hzscqx0XZhvtyESEArCkK81kwb8nLAWjPNtLfsczl/OBARkEjRwHcietXR3ObkhzfZwOaMA== X-Received: by 2002:adf:acae:: with SMTP id o43-v6mr8605944wrc.132.1524476015989; Mon, 23 Apr 2018 02:33:35 -0700 (PDT) Received: from laranjeiro-vm.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id n73-v6sm13196703wrb.18.2018.04.23.02.33.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 23 Apr 2018 02:33:35 -0700 (PDT) Date: Mon, 23 Apr 2018 11:34:20 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Shahaf Shuler Cc: "dev@dpdk.org" , Yongseok Koh , Adrien Mazarguil Message-ID: <20180423093420.wgng5yll3s3onwkq@laranjeiro-vm.dev.6wind.com> References: <870d4840cdb872d363d103808f82eb3645fd36b0.1524059312.git.nelio.laranjeiro@6wind.com> <20180423073307.j7m7bdl4yguoikff@laranjeiro-vm.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop 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: Mon, 23 Apr 2018 09:33:36 -0000 On Mon, Apr 23, 2018 at 07:57:36AM +0000, Shahaf Shuler wrote: > Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro: > [...] > > > > +/** > > > > + * DPDK callback to set multicast addresses list. > > > > + * > > > > + * @param dev > > > > + * Pointer to Ethernet device structure. > > > > + * @param mac_addr_set > > > > + * Multicast MAC address pointer array. > > > > + * @param nb_mac_addr > > > > + * Number of entries in the array. > > > > + * > > > > + * @return > > > > + * 0 on success, a negative errno value otherwise and rte_errno is set. > > > > + */ > > > > +int > > > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev, > > > > + struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) { > > > > + uint32_t i; > > > > + int ret; > > > > + > > > > > > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES > > before we start > > > operate. > > > > This verification is done in the sub function. > > I see only assert. Did I missed anything? No. > > Considering an application calling such API wants to remove/replace the old > > list with new entries. > > That this new one can be just an addition or totally different list or even > > empty. > > This new list can be larger than the amount of MAC addresses the PMD can > > support. > > > > There are two possibilities: > > > > 1. The list is too large: the application will enable the all multicast mode to > > receive the extra mac addresses it needs. > > How can application know the size of the MC list? > only the UC size is being reported: > info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES Such information is not reported at all. The application has to guess. > > 2. The list fits (or empty): no issues. > > > > At the end the application can also call this API with an empty list to clear it > > before/after enabling the "all multicast" mode. > > The final result being the same, does it worse to add a duplicated > > verification? > > At the current code if the list is too large and the PMD was compiled > w/o debug mode it will results in seg fault. Right it needs a verification. > > Note: if an error happens the new list is not committed yet i.e. the traffic > > remains untouched. > > > > > > + for (i = MLX5_MAX_UC_MAC_ADDRESSES; i != > > > > MLX5_MAX_MAC_ADDRESSES; ++i) > > > > + mlx5_internal_mac_addr_remove(dev, i); > > > > + i = MLX5_MAX_UC_MAC_ADDRESSES; > > > > + while (nb_mc_addr--) { > > > > > > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip > > > + warn if it is not. > > > > Such verification should be done in the public API i.e. ethdev. > > I don't understand. > In the first patch of the series you add extra verification to check > the mac address validity. It only verify the MAC address is not zero, it does not verify the MAC address is valid in the function context (e.g. unicast in mlx5_mac_addr_add()). > But for the MC you claim it should be done on ethdev layer. Dito. > It should be consistant. Either ethdev verify the MAC address or the > PMD. If the first one, then there is no need to add the > is_zero_ether_addr check on the first patch. It is consistent, the PMD only verify the MAC address is not zero and this in both API. > > > > + ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++, > > > > i++); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + if (!dev->data->promiscuous) > > > > + return mlx5_traffic_restart(dev); > > > > + return 0; > > > > +} > > > > -- > > > > 2.17.0 Regards, -- Nélio Laranjeiro 6WIND