Thanks. I've posted v3 of this patch to address these comments. On Thu, Apr 30, 2015 at 5:55 AM, Maxim Uvarov <[email protected]> wrote:
> Bill, I think structure on top has to have some description about 2 bits > bit field. To make it easy for first time reader understand what each bit > mean. > > On 04/30/2015 12:28, Bill Fischofer wrote: > >> Lazy parsing defers parsing until the results are actually needed. >> This allows applications that do their own parsing and never reference >> ODP parse results to avoid the overhead of SW parsing. >> >> Signed-off-by: Bill Fischofer <[email protected]> >> --- >> >> Changes in v2; >> - Corrected processing of odp_packet_has_error() >> >> .../linux-generic/include/odp_packet_internal.h | 86 >> ++++++++++++---------- >> platform/linux-generic/odp_packet.c | 26 +++++-- >> platform/linux-generic/odp_packet_flags.c | 79 >> +++++++++++--------- >> platform/linux-generic/odp_packet_io.c | 2 +- >> platform/linux-generic/odp_packet_socket.c | 6 +- >> 5 files changed, 117 insertions(+), 82 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp_packet_internal.h >> b/platform/linux-generic/include/odp_packet_internal.h >> index c3dcdd8..5f9dcfc 100644 >> --- a/platform/linux-generic/include/odp_packet_internal.h >> +++ b/platform/linux-generic/include/odp_packet_internal.h >> @@ -31,37 +31,37 @@ extern "C" { >> */ >> typedef union { >> /* All input flags */ >> - uint32_t all; >> + uint64_t all; >> >> > 64_t here > > struct { >> /* Bitfield flags for each protocol */ >> - uint32_t l2:1; /**< known L2 protocol present */ >> - uint32_t l3:1; /**< known L3 protocol present */ >> - uint32_t l4:1; /**< known L4 protocol present */ >> - >> - uint32_t eth:1; /**< Ethernet */ >> - uint32_t jumbo:1; /**< Jumbo frame */ >> - uint32_t vlan:1; /**< VLAN hdr found */ >> - uint32_t vlan_qinq:1; /**< Stacked VLAN found, QinQ */ >> - >> - uint32_t snap:1; /**< SNAP */ >> - uint32_t arp:1; /**< ARP */ >> - >> - uint32_t ipv4:1; /**< IPv4 */ >> - uint32_t ipv6:1; /**< IPv6 */ >> - uint32_t ipfrag:1; /**< IP fragment */ >> - uint32_t ipopt:1; /**< IP optional headers */ >> - uint32_t ipsec:1; /**< IPSec decryption may be needed >> */ >> - >> - uint32_t udp:1; /**< UDP */ >> - uint32_t tcp:1; /**< TCP */ >> - uint32_t tcpopt:1; /**< TCP options present */ >> - uint32_t sctp:1; /**< SCTP */ >> - uint32_t icmp:1; /**< ICMP */ >> + uint32_t l2:2; /**< known L2 protocol present */ >> + uint32_t l3:2; /**< known L3 protocol present */ >> + uint32_t l4:2; /**< known L4 protocol present */ >> + >> + uint32_t eth:2; /**< Ethernet */ >> + uint32_t jumbo:2; /**< Jumbo frame */ >> + uint32_t vlan:2; /**< VLAN hdr found */ >> + uint32_t vlan_qinq:2; /**< Stacked VLAN found, QinQ */ >> + >> + uint32_t snap:2; /**< SNAP */ >> + uint32_t arp:2; /**< ARP */ >> + >> + uint32_t ipv4:2; /**< IPv4 */ >> + uint32_t ipv6:2; /**< IPv6 */ >> + uint32_t ipfrag:2; /**< IP fragment */ >> + uint32_t ipopt:2; /**< IP optional headers */ >> + uint32_t ipsec:2; /**< IPSec decryption may be needed >> */ >> + >> + uint32_t udp:2; /**< UDP */ >> + uint32_t tcp:2; /**< TCP */ >> + uint32_t tcpopt:2; /**< TCP options present */ >> + uint32_t sctp:2; /**< SCTP */ >> + uint32_t icmp:2; /**< ICMP */ >> }; >> > 32_t here. Should be also 64. > > } input_flags_t; >> -_ODP_STATIC_ASSERT(sizeof(input_flags_t) == sizeof(uint32_t), >> +_ODP_STATIC_ASSERT(sizeof(input_flags_t) == sizeof(uint64_t), >> "INPUT_FLAGS_SIZE_ERROR"); >> /** >> @@ -73,13 +73,13 @@ typedef union { >> struct { >> /* Bitfield flags for each detected error */ >> - uint32_t app_error:1; /**< Error bit for application use >> */ >> - uint32_t frame_len:1; /**< Frame length error */ >> - uint32_t snap_len:1; /**< Snap length error */ >> - uint32_t l2_chksum:1; /**< L2 checksum error, checks TBD >> */ >> - uint32_t ip_err:1; /**< IP error, checks TBD */ >> - uint32_t tcp_err:1; /**< TCP error, checks TBD */ >> - uint32_t udp_err:1; /**< UDP error, checks TBD */ >> + uint32_t app_error:2; /**< Error bit for application use >> */ >> + uint32_t frame_len:2; /**< Frame length error */ >> + uint32_t snap_len:2; /**< Snap length error */ >> + uint32_t l2_chksum:2; /**< L2 checksum error, checks TBD >> */ >> + uint32_t ip_err:2; /**< IP error, checks TBD */ >> + uint32_t tcp_err:2; /**< TCP error, checks TBD */ >> + uint32_t udp_err:2; /**< UDP error, checks TBD */ >> }; >> } error_flags_t; >> @@ -95,10 +95,10 @@ typedef union { >> struct { >> /* Bitfield flags for each output option */ >> - uint32_t l3_chksum_set:1; /**< L3 chksum bit is valid */ >> - uint32_t l3_chksum:1; /**< L3 chksum override */ >> - uint32_t l4_chksum_set:1; /**< L3 chksum bit is valid */ >> - uint32_t l4_chksum:1; /**< L4 chksum override */ >> + uint32_t l3_chksum_set:2; /**< L3 chksum bit is valid */ >> + uint32_t l3_chksum:2; /**< L3 chksum override */ >> + uint32_t l4_chksum_set:2; /**< L3 chksum bit is valid */ >> + uint32_t l4_chksum:2; /**< L4 chksum override */ >> }; >> } output_flags_t; >> @@ -248,6 +248,18 @@ static inline void packet_set_len(odp_packet_t >> pkt, uint32_t len) >> odp_packet_hdr(pkt)->frame_len = len; >> } >> +#define ODP_PACKET_OFFSET_UNPARSED (ODP_PACKET_OFFSET_INVALID - 1) >> + >> +static inline void _odp_packet_reset_parse(odp_packet_t pkt) >> +{ >> + odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); >> + pkt_hdr->input_flags.all = -1; >> + pkt_hdr->error_flags.all = -1; >> > > setting -1 for unsigned u64 does not look good. > > > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_UNPARSED; >> + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_UNPARSED; >> + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_UNPARSED; >> +} >> + >> /* Forward declarations */ >> int _odp_packet_copy_to_packet(odp_packet_t srcpkt, uint32_t srcoffset, >> odp_packet_t dstpkt, uint32_t dstoffset, >> @@ -257,7 +269,7 @@ void _odp_packet_copy_md_to_packet(odp_packet_t >> srcpkt, odp_packet_t dstpkt); >> odp_packet_t _odp_packet_alloc(odp_pool_t pool_hdl); >> -int _odp_packet_parse(odp_packet_t pkt); >> +int _odp_packet_parse(odp_packet_hdr_t *pkt_hdr); >> /* Convert a packet handle to a buffer handle */ >> odp_buffer_t _odp_packet_to_buffer(odp_packet_t pkt); >> diff --git a/platform/linux-generic/odp_packet.c >> b/platform/linux-generic/odp_packet.c >> index c5a3f7d..ac171cd 100644 >> --- a/platform/linux-generic/odp_packet.c >> +++ b/platform/linux-generic/odp_packet.c >> @@ -242,12 +242,17 @@ void odp_packet_user_u64_set(odp_packet_t pkt, >> uint64_t ctx) >> void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t *len) >> { >> odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); >> + if (pkt_hdr->l2_offset == ODP_PACKET_OFFSET_UNPARSED) >> + _odp_packet_parse(pkt_hdr); >> return packet_map(pkt_hdr, pkt_hdr->l2_offset, len); >> } >> uint32_t odp_packet_l2_offset(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->l2_offset; >> + odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); >> + if (pkt_hdr->l2_offset == ODP_PACKET_OFFSET_UNPARSED) >> + _odp_packet_parse(pkt_hdr); >> + return pkt_hdr->l2_offset; >> } >> int odp_packet_l2_offset_set(odp_packet_t pkt, uint32_t offset) >> @@ -264,12 +269,17 @@ int odp_packet_l2_offset_set(odp_packet_t pkt, >> uint32_t offset) >> void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t *len) >> { >> odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); >> + if (pkt_hdr->l3_offset == ODP_PACKET_OFFSET_UNPARSED) >> + _odp_packet_parse(pkt_hdr); >> return packet_map(pkt_hdr, pkt_hdr->l3_offset, len); >> } >> uint32_t odp_packet_l3_offset(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->l3_offset; >> + odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); >> + if (pkt_hdr->l3_offset == ODP_PACKET_OFFSET_UNPARSED) >> + _odp_packet_parse(pkt_hdr); >> + return pkt_hdr->l3_offset; >> } >> int odp_packet_l3_offset_set(odp_packet_t pkt, uint32_t offset) >> @@ -286,12 +296,17 @@ int odp_packet_l3_offset_set(odp_packet_t pkt, >> uint32_t offset) >> void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t *len) >> { >> odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); >> + if (pkt_hdr->l4_offset == ODP_PACKET_OFFSET_UNPARSED) >> + _odp_packet_parse(pkt_hdr); >> return packet_map(pkt_hdr, pkt_hdr->l4_offset, len); >> } >> uint32_t odp_packet_l4_offset(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->l4_offset; >> + odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); >> + if (pkt_hdr->l4_offset == ODP_PACKET_OFFSET_UNPARSED) >> + _odp_packet_parse(pkt_hdr); >> + return pkt_hdr->l4_offset; >> } >> int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset) >> @@ -558,7 +573,7 @@ void odp_packet_print(odp_packet_t pkt) >> len += snprintf(&str[len], n-len, "Packet "); >> len += odp_buffer_snprint(&str[len], n-len, (odp_buffer_t) pkt); >> len += snprintf(&str[len], n-len, >> - " input_flags 0x%x\n", hdr->input_flags.all); >> + " input_flags 0x%" PRIx64 "\n", >> hdr->input_flags.all); >> len += snprintf(&str[len], n-len, >> " error_flags 0x%x\n", hdr->error_flags.all); >> len += snprintf(&str[len], n-len, >> @@ -790,9 +805,8 @@ static inline void parse_udp(odp_packet_hdr_t >> *pkt_hdr, >> * Simple packet parser >> */ >> -int _odp_packet_parse(odp_packet_t pkt) >> +int _odp_packet_parse(odp_packet_hdr_t *pkt_hdr) >> { >> - odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); >> odph_ethhdr_t *eth; >> odph_vlanhdr_t *vlan; >> uint16_t ethtype; >> diff --git a/platform/linux-generic/odp_packet_flags.c >> b/platform/linux-generic/odp_packet_flags.c >> index ab3d12f..97e1cbd 100644 >> --- a/platform/linux-generic/odp_packet_flags.c >> +++ b/platform/linux-generic/odp_packet_flags.c >> @@ -7,182 +7,191 @@ >> #include <odp/packet_flags.h> >> #include <odp_packet_internal.h> >> +#define retflag(p, x) do { \ >> + odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(p); \ >> + if (pkt_hdr->x > 1) \ >> + _odp_packet_parse(pkt_hdr); \ >> + return pkt_hdr->x; \ >> + } while (0) >> int odp_packet_has_error(odp_packet_t pkt) >> { >> - return (odp_packet_hdr(pkt)->error_flags.all != 0); >> + odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); >> + if (pkt_hdr->error_flags.all == 0xffffffff) >> > it's better to add define for that 0xfff.. > > > > + _odp_packet_parse(pkt_hdr); >> + return odp_packet_hdr(pkt)->error_flags.all != 0; >> } >> /* Get Input Flags */ >> int odp_packet_has_l2(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.l2; >> + retflag(pkt, input_flags.l2); >> } >> int odp_packet_has_l3(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.l3; >> + retflag(pkt, input_flags.l3); >> } >> int odp_packet_has_l4(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.l4; >> + retflag(pkt, input_flags.l4); >> } >> int odp_packet_has_eth(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.eth; >> + retflag(pkt, input_flags.eth); >> } >> int odp_packet_has_jumbo(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.jumbo; >> + retflag(pkt, input_flags.jumbo); >> } >> int odp_packet_has_vlan(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.vlan; >> + retflag(pkt, input_flags.vlan); >> } >> int odp_packet_has_vlan_qinq(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.vlan_qinq; >> + retflag(pkt, input_flags.vlan_qinq); >> } >> int odp_packet_has_arp(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.arp; >> + retflag(pkt, input_flags.arp); >> } >> int odp_packet_has_ipv4(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.ipv4; >> + retflag(pkt, input_flags.ipv4); >> } >> int odp_packet_has_ipv6(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.ipv6; >> + retflag(pkt, input_flags.ipv6); >> } >> int odp_packet_has_ipfrag(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.ipfrag; >> + retflag(pkt, input_flags.ipfrag); >> } >> int odp_packet_has_ipopt(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.ipopt; >> + retflag(pkt, input_flags.ipopt); >> } >> int odp_packet_has_ipsec(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.ipsec; >> + retflag(pkt, input_flags.ipsec); >> } >> int odp_packet_has_udp(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.udp; >> + retflag(pkt, input_flags.udp); >> } >> int odp_packet_has_tcp(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.tcp; >> + retflag(pkt, input_flags.tcp); >> } >> int odp_packet_has_sctp(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.sctp; >> + retflag(pkt, input_flags.sctp); >> } >> int odp_packet_has_icmp(odp_packet_t pkt) >> { >> - return odp_packet_hdr(pkt)->input_flags.icmp; >> + retflag(pkt, input_flags.icmp); >> } >> /* Set Input Flags */ >> void odp_packet_has_l2_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.l2 = val; >> + odp_packet_hdr(pkt)->input_flags.l2 = val & 1; >> } >> void odp_packet_has_l3_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.l3 = val; >> + odp_packet_hdr(pkt)->input_flags.l3 = val & 1; >> } >> void odp_packet_has_l4_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.l4 = val; >> + odp_packet_hdr(pkt)->input_flags.l4 = val & 1; >> } >> void odp_packet_has_eth_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.eth = val; >> + odp_packet_hdr(pkt)->input_flags.eth = val & 1; >> } >> void odp_packet_has_jumbo_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.jumbo = val; >> + odp_packet_hdr(pkt)->input_flags.jumbo = val & 1; >> } >> void odp_packet_has_vlan_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.vlan = val; >> + odp_packet_hdr(pkt)->input_flags.vlan = val & 1; >> } >> void odp_packet_has_vlan_qinq_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.vlan_qinq = val; >> + odp_packet_hdr(pkt)->input_flags.vlan_qinq = val & 1; >> } >> void odp_packet_has_arp_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.arp = val; >> + odp_packet_hdr(pkt)->input_flags.arp = val & 1; >> } >> void odp_packet_has_ipv4_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.ipv4 = val; >> + odp_packet_hdr(pkt)->input_flags.ipv4 = val & 1; >> } >> void odp_packet_has_ipv6_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.ipv6 = val; >> + odp_packet_hdr(pkt)->input_flags.ipv6 = val & 1; >> } >> void odp_packet_has_ipfrag_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.ipfrag = val; >> + odp_packet_hdr(pkt)->input_flags.ipfrag = val & 1; >> } >> void odp_packet_has_ipopt_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.ipopt = val; >> + odp_packet_hdr(pkt)->input_flags.ipopt = val & 1; >> } >> void odp_packet_has_ipsec_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.ipsec = val; >> + odp_packet_hdr(pkt)->input_flags.ipsec = val & 1; >> } >> void odp_packet_has_udp_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.udp = val; >> + odp_packet_hdr(pkt)->input_flags.udp = val & 1; >> } >> void odp_packet_has_tcp_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.tcp = val; >> + odp_packet_hdr(pkt)->input_flags.tcp = val & 1; >> } >> void odp_packet_has_sctp_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.sctp = val; >> + odp_packet_hdr(pkt)->input_flags.sctp = val & 1; >> } >> void odp_packet_has_icmp_set(odp_packet_t pkt, int val) >> { >> - odp_packet_hdr(pkt)->input_flags.icmp = val; >> + odp_packet_hdr(pkt)->input_flags.icmp = val & 1; >> } >> diff --git a/platform/linux-generic/odp_packet_io.c >> b/platform/linux-generic/odp_packet_io.c >> index f16685d..5ae24b9 100644 >> --- a/platform/linux-generic/odp_packet_io.c >> +++ b/platform/linux-generic/odp_packet_io.c >> @@ -378,7 +378,7 @@ static int deq_loopback(pktio_entry_t *pktio_entry, >> odp_packet_t pkts[], >> for (i = 0; i < nbr; ++i) { >> pkts[i] = >> _odp_packet_from_buffer(odp_hdr_to_buf(hdr_tbl[i])); >> - _odp_packet_parse(pkts[i]); >> + _odp_packet_reset_parse(pkts[i]); >> } >> return nbr; >> diff --git a/platform/linux-generic/odp_packet_socket.c >> b/platform/linux-generic/odp_packet_socket.c >> index 2802c43..9272146 100644 >> --- a/platform/linux-generic/odp_packet_socket.c >> +++ b/platform/linux-generic/odp_packet_socket.c >> @@ -251,7 +251,7 @@ int recv_pkt_sock_basic(pkt_sock_t *const pkt_sock, >> /* Parse and set packet header data */ >> odp_packet_pull_tail(pkt, pkt_sock->max_frame_len - >> recv_bytes); >> - _odp_packet_parse(pkt); >> + _odp_packet_reset_parse(pkt); >> pkt_table[nb_rx] = pkt; >> pkt = ODP_PACKET_INVALID; >> @@ -360,7 +360,7 @@ int recv_pkt_sock_mmsg(pkt_sock_t *const pkt_sock, >> odp_packet_pull_tail(pkt_table[i], >> pkt_sock->max_frame_len - >> msgvec[i].msg_len); >> - _odp_packet_parse(pkt_table[i]); >> + _odp_packet_reset_parse(pkt_table[i]); >> pkt_table[nb_rx] = pkt_table[i]; >> nb_rx++; >> @@ -519,7 +519,7 @@ static inline unsigned pkt_mmap_v2_rx(int sock, >> struct ring *ring, >> mmap_rx_user_ready(ppd.raw); >> /* Parse and set packet header data */ >> - _odp_packet_parse(pkt_table[i]); >> + _odp_packet_reset_parse(pkt_table[i]); >> frame_num = next_frame_num; >> i++; >> > > _______________________________________________ > lng-odp mailing list > [email protected] > https://lists.linaro.org/mailman/listinfo/lng-odp >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
