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 E95E32BE6 for ; Fri, 28 Oct 2016 15:42:44 +0200 (CEST) Received: by mail-wm0-f53.google.com with SMTP id p190so11505970wmp.1 for ; Fri, 28 Oct 2016 06:42:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:user-agent:in-reply-to :references:mime-version:content-transfer-encoding; bh=mZv11qYtGneMFPW1uOQqd95qKS1OpI3tNJ8NtGfVAd4=; b=f5xjMX2x8YRmI0Lul9BXeIvG963Eu5xE2/MscrNHKZpgTjKkGh1ulhbWf+t6qe6kwb SRg9KDtI9w0UhY4ctxBU5D3I0OaaSjYaZRoBw83CHhPcIkLDJH5BFlHbHvo4s09JcwYI LdCy25DkktsTpmMImtNXrH/WAzTGCjJnah2cm2HB8JHuqLyLsB274lblkctgPraWAvdK wPcEEv9ezuWKPprw/77nHSs4gs3qIvHlhPB4Z470ileuXbaDQ+P8hhheb7QDKhrHN9Dj YDSnPyoc2Lj99Yrau8tBZA0qSdVnfV5Fci6L7KZdfz2yN96aTw6XJwcS20xo6f1JYO/C PjoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:user-agent :in-reply-to:references:mime-version:content-transfer-encoding; bh=mZv11qYtGneMFPW1uOQqd95qKS1OpI3tNJ8NtGfVAd4=; b=k13m5Mo8C200QwXLQSuGYcRG6KOxRDA8z8FdCdztJOFJk8pwOsD+xVZfqsrQ6AgHZh zSBODbKXJVBhLYIIC0Wgc9UCNaHl/x+wVA8yGEZf+5h4vbk0voqkI9IkhVdaecKpddlu AYiGbTBZ+Fb86QyoVRCBoO21EOiBYkelrsIrUlyL5N0TUyKgs8D3KesXfhddkYtHOoa3 7Zn4uxrtQPtSpcN37kmT6L7Rez6+sKGVUnHcJFuh/JWYtCz5eGdGKQvee25ZL3kgxezM 4fRLJ2MJWLMvlgx3HspHQSa5ZwnFV28yLGPcwc0Z70273XzH1zyar6UO2p90tYS5Cr1D kWNg== X-Gm-Message-State: ABUngve0ZAsXkV0DDJQu2NQSGhEyjGzP3G7IPzKBrIkHFlPAXf8GM6QhL1Dftd4wpRYizAjX X-Received: by 10.28.191.206 with SMTP id o75mr3068610wmi.129.1477662164624; Fri, 28 Oct 2016 06:42:44 -0700 (PDT) Received: from xps13.localnet (184.203.134.77.rev.sfr.net. [77.134.203.184]) by smtp.gmail.com with ESMTPSA id 194sm9667439wmj.0.2016.10.28.06.42.43 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 28 Oct 2016 06:42:43 -0700 (PDT) From: Thomas Monjalon To: "Ananyev, Konstantin" Date: Fri, 28 Oct 2016 15:42:43 +0200 Message-ID: <2177010.quNaxrZShx@xps13> User-Agent: KMail/4.14.10 (Linux/4.5.4-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CED77@irsmsx105.ger.corp.intel.com> References: <1477327917-18564-1-git-send-email-tomaszx.kulasek@intel.com> <4589776.b4NVqTvzOG@xps13> <2601191342CEEE43887BDE71AB9772583F0CED77@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Oct 2016 13:42:45 -0000 2016-10-28 12:59, Ananyev, Konstantin: > > 2016-10-28 11:34, Ananyev, Konstantin: > > > > > 2016-10-27 16:24, Ananyev, Konstantin: > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > > > > > > 2016-10-27 15:52, Ananyev, Konstantin: > > > > > > > > > 2016-10-26 14:56, Tomasz Kulasek: > > > > > > > > > > --- a/config/common_base > > > > > > > > > > +++ b/config/common_base > > > > > > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y > > > > > > > > > > > > > > > > > > We cannot enable it until it is implemented in every drivers. > > > > > > > > > > > > > > > > Not sure why? > > > > > > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as noop. > > > > > > > > Right now it is not mandatory for the PMD to implement it. > > > > > > > > > > > > > > If it is not implemented, the application must do the preparation by itself. > > > > > > > From patch 6: > > > > > > > " > > > > > > > Removed pseudo header calculation for udp/tcp/tso packets from > > > > > > > application and used Tx preparation API for packet preparation and > > > > > > > verification. > > > > > > > " > > > > > > > So how does it behave with other drivers? > > > > > > > > > > > > Hmm so it seems that we broke testpmd csumonly mode for non-intel drivers.. > > > > > > My bad, missed that part completely. > > > > > > Yes, then I suppose for now we'll need to support both (with and without) code paths for testpmd. > > > > > > Probably a new fwd mode or just extra parameter for the existing one? > > > > > > Any other suggestions? > > > > > > > > > > Please think how we can use it in every applications. > > > > > It is not ready. > > > > > Either we introduce the API without enabling it, or we implement it > > > > > in every drivers. > > > > > > > > I understand your position here, but just like to point that: > > > > 1) It is a new functionality optional to use. > > > > The app is free not to use that functionality and still do the preparation itself > > > > (as it has to do it now). > > > > All existing apps would keep working as expected without using that function. > > > > Though if the app developer knows that for all HW models he plans to run on > > > > tx_prep is implemented - he is free to use it. > > > > 2) It would be difficult for Tomasz (and other Intel guys) to implement tx_prep() > > > > for all non-Intel HW that DPDK supports right now. > > > > We just don't have all the actual HW in stock and probably adequate knowledge of it. > > > > So we depend here on the good will of other PMD mainaners/developers to implement > > > > tx_prep() for these devices. > > > > From other side, if it will be disabled by default, then, I think, > > > > PMD developers just wouldn't be motivated to implement it. > > > > So it will be left untested and unused forever. > > > > > > Actually as another thought: > > > Can we have it enabled by default, but mark it as experimental or so? > > > If memory serves me right, we've done that for cryptodev in the past, no? > > > > Cryptodev was a whole new library. > > We won't play the game "find which function is experimental or not". > > > > We should not enable a function until it is fully implemented. > > > > If the user really understands that it will work only with few drivers > > then he can change the build configuration himself. > > Enabling in the default configuration is a message to say that it works > > everywhere without any risk. > > It's so simple that I don't even understand why I must argue for. > > > > And by the way, it is late for 16.11. > > Ok, I understand your concern about enabling it by default and testpmd breakage, > but what else you believe is not ready? That's already a lot! I commented also about function naming. All these things are trivial to fix. But it is late. After RC1, we should stop integrating new features. > > I suggest to integrate it in the beginning of 17.02 cycle, with the hope > > that you can convince other developers to implement it in other drivers, > > so we could finally enable it in the default config. > > Ok, any insights then, how we can convince people to do that? You just have to explain clearly what this new feature is bringing and what will be the future possibilities. > BTW, it means then that tx_prep() should become part of mandatory API > to be implemented by each PMD doing TX offloads, right? Right. The question is "what means mandatory"? Should we block some patches for non-compliant drivers? Should we remove offloads capability from non-compliant drivers? > > Oh, and I don't trust that nobody were thinking that it would break testpmd > > for non-Intel drivers. > > Well, believe it or not, but yes, I missed that one. > I think I already admitted that it was my fault, and apologized for that. And it's my fault not having seen that before. I was hoping that good reviews would be done by other contributors. > But sure, it is your choice to trust me here or not. Konstantin I trust you. However if nobody else was reviewing this patchset at Intel, this is probably an issue. And more importantly we must encourage other vendors to review such major patch for the ethdev API.