From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <olivier.matz@6wind.com> Received: from mail-wm0-f47.google.com (mail-wm0-f47.google.com [74.125.82.47]) by dpdk.org (Postfix) with ESMTP id 0FBA1133F for <dev@dpdk.org>; Mon, 20 Mar 2017 10:00:39 +0100 (CET) Received: by mail-wm0-f47.google.com with SMTP id n11so58435426wma.0 for <dev@dpdk.org>; Mon, 20 Mar 2017 02:00:39 -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:in-reply-to:references :mime-version:content-transfer-encoding; bh=TZ88YAqRvLt9FzA8KVk1+4icF4iODC03mhbe9a7WGWE=; b=e+KnWqeoCwmMLaGRf4hNk3V2HztGCbog0k4AI7m6+Usb1XqeetcvphPUBGylVA8HdH Zgt6mSiwrw7G1o1A091TBtxx9TFR/GAePREHenJtZszJP6CxhQbJqk5dtBJxcYKEdQjY SR/6f4/NS3I2s9FF3nDdSrKvcWO5oumY+5Mm0dNpEpXMBvlxTEnI9IeT1H0/I39cZGwA sgtcd5alwM+2JbV+V3deeHFpVtAIPuY+JRG/H1YLLeto6XajZGvxY4JWCX00R0f3wsDC 5adIx2ZMenNu6B8AGGavJOXvQrKzQwPvg0icFFLCJ3dO+nrWvS6or93Tlj+WWDZYUX1z mYUg== 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=TZ88YAqRvLt9FzA8KVk1+4icF4iODC03mhbe9a7WGWE=; b=AABtPBlvYsCYL4LIvJt1/t4y/k66TKHJUSxznASQC5crLjoUEfSYZ08o8r5XI7wGT2 CdxmIMapKWrvGG2aInCebtkZ+gzGWybbkxm+NNGQdla4PM/OumB6WYaMRJaW4a7lQuvD b0hjaWaT+EokQgXfj7kZCYYxbImpMt7nBmYz/L4Q8DZmKPVlsERwILV5Om37WGhtKxHM OxJVL5/dIBS6iYPV7tiY5/xAfRctME91vW/jqSzprWq441/AtwmajwKNlpPfYNP/s+QY 7K2tW+Tb43GCXbWMAL4bg4nI3E1J8yOH5RsPqB+nybEBjbl5aF0ZYCgVUD9sChO5+cOD Fkkw== X-Gm-Message-State: AFeK/H0tx3oLLXTBsbmNipt3tcQVkJH6shL3vacwuSfuqy4+RJ5US3V8QOBMpbr/fC6gRllI X-Received: by 10.28.139.134 with SMTP id n128mr8627290wmd.132.1490000439393; Mon, 20 Mar 2017 02:00:39 -0700 (PDT) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id v21sm15343237wrd.58.2017.03.20.02.00.39 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 20 Mar 2017 02:00:39 -0700 (PDT) Date: Mon, 20 Mar 2017 10:00:36 +0100 From: Olivier Matz <olivier.matz@6wind.com> To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com> Cc: Jan Blunck <jblunck@infradead.org>, "Richardson, Bruce" <bruce.richardson@intel.com>, "dev@dpdk.org" <dev@dpdk.org> Message-ID: <20170320100036.086109e6@platinum> In-Reply-To: <2601191342CEEE43887BDE71AB9772583FACBF10@IRSMSX109.ger.corp.intel.com> References: <1485271173-13408-1-git-send-email-olivier.matz@6wind.com> <CALe+Z01pVFdEckOUabXTnh1q-xEOmJajTagEB1hvqYZazrG7iA@mail.gmail.com> <2601191342CEEE43887BDE71AB9772583F11B4CC@irsmsx105.ger.corp.intel.com> <CALe+Z01ozmTdWwxcc7mG+NhSV16K4+-Pe5uDWASzBBs5oMyh1g@mail.gmail.com> <2601191342CEEE43887BDE71AB9772583F11B633@irsmsx105.ger.corp.intel.com> <20170224150053.279e718d@platinum> <CALe+Z01+K8Odpz3oqk672qsKnjVAXif0TJCwsuPhbcwX+Z11Sg@mail.gmail.com> <2601191342CEEE43887BDE71AB9772583F11E992@irsmsx105.ger.corp.intel.com> <20170228102359.5d601797@platinum> <2601191342CEEE43887BDE71AB9772583F11EA11@irsmsx105.ger.corp.intel.com> <20170228115043.3f78ce52@platinum> <2601191342CEEE43887BDE71AB9772583F11EA96@irsmsx105.ger.corp.intel.com> <20170228132825.37586902@platinum> <2601191342CEEE43887BDE71AB9772583F11EE7A@irsmsx105.ger.corp.intel.com> <20170302174623.268592a7@platinum> <2601191342CEEE43887BDE71AB9772583FACBF10@IRSMSX109.ger.corp.intel.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [RFC 0/8] mbuf: structure reorganization X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Mon, 20 Mar 2017 09:00:40 -0000 Hi Konstantin, On Wed, 8 Mar 2017 11:11:23 +0000, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote: > Hi Olivier, > > > > > Hi Konstantin, > > > > On Tue, 28 Feb 2017 22:53:55 +0000, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote: > > > > > Another thing that doesn't look very convenient to me here - > > > > > We can have 2 different values of timestamp (both normalized and not) > > > > > and there is no clear way for the application to know which one is in > > > > > use right now. So each app writer would have to come-up with his own > > > > > solution. > > > > > > > > It depends: > > > > - the solution you describe is to have the application storing the > > > > normalized value in its private metadata. > > > > - another solution would be to store the normalized value in > > > > m->timestamp. In this case, we would need a flag to tell if the > > > > timestamp value is normalized. > > > > > > My first thought also was about second flag to specify was timestamp > > > already normalized or not. > > > Though I still in doubt - is it all really worth it: extra ol_flag, new function in eth_dev API. > > > My feeling that we trying to overcomplicate things. > > > > I don't see what is so complicated. The idea is just to let the > > application do the normalization if it is required. > > I meant 2 ol_flags and special function just to treat properly one of the mbuf field > seems too much. > Though after second thought might be 2 ol_flags is not a bad idea - > it gives PMD writer a freedom to choose provide a normalized or raw value > on return from rx_burst(). I don't see a real advantage now, but I think this is something that could be added once we have the normalization code. > > If the time is normalized in nanosecond in the PMD, we would still > > need to normalized the time reference (the 0). And for that we'd need > > a call to a synchronization code as well. > > > > > > > > > > The problem pointed out by Jan is that doing the timestamp > > > > normalization may take some CPU cycles, even if a small part of packets > > > > requires it. > > > > > > I understand that point, but from what I've seen with real example: > > > http://dpdk.org/ml/archives/dev/2016-October/048810.html > > > the amount of calculations at RX is pretty small. > > > I don't think it would affect performance in a noticeable way > > > (though I don't have any numbers here to prove it). > > > > I think we can consider by default that adding code in the data path > > impacts performance. > > > > > > > From other side, if user doesn't want a timestamp he can always disable > > > that feature anad save cycles, right? > > > > > > BTW, you and Jan both mention that not every packet would need a timestamp. > > > Instead we need sort of a timestamp for the group of packets? > > > > I think that for many applications the timestamp should be as precise > > as possible for each packet. > > > > > > > Is that really the only foreseen usage model? > > > > No, but it could be one. > > > > > > > If so, then why not to have a special function that would extract 'latest' timestamp > > > from the dev? > > > Or even have tx_burst_extra() that would return a latest timestamp (extra parameter or so). > > > Then there is no need to put timestamp into mbuf at all. > > > > Doing that will give a poor precision for the timestamp. > > > > > > > > > > Applications that > > > > > > are doing this are responsible of what they change. > > > > > > > > > > > > > > > > > > > 3. In theory with eth_dev_detach() - mbuf->port value might be > > > > > > > not valid at the point when application would decide to do > > > > > > > normalization. > > > > > > > > > > > > > > So to me all that approach with delayed normalization seems > > > > > > > unnecessary overcomplicated. Original one suggested by Olivier, > > > > > > > when normalization is done in PMD at RX look much cleaner and > > > > > > > more manageable. > > > > > > > > > > > > Detaching a device requires a synchronization between control and > > > > > > data plane, and not only for this use case. > > > > > > > > > > Of course it does. > > > > > But right now it is possible to do: > > > > > > > > > > eth_rx_burst(port=0, ..., &mbuf, 1); > > > > > eth_dev_detach(port=0, ...); > > > > > ... > > > > > /*process previously received mbuf */ > > > > > > > > > > With what you are proposing it would be not always possible any more. > > > > > > > > With your example, it does not work even without the timestamp feature, > > > > since the mbuf input port would reference an invalid port. > > > > This port is usually used in the application to do a lookup for an port structure, > > > > so it is expected that the entry is valid. It would be even worse if you > > > > do a detach + attach. > > > > > > I am not talking about the mbuf->port value usage. > > > Right now user can access/interpret all metadata fields set by PMD RX routines > > > (vlan, rss hash, ol_flags, ptype, etc.) without need to accessing the device data or > > > calling device functions. > > > With that change it wouldn't be the case anymore. > > > > That's the same for some other functions. If in my application I want > > to call eth_rx_queue_count(m->port), I will have the same problem. > > Yes, but here you are trying to get extra information about device/queue based > on port value stored inside mbuf. > I am talking about information that already stored inside particular mbuf itself. > About m->port itself - as I said before my preference would be to remove it at all > (partly because of that implication - we can't guarantee that m->port information > would be valid though all mbuf lifetime). > But that's probably subject of another discussion. > > > > > I think we also have something quite similar in examples/ptpclient: > > > > rte_eth_rx_burst(portid, 0, &m, 1); > > ... > > parse_ptp_frames(portid, m); > > ... > > ptp_data.portid = portid; > > ... > > rte_eth_timesync_read_tx_timestamp(ptp_data->portid, ...) > > > > > > So, really, I think it's an application issue: when the app deletes > > a port, it should ask itself if there are remaining references to > > it (m->port). > > Hmm, and where in the example below do you see the reference to the m->port? > As I can see, what that the code above does: > - it deduces portid value from global variable - not from m->port > - saves portid info (not from m->port) inside global variable ptp_data.portid > - later inside same function it used that value to call rte_ethdev functions > (via parse_fup or parse_drsp). > > So I am not sure how it relates to the topic we are discussing. It's similar to what I proposed for the timestamp normalization: for both functions, you need to call an ethdev function with a port_id as a parameter. Either you get the port from the mbuf (this is my initial suggestion that you don't like), either you know it because you retrieved your mbuf with rte_eth_rx_burst(port_id, ...) (this is what is done in examples/ptpclient). So, do you still see an issue with having a function to normalize/synchronize the timestamp that takes a port id as a parameter? > Anyway, to summarize how the proposal looks right now: > > 1. m->timestamp value after rx_burst() could be either in raw or normalized format. > 2. validity of m->timesamp and the it's format should be determined by 2 ol_flags > (something like: RX_TIMESTAMP, RX_TIMESTAMP_NORM). > 3. PMD is free to choose what timestamp value to return (raw/normalized) I think it needs to be raw now, because we don't have any normalization code at the moment. Maybe we could add a "normalized" flag if it makes sense in the future, once we have decided what normalized means, in a context where several PMDs/libs can have their own timestamp. But once we have a clear definition of what normalized means + an example of normalization code, we may have this NORM flag. > 4. PMD can provide an optional routine inside devops: > uint64_t dev_ops->timestamp_normalise(uint64_t timestamps); I think (but I'm not sure, it's really out of scope of this patchset), that the timestamp synchronization API will be more complex than that. My current idea: - a rte_timestamp library holds the normalization code - we decide, for instance, that "normalized" means: - unit: nanosecond - based on system clock - reference: 0 = time when rte_timestamp_init() was called - the PMD provides an API to get its clock - the lib provides something like: uint64_t rte_timestamp_normalize(unsigned int port_id, uint64_t timestamp) > 5. If the user wants to use that function it would be his responsibility to map mbuf > to the port it was received from. Yes, if the application uses a port_id, it's its responsibility to ensure that this port exists. Regards, Olivier