From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f53.google.com (mail-wm0-f53.google.com [74.125.82.53]) by dpdk.org (Postfix) with ESMTP id 98AC21B1A6 for ; Tue, 9 Jan 2018 11:36:06 +0100 (CET) Received: by mail-wm0-f53.google.com with SMTP id f206so19590788wmf.5 for ; Tue, 09 Jan 2018 02:36:06 -0800 (PST) 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=tqY/3xWrkfSHz0A6lqoAcDnS/Ps6h3Evfts6EXTirVE=; b=fWKHGW5fu1QjCeCTQgNaLLMX/nPdX5HWt8MgQzEKEFljOEGE9ArzBtsdS69EylVE0P jsOC6IR+ZRLdn2j/qIgv5Dlw+wHc6fmfaG2Pi6g4uv+EyQqcYixsYzFNS0iaLbNV/6We CjqGWCemqBzB2Wi5MbHZdSnjZGpjI8Drruzk/k/+BEmz/mSHON4Eupo1OeS9lRdOJahe RgQZnw3OJbdTn85/Guvi370Nzmf7MP4wHQngxbQboLY89Qc+4bgg5G//KN2rHQq/eufS iTc9ecxDH1op2ux3rxZ18tHkwUFzmPxSRlHg7ZjBNyqYgv0e1l2WZc97BMWwnnr5C2bG v15g== 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=tqY/3xWrkfSHz0A6lqoAcDnS/Ps6h3Evfts6EXTirVE=; b=txi8z+ZPT1gShCyxbsDAK5napMGznrsAXboa8IY4LJqHTD4nbD0vicnFcDDzGAzYf9 DxHEpzns6REi/vxequ4HmPaG3ExOqG4XMEMF0kT5Kz7t50E5i9IWxXxccdfLlJeb1kED fsyEUGOpD3Rt1+QNw0yn9BeBiwYZb6WU0d4sahGOGmMBrlzMl64cGMUH5fapwhZWxRGV WBOAOrzKgeNtzng1WF2edFRJGxNdn9He/1qxvIQybbRpcg9TKxw1+oxR4lkUGllRvAG/ 3MmanqmoyFLlv3g+RZsqm6Y4iwqSbPlfkt18CLsMNFzVD/uzxJ8IOQZvvp4NPOsSirqZ p49w== X-Gm-Message-State: AKGB3mIiAHyQSmAq2z7qlHNskAlfhK0UK/7Bjv4Lpv9YXB9o5nW0VIRe p4IsMEwhxCZ9WzFMhD7+xyil X-Google-Smtp-Source: ACJfBotjtJB0PQaw3aww0n6g+/NLZ+5nzSIU7DNRgUrXU9/XyPXLc7uuhKlxPuZz8VHx5ffWErSGaw== X-Received: by 10.28.101.132 with SMTP id z126mr10935245wmb.106.1515494166236; Tue, 09 Jan 2018 02:36:06 -0800 (PST) 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 201sm15921602wmm.38.2018.01.09.02.36.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 09 Jan 2018 02:36:05 -0800 (PST) Date: Tue, 9 Jan 2018 11:35:32 +0100 From: Nelio Laranjeiro To: Shahaf Shuler Cc: Adrien Mazarguil , Yongseok Koh , "dev@dpdk.org" Message-ID: <20180109103532.vgqd2u63zs3k2g7c@laranjeiro-vm.dev.6wind.com> References: <20171123120252.143695-1-shahafs@mellanox.com> <20180103172924.GE4256@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 6/7] net/mlx4: convert to new Tx offloads API 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: Tue, 09 Jan 2018 10:36:06 -0000 Hi Shahaf, On Thu, Jan 04, 2018 at 11:55:17AM +0000, Shahaf Shuler wrote: > Hi Adrien and Nelio, > > See below comment regarding your output on the offload check. > Rest of the comments were accepted. > > Wednesday, January 3, 2018 7:29 PM, Adrien Mazarguil : > > [...] > > > > > > +{ > > > + uint64_t port_offloads = priv->dev->data- > > >dev_conf.txmode.offloads; > > > + uint64_t port_supp_offloads = > > mlx4_priv_get_tx_port_offloads(priv); > > > > Instead of a redundant "port_", how about clarifying it all as follows: > > > > offloads -> requested > > port_offloads -> mandatory > > port_supp_offloads -> supported > > > > > + > > > + if ((port_offloads ^ offloads) & port_supp_offloads) > > > + return 0; > > > + return 1; > > > > And simplify this as: > > > > return !((mandatory ^ requested) & supported); > > > > Maybe I missed something, but there seems to be an inconsistency, e.g. > > You are correct that the purpose of this function is to check if the offload configuration is correct. > However the current code being done on mlx4 does not validate if the queue offloads configured are supported. > It only validates if the port offloads configuration matches the queue offload configuration. > > The reason it lack the supported offloads check was discussed in internal mail (you both CC I believe). Generally it was due to the fact that CRC and VLAN strip offloads are not supported by the PMD, however set for almost every example/application in DPDK. > For the complete check look on mlx5 patches on this series. > > > requesting an unsupported offload does not necessarily fail: > > > > mandatory = 0x00 > > requested = 0x40 > > supported = 0x10 > > > > => valid but shouldn't be > > It should if the offload is per-queue offload. > > > > > > And requesting a supported offload when there are no mandatory ones > > should not be a problem: > > > > mandatory = 0x00 > > requested = 0x10 > > supported = 0x10 > > > > => invalid but it should be > > It is invalid indeed. If the application declare some port offload not to be set on dev_configure, it cannot enable it from the queue setup. > Port offloads can be set only on device configuration, and when set every queue should have them set as well. > > > > > A naive translation of the above requirements results in the following > > expression: > > > > return (requested | supported) == supported && > > (requested & mandatory) == mandatory; > > > > What's your opinion? > > >>From an application point of view, it seems strange to provide an already configured offload when configuring the queues, i.e. rte_eth_dev_configure() is called before rte_eth_{tx,rx}_queue_setup(). I think this "mandatory" information should be removed from the API documentation letting the application the capability to request a null offload when configuring the queue. As this modification does not break the API/ABI it only needs eventually a modification in the driver, it can be done in the future. For mlx5 part: Acked-by: Nelio Laranjeiro -- Nélio Laranjeiro 6WIND