As I mentioned during the meeting, the intent here is for a fastpath to get access to the packet headers. Since the minimum segment length can be specified when pools are created, the application can ensure that the first segment is large enough to fit the headers it is interested in. So odp_packet_data() is Ok to use for initial packet header processing. Once a packet has moved through one or more processing stages, however, applications need to be aware of potential segment breaks.
Since this API has been around since the start of ODP we may just wish to add additional documentation rather than try to deprecate it as it is still useful when used as described above, as was the original intent. On Thu, Feb 15, 2018 at 8:48 AM, Dmitry Eremin-Solenikov < dmitry.ereminsoleni...@linaro.org> wrote: > On 15/02/18 17:42, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > > > >> -----Original Message----- > >> From: Dmitry Eremin-Solenikov [mailto:dmitry.ereminsoleni...@linaro.org > ] > >> Sent: Thursday, February 15, 2018 4:00 PM > >> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>; > >> Bill Fischofer <bill.fischo...@linaro.org> > >> Cc: lng-odp-forward <lng-odp@lists.linaro.org> > >> Subject: Re: [lng-odp] odp_packet_data() considered harmful > >> > >> On 15/02/18 16:32, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> > >>> odp_packet_offset() is more complex than the proposed > >> odp_packet_data_seg_len(). Application mostly finds its data from the > >> first segment, so it's important to keep the most common use case fast > (== > >> simple). We cannot and should not modify/remove current data() and > >> offset() functions. But we can add another which combines data() + > >> seg_len(), which is mostly enough for application. When application > knows > >> already that all (interesting) data is in the first segment, it can use > >> data() which is the fastest and simplest function (to inline) to get > >> access to the data. > >> > >> I'm fine with proposed odp_packet_data_seg_len() if that will allow us > >> to drop odp_packet_data(). > > > > There's no need or possibility to remove data(). Documentation for > data() may be improved to be more explicit about possibility of multiple > segments. The function just returns pointer to current head of data, it > does not promise anything more. When application knows already how much > data follows (and usually all data follows), it does not need ask e.g. > segment length over and over again. > > Well. We have documentation that packets are (can be) segmented. People > still try to use odp_packet_data() to address whole packet (see PR > #422/#470). We can add big fat warning to the function. But I fear that > we might gain nothing from that warning. > > Let's agree to expand documentation for now and to deprecate that > function when next issue of using odp_packet_data() for the whole packet > arises. Will that work for you? > > > > > static inline void* odp_packet_data(pkt) { > > return (pkt_desc_t)pkt->data_ptr; > > } > > > > static inline void* odp_packet_data_seg_len(pkt, *len) { > > *len = (pkt_desc_t)pkt->seg_len; > > return (pkt_desc_t)pkt->data_ptr; > > } > > If it is inlineable, second one will be as fast as first one, if you add > if (len != NULL) check. GCC/Clang will optimize static check + > assignment away. > > > The first is faster when application does not need to ask the segment > length. > > > -- > With best wishes > Dmitry >
