| Message ID | 1490367881-16266-4-git-send-email-petri.savolainen@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [API-NEXT,v3,1/4] api: ipsec: extend lookaside API | expand |
This patch is independent of the IPsec patch series and should be a separate patch. On Fri, Mar 24, 2017 at 10:04 AM, Petri Savolainen <petri.savolainen@linaro.org> wrote: > Checksum insertion has pktio interface level configuration > options. Per packet override is needed for example when > L4 checksumming is enabled and application forwards packets. > Forwarded packets need to maintain original, end-to-end checksum > value. > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > include/odp/api/spec/packet.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/odp/api/spec/packet.h b/include/odp/api/spec/packet.h > index 92c35ae..42ef51f 100644 > --- a/include/odp/api/spec/packet.h > +++ b/include/odp/api/spec/packet.h > @@ -1366,6 +1366,25 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt); > int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); > > /** > + * Force checksum insertion > + * > + * Override checksum insertion configuration per packet. > + * > + * @param pkt Packet handle > + * @param l3 0: do not insert L3 checksum > + * 1: insert L3 checksum > + * -1: use default L3 checksum configuration > + * > + * @param l4 0: do not insert L4 checksum > + * 1: insert L4 checksum > + * -1: use default L4 checksum configuration > + * > + * @retval 0 on success > + * @retval <0 on failure > + */ > +int odp_packet_chksum_force(odp_packet_t pkt, int l3, int l4); Several comments. 1. Why is this not a void function, as presumably this function is advisory in nature? How would it fail and what would an application be expected to do in the event of a reported failure? Having to check return codes for this would seem to be adding a lot of overhead for little benefit. 2. "force" sounds awkward. Perhaps odp_packet_chksum_override(), odp_packet_chksum_insert(), or simply odp_packet_chksum() might be better? 3. To make this useful, shouldn't it be coupled with an odp_pktio_capability() extension that reports whether per-packet chksum overrides are supported on this interface, since checksum insertion on output is configured as part of the odp_pktout_config_opt_t for each odp_pktio_t? > + > +/** > * Packet flow hash value > * > * Returns the hash generated from the packet header. Use > -- > 2.8.1 >
> -----Original Message----- > From: Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Friday, March 24, 2017 5:39 PM > To: Petri Savolainen <petri.savolainen@linaro.org> > Cc: lng-odp-forward <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [API-NEXT PATCH v3 4/4] api: packet: add per packet > checksum control > > This patch is independent of the IPsec patch series and should be a > separate patch. IPsec inline patch refers to this missing feature (as does current pktout config). /** * Configuration options for IPSEC outbound processing */ typedef struct odp_ipsec_outbound_config_t { /** Flags to control L3/L4 checksum insertion as part of outbound * packet processing. Packet must have set with valid L3/L4 offsets. * Checksum configuration is ignored for packets that checksum cannot * be computed for (e.g. IPv4 fragments). Application may use a packet * metadata flag to disable checksum insertion per packet bases. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ So, it is related and needs to be added soon. There's no harm to do that right now. > > /** > > + * Force checksum insertion > > + * > > + * Override checksum insertion configuration per packet. > > + * > > + * @param pkt Packet handle > > + * @param l3 0: do not insert L3 checksum > > + * 1: insert L3 checksum > > + * -1: use default L3 checksum configuration > > + * > > + * @param l4 0: do not insert L4 checksum > > + * 1: insert L4 checksum > > + * -1: use default L4 checksum configuration > > + * > > + * @retval 0 on success > > + * @retval <0 on failure > > + */ > > +int odp_packet_chksum_force(odp_packet_t pkt, int l3, int l4); > > Several comments. > > 1. Why is this not a void function, as presumably this function is > advisory in nature? How would it fail and what would an application be > expected to do in the event of a reported failure? Having to check > return codes for this would seem to be adding a lot of overhead for > little benefit. > > 2. "force" sounds awkward. Perhaps odp_packet_chksum_override(), > odp_packet_chksum_insert(), or simply odp_packet_chksum() might be > better? > > 3. To make this useful, shouldn't it be coupled with an > odp_pktio_capability() extension that reports whether per-packet > chksum overrides are supported on this interface, since checksum > insertion on output is configured as part of the > odp_pktout_config_opt_t for each odp_pktio_t? I changed it to these two functions under. Void is correct, I did copy-paste odp_packet_l4_offset_set() prototype which has a return value. Actually, those offset_set functions should be changed to be void as well. Per packet checksum control is mandatory. HW is typically implemented with per packet control. As explained in commit log text, e.g. a L4 checksum must not be overwritten during routing/forwarding - it's an end-to-end feature. -Petri /** * Layer 3 checksum insertion override * * Override checksum insertion configuration per packet. This per packet setting * overrides a higher level configuration for checksum insertion into a L3 * header during packet output processing. * * @param pkt Packet handle * @param l3 0: do not insert L3 checksum * 1: insert L3 checksum */ void odp_packet_l3_chksum_insert(odp_packet_t pkt, int l3); /** * Layer 4 checksum insertion override * * Override checksum insertion configuration per packet. This per packet setting * overrides a higher level configuration for checksum insertion into a L4 * header during packet output processing. * * @param pkt Packet handle * @param l4 0: do not insert L4 checksum * 1: insert L4 checksum */ void odp_packet_l4_chksum_insert(odp_packet_t pkt, int l4);
On Mon, Mar 27, 2017 at 2:49 AM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: > > >> -----Original Message----- >> From: Bill Fischofer [mailto:bill.fischofer@linaro.org] >> Sent: Friday, March 24, 2017 5:39 PM >> To: Petri Savolainen <petri.savolainen@linaro.org> >> Cc: lng-odp-forward <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] [API-NEXT PATCH v3 4/4] api: packet: add per packet >> checksum control >> >> This patch is independent of the IPsec patch series and should be a >> separate patch. > > IPsec inline patch refers to this missing feature (as does current pktout config). > > /** > * Configuration options for IPSEC outbound processing > */ > typedef struct odp_ipsec_outbound_config_t { > /** Flags to control L3/L4 checksum insertion as part of outbound > * packet processing. Packet must have set with valid L3/L4 offsets. > * Checksum configuration is ignored for packets that checksum cannot > * be computed for (e.g. IPv4 fragments). Application may use a packet > * metadata flag to disable checksum insertion per packet bases. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > So, it is related and needs to be added soon. There's no harm to do that right now. I agree and don't see why this cannot be expedited, but it's still a separate patch, so please post it as such along with the changes you propose below. > > >> > /** >> > + * Force checksum insertion >> > + * >> > + * Override checksum insertion configuration per packet. >> > + * >> > + * @param pkt Packet handle >> > + * @param l3 0: do not insert L3 checksum >> > + * 1: insert L3 checksum >> > + * -1: use default L3 checksum configuration >> > + * >> > + * @param l4 0: do not insert L4 checksum >> > + * 1: insert L4 checksum >> > + * -1: use default L4 checksum configuration >> > + * >> > + * @retval 0 on success >> > + * @retval <0 on failure >> > + */ >> > +int odp_packet_chksum_force(odp_packet_t pkt, int l3, int l4); >> >> Several comments. >> >> 1. Why is this not a void function, as presumably this function is >> advisory in nature? How would it fail and what would an application be >> expected to do in the event of a reported failure? Having to check >> return codes for this would seem to be adding a lot of overhead for >> little benefit. >> >> 2. "force" sounds awkward. Perhaps odp_packet_chksum_override(), >> odp_packet_chksum_insert(), or simply odp_packet_chksum() might be >> better? >> >> 3. To make this useful, shouldn't it be coupled with an >> odp_pktio_capability() extension that reports whether per-packet >> chksum overrides are supported on this interface, since checksum >> insertion on output is configured as part of the >> odp_pktout_config_opt_t for each odp_pktio_t? > > > I changed it to these two functions under. Void is correct, I did copy-paste odp_packet_l4_offset_set() prototype which has a return value. Actually, those offset_set functions should be changed to be void as well. Not a bad idea for a cleanup, but that would be a separate patch as well. > > Per packet checksum control is mandatory. HW is typically implemented with per packet control. As explained in commit log text, e.g. a L4 checksum must not be overwritten during routing/forwarding - it's an end-to-end feature. > > -Petri > > > /** > * Layer 3 checksum insertion override > * > * Override checksum insertion configuration per packet. This per packet setting > * overrides a higher level configuration for checksum insertion into a L3 > * header during packet output processing. > * > * @param pkt Packet handle > * @param l3 0: do not insert L3 checksum > * 1: insert L3 checksum > */ > void odp_packet_l3_chksum_insert(odp_packet_t pkt, int l3); > > /** > * Layer 4 checksum insertion override > * > * Override checksum insertion configuration per packet. This per packet setting > * overrides a higher level configuration for checksum insertion into a L4 > * header during packet output processing. > * > * @param pkt Packet handle > * @param l4 0: do not insert L4 checksum > * 1: insert L4 checksum > */ > void odp_packet_l4_chksum_insert(odp_packet_t pkt, int l4); I think separating these out like this looks better. > >
diff --git a/include/odp/api/spec/packet.h b/include/odp/api/spec/packet.h index 92c35ae..42ef51f 100644 --- a/include/odp/api/spec/packet.h +++ b/include/odp/api/spec/packet.h @@ -1366,6 +1366,25 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt); int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); /** + * Force checksum insertion + * + * Override checksum insertion configuration per packet. + * + * @param pkt Packet handle + * @param l3 0: do not insert L3 checksum + * 1: insert L3 checksum + * -1: use default L3 checksum configuration + * + * @param l4 0: do not insert L4 checksum + * 1: insert L4 checksum + * -1: use default L4 checksum configuration + * + * @retval 0 on success + * @retval <0 on failure + */ +int odp_packet_chksum_force(odp_packet_t pkt, int l3, int l4); + +/** * Packet flow hash value * * Returns the hash generated from the packet header. Use
Checksum insertion has pktio interface level configuration options. Per packet override is needed for example when L4 checksumming is enabled and application forwards packets. Forwarded packets need to maintain original, end-to-end checksum value. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- include/odp/api/spec/packet.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) -- 2.8.1