| Message ID | A400FC85CF2669428A5A081F01B94F531D9C9200@DEMUMBX012.nsn-intra.net |
|---|---|
| State | New |
| Headers | show |
On 17 December 2014 at 13:52, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > > > > From: ext Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Wednesday, December 17, 2014 2:40 PM > To: Savolainen, Petri (NSN - FI/Espoo) > Cc: Petri Savolainen; lng-odp-forward > Subject: Re: [lng-odp] [PATCH] Remove packet l2/l3/l4 offset adjustments > > > > While ODP may not know why an application is choosing to make a specific > push/pull request, the implications of any given call are precise. After > pushing the head by X bytes, all of the other packet offset metadata are now > in error until and unless they are adjusted. If it is part of the API > definition that the implementation MUST NOT assist in these adjustments, > then there are several implications: > > > > 1. Other ODP APIs that reference these now-erroneous metadata are > invalidated and will silently return incorrect values. Does this help > promote good programming? > > > > We have to specify (per ODP API) anyway which metadata fields are required > to have valid values. If application creates a packet and passes it to e.g. > a crypto IPSEC API, it has to first fill in the correct metadata (e.g. l3 > offset). > > > > 2. The application takes responsibility for knowing which metadata items > have been invalidated, even if additional metadata is added to ODP in > subsequent releases. How does it know this? > > > > It’s then a new API version. An application is coded against an API version. > > > > 3. As has been pointed out, since the API does not provide relative offset > adjustment APIs, for an application to adjust a given offset value > correctly, it must read/modify/write each individually. Hardly efficient. > > > > Application likely read the offsets anyway before push/pull. Also that data > is likely on same cache with all other packet metadata. Which makes it efficient for the ODP implementation to update the Lx offsets when the packet is pulled (or pushed). > There’s really no > difference if application increments offsets by ‘len’ or if implementation Yes semantically there is a big difference. Should ODP maintain the correctness and consistency of its packet attributes as some attributes are changed? Or is the correctness of the packet attributes completely determined by the application? > does it. The real difference is that is an offset is written twice vs only > once, OR once vs not at all. There might be a slight performance difference between ODP making these updates automatically as opposed to the application doing them. I am not sure this necessarily will be in the favor of the application though. The overhead of adjusting three offsets (three loads, three, adds/subs, three stores, possibly 4-5 cycles on a dual-issue in-order CPU) when doing some other change to the packet (e.g. pull header) might easily be dwarfed by the overhead of calling the ODP functions and translating packet handle to actual pointer. If you are using the performance argument against ODP making these adjustments automatically, I think you should provide actual numbers that show the difference in performance. Otherwise we are just speculating and basing API design on speculation. > > > > > > 4. Implementations that support push/pull operations with HW assistance may > need to add special code to undo any automatic metadata adjustments to > comply with the API. > > > > Is there any HW like this? Typically these are just fields in the packet > descriptor (in the memory) and HW does not see it before application passes > the packet to e.g. the output HW. > > > > -Petri > > > > So it's not at all clear how the application benefits from this stipulation > or how it would be harmed if it was the implementations responsibility to > maintain the system metadata in a consistent state. > > > > On Wed, Dec 17, 2014 at 6:24 AM, Savolainen, Petri (NSN - FI/Espoo) > <petri.savolainen@nsn.com> wrote: > > > > Once again. There’s no point of implementation speculating what will happen > to the packet after the pull/push operation. Which L2/L3/L4/… offsets are > going to be changed and in which direction? Between which headers the > application is going to add/remove layers? What header are moved? What > happens next in the chain? What offsets really need an update? Application > has all that knowledge and can do only those updates that are needed. We > keep it simple – if application modifies the packet it is responsible to > maintain metadata offsets accurate. > > > > -Petri > > > > > > > > From: ext Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Wednesday, December 17, 2014 2:06 PM > To: Savolainen, Petri (NSN - FI/Espoo) > Cc: Petri Savolainen; lng-odp-forward > > > Subject: Re: [lng-odp] [PATCH] Remove packet l2/l3/l4 offset adjustments > > > > You are correct that the API states this. What others are pointing out are > the non-intutive implications of choosing to define the API this way. > You've not provided a convincing rationale for making this choice. While > you can assert your right not to do so, it would be helpful to others if you > did. > > > > On Wed, Dec 17, 2014 at 5:51 AM, Savolainen, Petri (NSN - FI/Espoo) > <petri.savolainen@nsn.com> wrote: > > The API says already: > > > > “Operation does not modify packet segmentation or move data. Handles and > > pointers remain valid. User is responsible to update packet meta-data > > offsets when needed.” > > > > If application modifies packet headers (adds/removes/changes them), it must > update those offsets that it or some other block after it in the chain cares > about (reloads from the packet). It may leave some offsets unchanged because > it knows that those are not needed any more. > > > > Some more words can be added, if the current text is not clear enough. The > current implementation does not implement the spec. This patch fixes that > issue. > > > > > > -Petri > > > > > > > > > > From: ext Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Wednesday, December 17, 2014 1:33 PM > To: Petri Savolainen > Cc: lng-odp-forward > Subject: Re: [lng-odp] [PATCH] Remove packet l2/l3/l4 offset adjustments > > > > If we do this we really need a documentation note that says: > > > > Note: After doing a head push/pull, applications cannot use routines like > odp_packet_l3_ptr(), etc., until they make appropriate calls to > odp_packet_l3_offset_set(), etc., to reflect the previous operation. > Otherwise they will receive pointers to incorrect areas of the packet. > > > > On Wed, Dec 17, 2014 at 3:12 AM, Petri Savolainen > <petri.savolainen@linaro.org> wrote: > > Packet head push/pull calls do not automatically adjust > metadata offsets, only data pointer and headroom. > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > platform/linux-generic/include/odp_packet_internal.h | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/platform/linux-generic/include/odp_packet_internal.h > b/platform/linux-generic/include/odp_packet_internal.h > index a0eff30..18e69b3 100644 > --- a/platform/linux-generic/include/odp_packet_internal.h > +++ b/platform/linux-generic/include/odp_packet_internal.h > @@ -192,24 +192,16 @@ static inline void *packet_map(odp_packet_hdr_t > *pkt_hdr, > pkt_hdr->headroom + pkt_hdr->frame_len); > } > > -#define pull_offset(x, len) (x = x < len ? 0 : x - len) > - > static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len) > { > pkt_hdr->headroom -= len; > pkt_hdr->frame_len += len; > - pkt_hdr->l2_offset += len; > - pkt_hdr->l3_offset += len; > - pkt_hdr->l4_offset += len; > } > > static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) > { > pkt_hdr->headroom += len; > pkt_hdr->frame_len -= len; > - pull_offset(pkt_hdr->l2_offset, len); > - pull_offset(pkt_hdr->l3_offset, len); > - pull_offset(pkt_hdr->l4_offset, len); > } > > static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len) > -- > 2.2.0 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h index a0eff30..18e69b3 100644 --- a/platform/linux-generic/include/odp_packet_internal.h +++ b/platform/linux-generic/include/odp_packet_internal.h @@ -192,24 +192,16 @@ static inline void *packet_map(odp_packet_hdr_t *pkt_hdr, pkt_hdr->headroom + pkt_hdr->frame_len); } -#define pull_offset(x, len) (x = x < len ? 0 : x - len) - static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len) { pkt_hdr->headroom -= len; pkt_hdr->frame_len += len; - pkt_hdr->l2_offset += len; - pkt_hdr->l3_offset += len; - pkt_hdr->l4_offset += len; } static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) { pkt_hdr->headroom += len; pkt_hdr->frame_len -= len; - pull_offset(pkt_hdr->l2_offset, len); - pull_offset(pkt_hdr->l3_offset, len); - pull_offset(pkt_hdr->l4_offset, len); } static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len)