| Message ID | 1395934729-9029-2-git-send-email-ciprian.barbu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Thursday, 27 March 2014 17:38:49 UTC+2, Ciprian Barbu wrote: > Signed-off-by: Ciprian Barbu <cipria...@linaro.org <javascript:>> > --- > platform/linux-generic/include/odp_packet_internal.h | 2 +- > platform/linux-generic/source/odp_packet.c | 8 +++++--- > platform/linux-generic/source/odp_packet_netmap.c | 6 +++++- > platform/linux-generic/source/odp_packet_socket.c | 20 > ++++++++++++++++---- > 4 files changed, 27 insertions(+), 9 deletions(-) > > diff --git a/platform/linux-generic/include/odp_packet_internal.h > b/platform/linux-generic/include/odp_packet_internal.h > index 792fc7c..b316b20 100644 > --- a/platform/linux-generic/include/odp_packet_internal.h > +++ b/platform/linux-generic/include/odp_packet_internal.h > @@ -135,7 +135,7 @@ static inline odp_packet_hdr_t > *odp_packet_hdr(odp_packet_t pkt) > /** > * Parse packet and set internal metadata > */ > -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); > +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); > > #ifdef __cplusplus > } > diff --git a/platform/linux-generic/source/odp_packet.c > b/platform/linux-generic/source/odp_packet.c > index 5f07374..700ac39 100644 > --- a/platform/linux-generic/source/odp_packet.c > +++ b/platform/linux-generic/source/odp_packet.c > @@ -138,8 +138,9 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, size_t > offset) > * @param pkt Packet handle > * @param len Packet length in bytes > * @param frame_offset Byte offset to L2 header > + * @return 1 if the packet is valid, 0 otherwise > Return status should be more standard (posix style) with 0 on success and (negative) error code on failure. > */ > -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) > +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) > { > odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); > odp_ethhdr_t *eth; > @@ -154,9 +155,9 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, > size_t frame_offset) > pkt_hdr->frame_offset = frame_offset; > pkt_hdr->frame_len = len; > > - if (odp_unlikely(len < ODP_ETH_LEN_MIN)) { > + if (odp_unlikely(len < ODP_ETHHDR_LEN)) { > pkt_hdr->error_flags.frame_len = 1; > - return; > + return 0; > return -1; > } else if (len > ODP_ETH_LEN_MAX) { > pkt_hdr->input_flags.jumbo = 1; > } > @@ -235,6 +236,7 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, > size_t frame_offset) > } > break; > } > + return 1; > return 0; > } > > static inline uint8_t parse_ipv4(odp_packet_hdr_t *pkt_hdr, odp_ipv4hdr_t > *ipv4, > diff --git a/platform/linux-generic/source/odp_packet_netmap.c > b/platform/linux-generic/source/odp_packet_netmap.c > index 1cbd84c..0dc109c 100644 > --- a/platform/linux-generic/source/odp_packet_netmap.c > +++ b/platform/linux-generic/source/odp_packet_netmap.c > @@ -308,7 +308,11 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, > odp_packet_t pkt_table[], > > /* Initialize, parse and set packet header data > */ > odp_packet_init(pkt); > - odp_packet_parse(pkt, payload_len, > pkt_nm->l2_offset); > + if (!odp_packet_parse(pkt, payload_len, > pkt_nm->l2_offset)) { > + ODP_DBG("Packet parsing failed\n"); > + odp_packet_init(pkt); > + continue; > + } > > pkt_table[nb_rx] = pkt; > pkt = ODP_PACKET_INVALID; > diff --git a/platform/linux-generic/source/odp_packet_socket.c > b/platform/linux-generic/source/odp_packet_socket.c > index aaf2605..65796de 100644 > --- a/platform/linux-generic/source/odp_packet_socket.c > +++ b/platform/linux-generic/source/odp_packet_socket.c > @@ -224,7 +224,11 @@ int recv_pkt_sock(pkt_sock_t *const pkt_sock, > continue; > > /* Parse and set packet header data */ > - odp_packet_parse(pkt, recv_bytes, > pkt_sock->frame_offset); > Packet parse just fills in metadata. If it don't understand (some) packet header format, it's not a failure. By default packet should not be dropped in that case. There's just less metadata and the application (e.g. IP stack) needs to do more work on the packet and do the drop decision. Same applies to the other (netmap and mmap versions). -Petri > + if (!odp_packet_parse(pkt, recv_bytes, > pkt_sock->frame_offset)) { > + ODP_DBG("Packet parsing failed\n"); > + odp_packet_init(pkt); > + continue; > + } > > pkt_table[nb_rx] = pkt; > pkt = ODP_PACKET_INVALID; > @@ -331,8 +335,12 @@ int recv_pkt_sock(pkt_sock_t * const pkt_sock, > } > > /* Parse and set packet header data */ > - odp_packet_parse(pkt_table[i], msgvec[i].msg_len, > - pkt_sock->frame_offset); > + if (!odp_packet_parse(pkt_table[i], msgvec[i].msg_len, > + pkt_sock->frame_offset)) { > + ODP_DBG("Packet parsing failed\n"); > + odp_packet_init(pkt); > + continue; > + } > > pkt_table[nb_rx] = pkt_table[i]; > nb_rx++; > @@ -489,7 +497,11 @@ static inline unsigned pkt_mmap_v2_rx(int sock, > struct ring *ring, > rx_user_ready(ppd.raw); > > /* Parse and set packet header data */ > - odp_packet_parse(pkt_table[i], pkt_len, > frame_offset); > + if (!odp_packet_parse(pkt_table[i], pkt_len, > frame_offset)) { > + ODP_DBG("Packet parsing failed\n"); > + odp_packet_init(pkt_table[i]); > + continue; > + } > > frame_num = next_frame_num; > i++; > -- > 1.8.3.2 > >
On Fri, Mar 28, 2014 at 9:09 AM, Petri Savolainen < petri.savolainen@linaro.org> wrote: > Hi, > > On Thursday, 27 March 2014 17:38:49 UTC+2, Ciprian Barbu wrote: > >> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >> --- >> platform/linux-generic/include/odp_packet_internal.h | 2 +- >> platform/linux-generic/source/odp_packet.c | 8 +++++--- >> platform/linux-generic/source/odp_packet_netmap.c | 6 +++++- >> platform/linux-generic/source/odp_packet_socket.c | 20 >> ++++++++++++++++---- >> 4 files changed, 27 insertions(+), 9 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp_packet_internal.h >> b/platform/linux-generic/include/odp_packet_internal.h >> index 792fc7c..b316b20 100644 >> --- a/platform/linux-generic/include/odp_packet_internal.h >> +++ b/platform/linux-generic/include/odp_packet_internal.h >> @@ -135,7 +135,7 @@ static inline odp_packet_hdr_t >> *odp_packet_hdr(odp_packet_t pkt) >> /** >> * Parse packet and set internal metadata >> */ >> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); >> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); >> >> #ifdef __cplusplus >> } >> diff --git a/platform/linux-generic/source/odp_packet.c >> b/platform/linux-generic/source/odp_packet.c >> index 5f07374..700ac39 100644 >> --- a/platform/linux-generic/source/odp_packet.c >> +++ b/platform/linux-generic/source/odp_packet.c >> @@ -138,8 +138,9 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, >> size_t offset) >> * @param pkt Packet handle >> * @param len Packet length in bytes >> * @param frame_offset Byte offset to L2 header >> + * @return 1 if the packet is valid, 0 otherwise >> > > Return status should be more standard (posix style) with 0 on success and > (negative) error code on failure. > > Sure I can do that, but what I actually did was to look at odp_buffer_is_valid. It fits better because the return status is in both cases a boolean value. > */ >> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) >> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) >> { >> odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); >> odp_ethhdr_t *eth; >> @@ -154,9 +155,9 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, >> size_t frame_offset) >> pkt_hdr->frame_offset = frame_offset; >> pkt_hdr->frame_len = len; >> >> - if (odp_unlikely(len < ODP_ETH_LEN_MIN)) { >> + if (odp_unlikely(len < ODP_ETHHDR_LEN)) { >> pkt_hdr->error_flags.frame_len = 1; >> - return; >> + return 0; >> > > return -1; > > >> } else if (len > ODP_ETH_LEN_MAX) { >> pkt_hdr->input_flags.jumbo = 1; >> } >> @@ -235,6 +236,7 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, >> size_t frame_offset) >> } >> break; >> } >> + return 1; >> > > return 0; > > >> } >> >> static inline uint8_t parse_ipv4(odp_packet_hdr_t *pkt_hdr, >> odp_ipv4hdr_t *ipv4, >> diff --git a/platform/linux-generic/source/odp_packet_netmap.c >> b/platform/linux-generic/source/odp_packet_netmap.c >> index 1cbd84c..0dc109c 100644 >> --- a/platform/linux-generic/source/odp_packet_netmap.c >> +++ b/platform/linux-generic/source/odp_packet_netmap.c >> @@ -308,7 +308,11 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, >> odp_packet_t pkt_table[], >> >> /* Initialize, parse and set packet header data >> */ >> odp_packet_init(pkt); >> - odp_packet_parse(pkt, payload_len, >> pkt_nm->l2_offset); >> + if (!odp_packet_parse(pkt, payload_len, >> pkt_nm->l2_offset)) { >> + ODP_DBG("Packet parsing failed\n"); >> + odp_packet_init(pkt); >> + continue; >> + } >> >> pkt_table[nb_rx] = pkt; >> pkt = ODP_PACKET_INVALID; >> diff --git a/platform/linux-generic/source/odp_packet_socket.c >> b/platform/linux-generic/source/odp_packet_socket.c >> index aaf2605..65796de 100644 >> --- a/platform/linux-generic/source/odp_packet_socket.c >> +++ b/platform/linux-generic/source/odp_packet_socket.c >> @@ -224,7 +224,11 @@ int recv_pkt_sock(pkt_sock_t *const pkt_sock, >> continue; >> >> /* Parse and set packet header data */ >> - odp_packet_parse(pkt, recv_bytes, >> pkt_sock->frame_offset); >> > > Packet parse just fills in metadata. If it don't understand (some) packet > header format, it's not a failure. By default packet should not be dropped > in that case. There's just less metadata and the application (e.g. IP > stack) needs to do more work on the packet and do the drop decision. > > Same applies to the other (netmap and mmap versions). > > -Petri > > >> + if (!odp_packet_parse(pkt, recv_bytes, >> pkt_sock->frame_offset)) { >> + ODP_DBG("Packet parsing failed\n"); >> + odp_packet_init(pkt); >> + continue; >> + } >> >> pkt_table[nb_rx] = pkt; >> pkt = ODP_PACKET_INVALID; >> @@ -331,8 +335,12 @@ int recv_pkt_sock(pkt_sock_t * const pkt_sock, >> } >> >> /* Parse and set packet header data */ >> - odp_packet_parse(pkt_table[i], msgvec[i].msg_len, >> - pkt_sock->frame_offset); >> + if (!odp_packet_parse(pkt_table[i], msgvec[i].msg_len, >> + pkt_sock->frame_offset)) { >> + ODP_DBG("Packet parsing failed\n"); >> + odp_packet_init(pkt); >> + continue; >> + } >> >> pkt_table[nb_rx] = pkt_table[i]; >> nb_rx++; >> @@ -489,7 +497,11 @@ static inline unsigned pkt_mmap_v2_rx(int sock, >> struct ring *ring, >> rx_user_ready(ppd.raw); >> >> /* Parse and set packet header data */ >> - odp_packet_parse(pkt_table[i], pkt_len, >> frame_offset); >> + if (!odp_packet_parse(pkt_table[i], pkt_len, >> frame_offset)) { >> + ODP_DBG("Packet parsing failed\n"); >> + odp_packet_init(pkt_table[i]); >> + continue; >> + } >> >> frame_num = next_frame_num; >> i++; >> -- >> 1.8.3.2 >> >>
When should packet_parse fail and return an error code? Possibly the original prototype with void return type is better. the call never fails, it is just the amount of parsing that is done which varies, potentially no fields at all are filled in. On 28 March 2014 10:10, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > > > > On Fri, Mar 28, 2014 at 9:09 AM, Petri Savolainen < > petri.savolainen@linaro.org> wrote: > >> Hi, >> >> On Thursday, 27 March 2014 17:38:49 UTC+2, Ciprian Barbu wrote: >> >>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>> --- >>> platform/linux-generic/include/odp_packet_internal.h | 2 +- >>> platform/linux-generic/source/odp_packet.c | 8 +++++--- >>> platform/linux-generic/source/odp_packet_netmap.c | 6 +++++- >>> platform/linux-generic/source/odp_packet_socket.c | 20 >>> ++++++++++++++++---- >>> 4 files changed, 27 insertions(+), 9 deletions(-) >>> >>> diff --git a/platform/linux-generic/include/odp_packet_internal.h >>> b/platform/linux-generic/include/odp_packet_internal.h >>> index 792fc7c..b316b20 100644 >>> --- a/platform/linux-generic/include/odp_packet_internal.h >>> +++ b/platform/linux-generic/include/odp_packet_internal.h >>> @@ -135,7 +135,7 @@ static inline odp_packet_hdr_t >>> *odp_packet_hdr(odp_packet_t pkt) >>> /** >>> * Parse packet and set internal metadata >>> */ >>> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); >>> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); >>> >>> #ifdef __cplusplus >>> } >>> diff --git a/platform/linux-generic/source/odp_packet.c >>> b/platform/linux-generic/source/odp_packet.c >>> index 5f07374..700ac39 100644 >>> --- a/platform/linux-generic/source/odp_packet.c >>> +++ b/platform/linux-generic/source/odp_packet.c >>> @@ -138,8 +138,9 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, >>> size_t offset) >>> * @param pkt Packet handle >>> * @param len Packet length in bytes >>> * @param frame_offset Byte offset to L2 header >>> + * @return 1 if the packet is valid, 0 otherwise >>> >> >> Return status should be more standard (posix style) with 0 on success and >> (negative) error code on failure. >> >> > Sure I can do that, but what I actually did was to look at > odp_buffer_is_valid. It fits better because the return status is in both > cases a boolean value. > >> */ >>> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>> frame_offset) >>> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) >>> { >>> odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); >>> odp_ethhdr_t *eth; >>> @@ -154,9 +155,9 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, >>> size_t frame_offset) >>> pkt_hdr->frame_offset = frame_offset; >>> pkt_hdr->frame_len = len; >>> >>> - if (odp_unlikely(len < ODP_ETH_LEN_MIN)) { >>> + if (odp_unlikely(len < ODP_ETHHDR_LEN)) { >>> pkt_hdr->error_flags.frame_len = 1; >>> - return; >>> + return 0; >>> >> >> return -1; >> >> >>> } else if (len > ODP_ETH_LEN_MAX) { >>> pkt_hdr->input_flags.jumbo = 1; >>> } >>> @@ -235,6 +236,7 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, >>> size_t frame_offset) >>> } >>> break; >>> } >>> + return 1; >>> >> >> return 0; >> >> >>> } >>> >>> static inline uint8_t parse_ipv4(odp_packet_hdr_t *pkt_hdr, >>> odp_ipv4hdr_t *ipv4, >>> diff --git a/platform/linux-generic/source/odp_packet_netmap.c >>> b/platform/linux-generic/source/odp_packet_netmap.c >>> index 1cbd84c..0dc109c 100644 >>> --- a/platform/linux-generic/source/odp_packet_netmap.c >>> +++ b/platform/linux-generic/source/odp_packet_netmap.c >>> @@ -308,7 +308,11 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, >>> odp_packet_t pkt_table[], >>> >>> /* Initialize, parse and set packet header data >>> */ >>> odp_packet_init(pkt); >>> - odp_packet_parse(pkt, payload_len, >>> pkt_nm->l2_offset); >>> + if (!odp_packet_parse(pkt, payload_len, >>> pkt_nm->l2_offset)) { >>> + ODP_DBG("Packet parsing failed\n"); >>> + odp_packet_init(pkt); >>> + continue; >>> + } >>> >>> pkt_table[nb_rx] = pkt; >>> pkt = ODP_PACKET_INVALID; >>> diff --git a/platform/linux-generic/source/odp_packet_socket.c >>> b/platform/linux-generic/source/odp_packet_socket.c >>> index aaf2605..65796de 100644 >>> --- a/platform/linux-generic/source/odp_packet_socket.c >>> +++ b/platform/linux-generic/source/odp_packet_socket.c >>> @@ -224,7 +224,11 @@ int recv_pkt_sock(pkt_sock_t *const pkt_sock, >>> continue; >>> >>> /* Parse and set packet header data */ >>> - odp_packet_parse(pkt, recv_bytes, >>> pkt_sock->frame_offset); >>> >> >> Packet parse just fills in metadata. If it don't understand (some) packet >> header format, it's not a failure. By default packet should not be dropped >> in that case. There's just less metadata and the application (e.g. IP >> stack) needs to do more work on the packet and do the drop decision. >> >> Same applies to the other (netmap and mmap versions). >> >> -Petri >> >> >>> + if (!odp_packet_parse(pkt, recv_bytes, >>> pkt_sock->frame_offset)) { >>> + ODP_DBG("Packet parsing failed\n"); >>> + odp_packet_init(pkt); >>> + continue; >>> + } >>> >>> pkt_table[nb_rx] = pkt; >>> pkt = ODP_PACKET_INVALID; >>> @@ -331,8 +335,12 @@ int recv_pkt_sock(pkt_sock_t * const pkt_sock, >>> } >>> >>> /* Parse and set packet header data */ >>> - odp_packet_parse(pkt_table[i], msgvec[i].msg_len, >>> - pkt_sock->frame_offset); >>> + if (!odp_packet_parse(pkt_table[i], msgvec[i].msg_len, >>> + pkt_sock->frame_offset)) { >>> + ODP_DBG("Packet parsing failed\n"); >>> + odp_packet_init(pkt); >>> + continue; >>> + } >>> >>> pkt_table[nb_rx] = pkt_table[i]; >>> nb_rx++; >>> @@ -489,7 +497,11 @@ static inline unsigned pkt_mmap_v2_rx(int sock, >>> struct ring *ring, >>> rx_user_ready(ppd.raw); >>> >>> /* Parse and set packet header data */ >>> - odp_packet_parse(pkt_table[i], pkt_len, >>> frame_offset); >>> + if (!odp_packet_parse(pkt_table[i], pkt_len, >>> frame_offset)) { >>> + ODP_DBG("Packet parsing failed\n"); >>> + odp_packet_init(pkt_table[i]); >>> + continue; >>> + } >>> >>> frame_num = next_frame_num; >>> i++; >>> -- >>> 1.8.3.2 >>> >>> > -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZgOJEasw8KBdxm6Uqu%3DDfxb4EhV0EGoE2KiaycHEeLWQ%40mail.gmail.com<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZgOJEasw8KBdxm6Uqu%3DDfxb4EhV0EGoE2KiaycHEeLWQ%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
On 2014-03-27 16:38, Ciprian Barbu wrote: > Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> > --- > platform/linux-generic/include/odp_packet_internal.h | 2 +- > platform/linux-generic/source/odp_packet.c | 8 +++++--- > platform/linux-generic/source/odp_packet_netmap.c | 6 +++++- > platform/linux-generic/source/odp_packet_socket.c | 20 ++++++++++++++++---- > 4 files changed, 27 insertions(+), 9 deletions(-) > > diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h > index 792fc7c..b316b20 100644 > --- a/platform/linux-generic/include/odp_packet_internal.h > +++ b/platform/linux-generic/include/odp_packet_internal.h > @@ -135,7 +135,7 @@ static inline odp_packet_hdr_t *odp_packet_hdr(odp_packet_t pkt) > /** > * Parse packet and set internal metadata > */ > -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); > +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); > > #ifdef __cplusplus > } > diff --git a/platform/linux-generic/source/odp_packet.c b/platform/linux-generic/source/odp_packet.c > index 5f07374..700ac39 100644 > --- a/platform/linux-generic/source/odp_packet.c > +++ b/platform/linux-generic/source/odp_packet.c > @@ -138,8 +138,9 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, size_t offset) > * @param pkt Packet handle > * @param len Packet length in bytes > * @param frame_offset Byte offset to L2 header > + * @return 1 if the packet is valid, 0 otherwise > */ > -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) > +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) > { > odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); > odp_ethhdr_t *eth; > @@ -154,9 +155,9 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) > pkt_hdr->frame_offset = frame_offset; > pkt_hdr->frame_len = len; > > - if (odp_unlikely(len < ODP_ETH_LEN_MIN)) { > + if (odp_unlikely(len < ODP_ETHHDR_LEN)) { > pkt_hdr->error_flags.frame_len = 1; > - return; > + return 0; > } else if (len > ODP_ETH_LEN_MAX) { > pkt_hdr->input_flags.jumbo = 1; > } > @@ -235,6 +236,7 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) > } > break; > } > + return 1; > } > > static inline uint8_t parse_ipv4(odp_packet_hdr_t *pkt_hdr, odp_ipv4hdr_t *ipv4, > diff --git a/platform/linux-generic/source/odp_packet_netmap.c b/platform/linux-generic/source/odp_packet_netmap.c > index 1cbd84c..0dc109c 100644 > --- a/platform/linux-generic/source/odp_packet_netmap.c > +++ b/platform/linux-generic/source/odp_packet_netmap.c > @@ -308,7 +308,11 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, odp_packet_t pkt_table[], > > /* Initialize, parse and set packet header data */ > odp_packet_init(pkt); > - odp_packet_parse(pkt, payload_len, pkt_nm->l2_offset); > + if (!odp_packet_parse(pkt, payload_len, pkt_nm->l2_offset)) { > + ODP_DBG("Packet parsing failed\n"); > + odp_packet_init(pkt); > + continue; > + } > > pkt_table[nb_rx] = pkt; > pkt = ODP_PACKET_INVALID; > diff --git a/platform/linux-generic/source/odp_packet_socket.c b/platform/linux-generic/source/odp_packet_socket.c > index aaf2605..65796de 100644 > --- a/platform/linux-generic/source/odp_packet_socket.c > +++ b/platform/linux-generic/source/odp_packet_socket.c > @@ -224,7 +224,11 @@ int recv_pkt_sock(pkt_sock_t *const pkt_sock, > continue; > > /* Parse and set packet header data */ > - odp_packet_parse(pkt, recv_bytes, pkt_sock->frame_offset); > + if (!odp_packet_parse(pkt, recv_bytes, pkt_sock->frame_offset)) { > + ODP_DBG("Packet parsing failed\n"); > + odp_packet_init(pkt); > + continue; > + } > > pkt_table[nb_rx] = pkt; > pkt = ODP_PACKET_INVALID; > @@ -331,8 +335,12 @@ int recv_pkt_sock(pkt_sock_t * const pkt_sock, > } > > /* Parse and set packet header data */ > - odp_packet_parse(pkt_table[i], msgvec[i].msg_len, > - pkt_sock->frame_offset); > + if (!odp_packet_parse(pkt_table[i], msgvec[i].msg_len, > + pkt_sock->frame_offset)) { > + ODP_DBG("Packet parsing failed\n"); > + odp_packet_init(pkt); > + continue; > + } > > pkt_table[nb_rx] = pkt_table[i]; > nb_rx++; > @@ -489,7 +497,11 @@ static inline unsigned pkt_mmap_v2_rx(int sock, struct ring *ring, > rx_user_ready(ppd.raw); > > /* Parse and set packet header data */ > - odp_packet_parse(pkt_table[i], pkt_len, frame_offset); > + if (!odp_packet_parse(pkt_table[i], pkt_len, frame_offset)) { > + ODP_DBG("Packet parsing failed\n"); > + odp_packet_init(pkt_table[i]); > + continue; > + } > > frame_num = next_frame_num; > i++; > -- > 1.8.3.2 Have you runned this patch via checkpatch.pl before you sent it? please do that! Cheers, Anders
On Fri, Mar 28, 2014 at 11:18 AM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: > When should packet_parse fail and return an error code? Possibly the > original prototype with void return type is better. the call never fails, > it is just the amount of parsing that is done which varies, potentially no > fields at all are filled in. > > Unfortunately if packet parsing fails it is also impossible to use the packet anymore, because odp_packet_l2 uses l2_offset which is unset if parsing fails. I come back with the question whether l2_offset is really necessary if we already have frame_offset. Or perhaps we should change odp_packet_l2 to use frame_offset instead of l2_offset. > > On 28 March 2014 10:10, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > >> >> >> >> On Fri, Mar 28, 2014 at 9:09 AM, Petri Savolainen < >> petri.savolainen@linaro.org> wrote: >> >>> Hi, >>> >>> On Thursday, 27 March 2014 17:38:49 UTC+2, Ciprian Barbu wrote: >>> >>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>> --- >>>> platform/linux-generic/include/odp_packet_internal.h | 2 +- >>>> platform/linux-generic/source/odp_packet.c | 8 +++++--- >>>> platform/linux-generic/source/odp_packet_netmap.c | 6 +++++- >>>> platform/linux-generic/source/odp_packet_socket.c | 20 >>>> ++++++++++++++++---- >>>> 4 files changed, 27 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/platform/linux-generic/include/odp_packet_internal.h >>>> b/platform/linux-generic/include/odp_packet_internal.h >>>> index 792fc7c..b316b20 100644 >>>> --- a/platform/linux-generic/include/odp_packet_internal.h >>>> +++ b/platform/linux-generic/include/odp_packet_internal.h >>>> @@ -135,7 +135,7 @@ static inline odp_packet_hdr_t >>>> *odp_packet_hdr(odp_packet_t pkt) >>>> /** >>>> * Parse packet and set internal metadata >>>> */ >>>> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); >>>> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); >>>> >>>> #ifdef __cplusplus >>>> } >>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>> b/platform/linux-generic/source/odp_packet.c >>>> index 5f07374..700ac39 100644 >>>> --- a/platform/linux-generic/source/odp_packet.c >>>> +++ b/platform/linux-generic/source/odp_packet.c >>>> @@ -138,8 +138,9 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, >>>> size_t offset) >>>> * @param pkt Packet handle >>>> * @param len Packet length in bytes >>>> * @param frame_offset Byte offset to L2 header >>>> + * @return 1 if the packet is valid, 0 otherwise >>>> >>> >>> Return status should be more standard (posix style) with 0 on success >>> and (negative) error code on failure. >>> >>> >> Sure I can do that, but what I actually did was to look at >> odp_buffer_is_valid. It fits better because the return status is in both >> cases a boolean value. >> >>> */ >>>> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>> frame_offset) >>>> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>> frame_offset) >>>> { >>>> odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); >>>> odp_ethhdr_t *eth; >>>> @@ -154,9 +155,9 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, >>>> size_t frame_offset) >>>> pkt_hdr->frame_offset = frame_offset; >>>> pkt_hdr->frame_len = len; >>>> >>>> - if (odp_unlikely(len < ODP_ETH_LEN_MIN)) { >>>> + if (odp_unlikely(len < ODP_ETHHDR_LEN)) { >>>> pkt_hdr->error_flags.frame_len = 1; >>>> - return; >>>> + return 0; >>>> >>> >>> return -1; >>> >>> >>>> } else if (len > ODP_ETH_LEN_MAX) { >>>> pkt_hdr->input_flags.jumbo = 1; >>>> } >>>> @@ -235,6 +236,7 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, >>>> size_t frame_offset) >>>> } >>>> break; >>>> } >>>> + return 1; >>>> >>> >>> return 0; >>> >>> >>>> } >>>> >>>> static inline uint8_t parse_ipv4(odp_packet_hdr_t *pkt_hdr, >>>> odp_ipv4hdr_t *ipv4, >>>> diff --git a/platform/linux-generic/source/odp_packet_netmap.c >>>> b/platform/linux-generic/source/odp_packet_netmap.c >>>> index 1cbd84c..0dc109c 100644 >>>> --- a/platform/linux-generic/source/odp_packet_netmap.c >>>> +++ b/platform/linux-generic/source/odp_packet_netmap.c >>>> @@ -308,7 +308,11 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, >>>> odp_packet_t pkt_table[], >>>> >>>> /* Initialize, parse and set packet header >>>> data */ >>>> odp_packet_init(pkt); >>>> - odp_packet_parse(pkt, payload_len, >>>> pkt_nm->l2_offset); >>>> + if (!odp_packet_parse(pkt, payload_len, >>>> pkt_nm->l2_offset)) { >>>> + ODP_DBG("Packet parsing failed\n"); >>>> + odp_packet_init(pkt); >>>> + continue; >>>> + } >>>> >>>> pkt_table[nb_rx] = pkt; >>>> pkt = ODP_PACKET_INVALID; >>>> diff --git a/platform/linux-generic/source/odp_packet_socket.c >>>> b/platform/linux-generic/source/odp_packet_socket.c >>>> index aaf2605..65796de 100644 >>>> --- a/platform/linux-generic/source/odp_packet_socket.c >>>> +++ b/platform/linux-generic/source/odp_packet_socket.c >>>> @@ -224,7 +224,11 @@ int recv_pkt_sock(pkt_sock_t *const pkt_sock, >>>> continue; >>>> >>>> /* Parse and set packet header data */ >>>> - odp_packet_parse(pkt, recv_bytes, >>>> pkt_sock->frame_offset); >>>> >>> >>> Packet parse just fills in metadata. If it don't understand (some) >>> packet header format, it's not a failure. By default packet should not be >>> dropped in that case. There's just less metadata and the application (e.g. >>> IP stack) needs to do more work on the packet and do the drop decision. >>> >>> Same applies to the other (netmap and mmap versions). >>> >>> -Petri >>> >>> >>>> + if (!odp_packet_parse(pkt, recv_bytes, >>>> pkt_sock->frame_offset)) { >>>> + ODP_DBG("Packet parsing failed\n"); >>>> + odp_packet_init(pkt); >>>> + continue; >>>> + } >>>> >>>> pkt_table[nb_rx] = pkt; >>>> pkt = ODP_PACKET_INVALID; >>>> @@ -331,8 +335,12 @@ int recv_pkt_sock(pkt_sock_t * const pkt_sock, >>>> } >>>> >>>> /* Parse and set packet header data */ >>>> - odp_packet_parse(pkt_table[i], msgvec[i].msg_len, >>>> - pkt_sock->frame_offset); >>>> + if (!odp_packet_parse(pkt_table[i], >>>> msgvec[i].msg_len, >>>> + pkt_sock->frame_offset)) { >>>> + ODP_DBG("Packet parsing failed\n"); >>>> + odp_packet_init(pkt); >>>> + continue; >>>> + } >>>> >>>> pkt_table[nb_rx] = pkt_table[i]; >>>> nb_rx++; >>>> @@ -489,7 +497,11 @@ static inline unsigned pkt_mmap_v2_rx(int sock, >>>> struct ring *ring, >>>> rx_user_ready(ppd.raw); >>>> >>>> /* Parse and set packet header data */ >>>> - odp_packet_parse(pkt_table[i], pkt_len, >>>> frame_offset); >>>> + if (!odp_packet_parse(pkt_table[i], pkt_len, >>>> frame_offset)) { >>>> + ODP_DBG("Packet parsing failed\n"); >>>> + odp_packet_init(pkt_table[i]); >>>> + continue; >>>> + } >>>> >>>> frame_num = next_frame_num; >>>> i++; >>>> -- >>>> 1.8.3.2 >>>> >>>> >> -- >> You received this message because you are subscribed to the Google Groups >> "LNG ODP Sub-team - lng-odp@linaro.org" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to lng-odp+unsubscribe@linaro.org. >> To post to this group, send email to lng-odp@linaro.org. >> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. >> To view this discussion on the web visit >> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZgOJEasw8KBdxm6Uqu%3DDfxb4EhV0EGoE2KiaycHEeLWQ%40mail.gmail.com<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZgOJEasw8KBdxm6Uqu%3DDfxb4EhV0EGoE2KiaycHEeLWQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >> > >
On Friday, 28 March 2014 13:08:34 UTC+2, Ciprian Barbu wrote: > > On Fri, Mar 28, 2014 at 11:18 AM, Ola Liljedahl <ola.li...@linaro.org<javascript:> > > wrote: > >> When should packet_parse fail and return an error code? Possibly the >> original prototype with void return type is better. the call never fails, >> it is just the amount of parsing that is done which varies, potentially no >> fields at all are filled in. >> >> Unfortunately if packet parsing fails it is also impossible to use the > packet anymore, because odp_packet_l2 uses l2_offset which is unset if > parsing fails. I come back with the question whether l2_offset is really > necessary if we already have frame_offset. Or perhaps we should change > odp_packet_l2 to use frame_offset instead of l2_offset. > It's not impossible, application would lack just some metadata. ODP cannot include all network protocols. Application typically uses metadata to accelerate its processing, but if some metadata is missing it can always go through the protocol headers itself. frame_offset == first byte of the frame l2_offset == first byte of the L2 header (which ODP/HW did find from the frame) Typically those are the same, but there could be some other (e.g. proprietary) header in between. -Petri > > >> >> On 28 March 2014 10:10, Ciprian Barbu <cipria...@linaro.org <javascript:> >> > wrote: >> >>> >>> >>> >>> On Fri, Mar 28, 2014 at 9:09 AM, Petri Savolainen < >>> petri.sa...@linaro.org <javascript:>> wrote: >>> >>>> Hi, >>>> >>>> On Thursday, 27 March 2014 17:38:49 UTC+2, Ciprian Barbu wrote: >>>> >>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>> --- >>>>> platform/linux-generic/include/odp_packet_internal.h | 2 +- >>>>> platform/linux-generic/source/odp_packet.c | 8 +++++--- >>>>> platform/linux-generic/source/odp_packet_netmap.c | 6 +++++- >>>>> platform/linux-generic/source/odp_packet_socket.c | 20 >>>>> ++++++++++++++++---- >>>>> 4 files changed, 27 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/platform/linux-generic/include/odp_packet_internal.h >>>>> b/platform/linux-generic/include/odp_packet_internal.h >>>>> index 792fc7c..b316b20 100644 >>>>> --- a/platform/linux-generic/include/odp_packet_internal.h >>>>> +++ b/platform/linux-generic/include/odp_packet_internal.h >>>>> @@ -135,7 +135,7 @@ static inline odp_packet_hdr_t >>>>> *odp_packet_hdr(odp_packet_t pkt) >>>>> /** >>>>> * Parse packet and set internal metadata >>>>> */ >>>>> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>>> l2_offset); >>>>> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); >>>>> >>>>> #ifdef __cplusplus >>>>> } >>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>> b/platform/linux-generic/source/odp_packet.c >>>>> index 5f07374..700ac39 100644 >>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>> @@ -138,8 +138,9 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, >>>>> size_t offset) >>>>> * @param pkt Packet handle >>>>> * @param len Packet length in bytes >>>>> * @param frame_offset Byte offset to L2 header >>>>> + * @return 1 if the packet is valid, 0 otherwise >>>>> >>>> >>>> Return status should be more standard (posix style) with 0 on success >>>> and (negative) error code on failure. >>>> >>>> >>> Sure I can do that, but what I actually did was to look at >>> odp_buffer_is_valid. It fits better because the return status is in both >>> cases a boolean value. >>> >>>> */ >>>>> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>>> frame_offset) >>>>> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>>> frame_offset) >>>>> { >>>>> odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); >>>>> odp_ethhdr_t *eth; >>>>> @@ -154,9 +155,9 @@ void odp_packet_parse(odp_packet_t pkt, size_t >>>>> len, size_t frame_offset) >>>>> pkt_hdr->frame_offset = frame_offset; >>>>> pkt_hdr->frame_len = len; >>>>> >>>>> - if (odp_unlikely(len < ODP_ETH_LEN_MIN)) { >>>>> + if (odp_unlikely(len < ODP_ETHHDR_LEN)) { >>>>> pkt_hdr->error_flags.frame_len = 1; >>>>> - return; >>>>> + return 0; >>>>> >>>> >>>> return -1; >>>> >>>> >>>>> } else if (len > ODP_ETH_LEN_MAX) { >>>>> pkt_hdr->input_flags.jumbo = 1; >>>>> } >>>>> @@ -235,6 +236,7 @@ void odp_packet_parse(odp_packet_t pkt, size_t >>>>> len, size_t frame_offset) >>>>> } >>>>> break; >>>>> } >>>>> + return 1; >>>>> >>>> >>>> return 0; >>>> >>>> >>>>> } >>>>> >>>>> static inline uint8_t parse_ipv4(odp_packet_hdr_t *pkt_hdr, >>>>> odp_ipv4hdr_t *ipv4, >>>>> diff --git a/platform/linux-generic/source/odp_packet_netmap.c >>>>> b/platform/linux-generic/source/odp_packet_netmap.c >>>>> index 1cbd84c..0dc109c 100644 >>>>> --- a/platform/linux-generic/source/odp_packet_netmap.c >>>>> +++ b/platform/linux-generic/source/odp_packet_netmap.c >>>>> @@ -308,7 +308,11 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, >>>>> odp_packet_t pkt_table[], >>>>> >>>>> /* Initialize, parse and set packet header >>>>> data */ >>>>> odp_packet_init(pkt); >>>>> - odp_packet_parse(pkt, payload_len, >>>>> pkt_nm->l2_offset); >>>>> + if (!odp_packet_parse(pkt, payload_len, >>>>> pkt_nm->l2_offset)) { >>>>> + ODP_DBG("Packet parsing failed\n"); >>>>> + odp_packet_init(pkt); >>>>> + continue; >>>>> + } >>>>> >>>>> pkt_table[nb_rx] = pkt; >>>>> pkt = ODP_PACKET_INVALID; >>>>> diff --git a/platform/linux-generic/source/odp_packet_socket.c >>>>> b/platform/linux-generic/source/odp_packet_socket.c >>>>> index aaf2605..65796de 100644 >>>>> --- a/platform/linux-generic/source/odp_packet_socket.c >>>>> +++ b/platform/linux-generic/source/odp_packet_socket.c >>>>> @@ -224,7 +224,11 @@ int recv_pkt_sock(pkt_sock_t *const pkt_sock, >>>>> continue; >>>>> >>>>> /* Parse and set packet header data */ >>>>> - odp_packet_parse(pkt, recv_bytes, >>>>> pkt_sock->frame_offset); >>>>> >>>> >>>> Packet parse just fills in metadata. If it don't understand (some) >>>> packet header format, it's not a failure. By default packet should not be >>>> dropped in that case. There's just less metadata and the application (e.g. >>>> IP stack) needs to do more work on the packet and do the drop decision. >>>> >>>> Same applies to the other (netmap and mmap versions). >>>> >>>> -Petri >>>> >>>> >>>>> + if (!odp_packet_parse(pkt, recv_bytes, >>>>> pkt_sock->frame_offset)) { >>>>> + ODP_DBG("Packet parsing failed\n"); >>>>> + odp_packet_init(pkt); >>>>> + continue; >>>>> + } >>>>> >>>>> pkt_table[nb_rx] = pkt; >>>>> pkt = ODP_PACKET_INVALID; >>>>> @@ -331,8 +335,12 @@ int recv_pkt_sock(pkt_sock_t * const pkt_sock, >>>>> } >>>>> >>>>> /* Parse and set packet header data */ >>>>> - odp_packet_parse(pkt_table[i], msgvec[i].msg_len, >>>>> - pkt_sock->frame_offset); >>>>> + if (!odp_packet_parse(pkt_table[i], >>>>> msgvec[i].msg_len, >>>>> + pkt_sock->frame_offset)) { >>>>> + ODP_DBG("Packet parsing failed\n"); >>>>> + odp_packet_init(pkt); >>>>> + continue; >>>>> + } >>>>> >>>>> pkt_table[nb_rx] = pkt_table[i]; >>>>> nb_rx++; >>>>> @@ -489,7 +497,11 @@ static inline unsigned pkt_mmap_v2_rx(int sock, >>>>> struct ring *ring, >>>>> rx_user_ready(ppd.raw); >>>>> >>>>> /* Parse and set packet header data */ >>>>> - odp_packet_parse(pkt_table[i], pkt_len, >>>>> frame_offset); >>>>> + if (!odp_packet_parse(pkt_table[i], pkt_len, >>>>> frame_offset)) { >>>>> + ODP_DBG("Packet parsing failed\n"); >>>>> + odp_packet_init(pkt_table[i]); >>>>> + continue; >>>>> + } >>>>> >>>>> frame_num = next_frame_num; >>>>> i++; >>>>> -- >>>>> 1.8.3.2 >>>>> >>>>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "LNG ODP Sub-team - lng...@linaro.org <javascript:>" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to lng-odp+u...@linaro.org <javascript:>. >>> To post to this group, send email to lng...@linaro.org <javascript:>. >>> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/ >>> . >>> To view this discussion on the web visit >>> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZgOJEasw8KBdxm6Uqu%3DDfxb4EhV0EGoE2KiaycHEeLWQ%40mail.gmail.com<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZgOJEasw8KBdxm6Uqu%3DDfxb4EhV0EGoE2KiaycHEeLWQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>> . >>> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >>> >> >> >
On Fri, Mar 28, 2014 at 1:19 PM, Petri Savolainen < petri.savolainen@linaro.org> wrote: > > > On Friday, 28 March 2014 13:08:34 UTC+2, Ciprian Barbu wrote: > >> >> On Fri, Mar 28, 2014 at 11:18 AM, Ola Liljedahl <ola.li...@linaro.org>wrote: >> >>> When should packet_parse fail and return an error code? Possibly the >>> original prototype with void return type is better. the call never fails, >>> it is just the amount of parsing that is done which varies, potentially no >>> fields at all are filled in. >>> >>> Unfortunately if packet parsing fails it is also impossible to use the >> packet anymore, because odp_packet_l2 uses l2_offset which is unset if >> parsing fails. I come back with the question whether l2_offset is really >> necessary if we already have frame_offset. Or perhaps we should change >> odp_packet_l2 to use frame_offset instead of l2_offset. >> > > It's not impossible, application would lack just some metadata. ODP cannot > include all network protocols. Application typically uses metadata to > accelerate its processing, but if some metadata is missing it can always go > through the protocol headers itself. > > Oh, I actually found what I was missing, odp_packet_start(). I was using odp_packet_l2() in odp_packet_netmap.c because I didn't see this function. > frame_offset == first byte of the frame > l2_offset == first byte of the L2 header (which ODP/HW did find from the > frame) > > Typically those are the same, but there could be some other (e.g. > proprietary) header in between. > > -Petri > > > > >> >> >>> >>> On 28 March 2014 10:10, Ciprian Barbu <cipria...@linaro.org> wrote: >>> >>>> >>>> >>>> >>>> On Fri, Mar 28, 2014 at 9:09 AM, Petri Savolainen < >>>> petri.sa...@linaro.org> wrote: >>>> >>>>> Hi, >>>>> >>>>> On Thursday, 27 March 2014 17:38:49 UTC+2, Ciprian Barbu wrote: >>>>> >>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>> --- >>>>>> platform/linux-generic/include/odp_packet_internal.h | 2 +- >>>>>> platform/linux-generic/source/odp_packet.c | 8 +++++--- >>>>>> platform/linux-generic/source/odp_packet_netmap.c | 6 +++++- >>>>>> platform/linux-generic/source/odp_packet_socket.c | 20 >>>>>> ++++++++++++++++---- >>>>>> 4 files changed, 27 insertions(+), 9 deletions(-) >>>>>> >>>>>> diff --git a/platform/linux-generic/include/odp_packet_internal.h >>>>>> b/platform/linux-generic/include/odp_packet_internal.h >>>>>> index 792fc7c..b316b20 100644 >>>>>> --- a/platform/linux-generic/include/odp_packet_internal.h >>>>>> +++ b/platform/linux-generic/include/odp_packet_internal.h >>>>>> @@ -135,7 +135,7 @@ static inline odp_packet_hdr_t >>>>>> *odp_packet_hdr(odp_packet_t pkt) >>>>>> /** >>>>>> * Parse packet and set internal metadata >>>>>> */ >>>>>> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>>>> l2_offset); >>>>>> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>>>> l2_offset); >>>>>> >>>>>> #ifdef __cplusplus >>>>>> } >>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>> index 5f07374..700ac39 100644 >>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>> @@ -138,8 +138,9 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, >>>>>> size_t offset) >>>>>> * @param pkt Packet handle >>>>>> * @param len Packet length in bytes >>>>>> * @param frame_offset Byte offset to L2 header >>>>>> + * @return 1 if the packet is valid, 0 otherwise >>>>>> >>>>> >>>>> Return status should be more standard (posix style) with 0 on success >>>>> and (negative) error code on failure. >>>>> >>>>> >>>> Sure I can do that, but what I actually did was to look at >>>> odp_buffer_is_valid. It fits better because the return status is in both >>>> cases a boolean value. >>>> >>>>> */ >>>>>> -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>>>> frame_offset) >>>>>> +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t >>>>>> frame_offset) >>>>>> { >>>>>> odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); >>>>>> odp_ethhdr_t *eth; >>>>>> @@ -154,9 +155,9 @@ void odp_packet_parse(odp_packet_t pkt, size_t >>>>>> len, size_t frame_offset) >>>>>> pkt_hdr->frame_offset = frame_offset; >>>>>> pkt_hdr->frame_len = len; >>>>>> >>>>>> - if (odp_unlikely(len < ODP_ETH_LEN_MIN)) { >>>>>> + if (odp_unlikely(len < ODP_ETHHDR_LEN)) { >>>>>> pkt_hdr->error_flags.frame_len = 1; >>>>>> - return; >>>>>> + return 0; >>>>>> >>>>> >>>>> return -1; >>>>> >>>>> >>>>>> } else if (len > ODP_ETH_LEN_MAX) { >>>>>> pkt_hdr->input_flags.jumbo = 1; >>>>>> } >>>>>> @@ -235,6 +236,7 @@ void odp_packet_parse(odp_packet_t pkt, size_t >>>>>> len, size_t frame_offset) >>>>>> } >>>>>> break; >>>>>> } >>>>>> + return 1; >>>>>> >>>>> >>>>> return 0; >>>>> >>>>> >>>>>> } >>>>>> >>>>>> static inline uint8_t parse_ipv4(odp_packet_hdr_t *pkt_hdr, >>>>>> odp_ipv4hdr_t *ipv4, >>>>>> diff --git a/platform/linux-generic/source/odp_packet_netmap.c >>>>>> b/platform/linux-generic/source/odp_packet_netmap.c >>>>>> index 1cbd84c..0dc109c 100644 >>>>>> --- a/platform/linux-generic/source/odp_packet_netmap.c >>>>>> +++ b/platform/linux-generic/source/odp_packet_netmap.c >>>>>> @@ -308,7 +308,11 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, >>>>>> odp_packet_t pkt_table[], >>>>>> >>>>>> /* Initialize, parse and set packet header >>>>>> data */ >>>>>> odp_packet_init(pkt); >>>>>> - odp_packet_parse(pkt, payload_len, >>>>>> pkt_nm->l2_offset); >>>>>> + if (!odp_packet_parse(pkt, payload_len, >>>>>> pkt_nm->l2_offset)) { >>>>>> + ODP_DBG("Packet parsing failed\n"); >>>>>> + odp_packet_init(pkt); >>>>>> + continue; >>>>>> + } >>>>>> >>>>>> pkt_table[nb_rx] = pkt; >>>>>> pkt = ODP_PACKET_INVALID; >>>>>> diff --git a/platform/linux-generic/source/odp_packet_socket.c >>>>>> b/platform/linux-generic/source/odp_packet_socket.c >>>>>> index aaf2605..65796de 100644 >>>>>> --- a/platform/linux-generic/source/odp_packet_socket.c >>>>>> +++ b/platform/linux-generic/source/odp_packet_socket.c >>>>>> @@ -224,7 +224,11 @@ int recv_pkt_sock(pkt_sock_t *const pkt_sock, >>>>>> continue; >>>>>> >>>>>> /* Parse and set packet header data */ >>>>>> - odp_packet_parse(pkt, recv_bytes, >>>>>> pkt_sock->frame_offset); >>>>>> >>>>> >>>>> Packet parse just fills in metadata. If it don't understand (some) >>>>> packet header format, it's not a failure. By default packet should not be >>>>> dropped in that case. There's just less metadata and the application (e.g. >>>>> IP stack) needs to do more work on the packet and do the drop decision. >>>>> >>>>> Same applies to the other (netmap and mmap versions). >>>>> >>>>> -Petri >>>>> >>>>> >>>>>> + if (!odp_packet_parse(pkt, recv_bytes, >>>>>> pkt_sock->frame_offset)) { >>>>>> + ODP_DBG("Packet parsing failed\n"); >>>>>> + odp_packet_init(pkt); >>>>>> + continue; >>>>>> + } >>>>>> >>>>>> pkt_table[nb_rx] = pkt; >>>>>> pkt = ODP_PACKET_INVALID; >>>>>> @@ -331,8 +335,12 @@ int recv_pkt_sock(pkt_sock_t * const pkt_sock, >>>>>> } >>>>>> >>>>>> /* Parse and set packet header data */ >>>>>> - odp_packet_parse(pkt_table[i], msgvec[i].msg_len, >>>>>> - pkt_sock->frame_offset); >>>>>> + if (!odp_packet_parse(pkt_table[i], >>>>>> msgvec[i].msg_len, >>>>>> + pkt_sock->frame_offset)) { >>>>>> + ODP_DBG("Packet parsing failed\n"); >>>>>> + odp_packet_init(pkt); >>>>>> + continue; >>>>>> + } >>>>>> >>>>>> pkt_table[nb_rx] = pkt_table[i]; >>>>>> nb_rx++; >>>>>> @@ -489,7 +497,11 @@ static inline unsigned pkt_mmap_v2_rx(int sock, >>>>>> struct ring *ring, >>>>>> rx_user_ready(ppd.raw); >>>>>> >>>>>> /* Parse and set packet header data */ >>>>>> - odp_packet_parse(pkt_table[i], pkt_len, >>>>>> frame_offset); >>>>>> + if (!odp_packet_parse(pkt_table[i], >>>>>> pkt_len, frame_offset)) { >>>>>> + ODP_DBG("Packet parsing failed\n"); >>>>>> + odp_packet_init(pkt_table[i]); >>>>>> + continue; >>>>>> + } >>>>>> >>>>>> frame_num = next_frame_num; >>>>>> i++; >>>>>> -- >>>>>> 1.8.3.2 >>>>>> >>>>>> >>>> -- >>>> You received this message because you are subscribed to the Google >>>> Groups "LNG ODP Sub-team - lng...@linaro.org" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to lng-odp+u...@linaro.org. >>>> To post to this group, send email to lng...@linaro.org. >>>> >>>> Visit this group at http://groups.google.com/a/ >>>> linaro.org/group/lng-odp/. >>>> To view this discussion on the web visit https://groups.google.com/a/ >>>> linaro.org/d/msgid/lng-odp/CADJJ_HZgOJEasw8KBdxm6Uqu% >>>> 3DDfxb4EhV0EGoE2KiaycHEeLWQ%40mail.gmail.com<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZgOJEasw8KBdxm6Uqu%3DDfxb4EhV0EGoE2KiaycHEeLWQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>> . >>>> For more options, visit https://groups.google.com/a/linaro.org/d/optout >>>> . >>>> >>> >>> >> -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/59bd646c-5c34-4690-8316-c48ab3889afc%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/59bd646c-5c34-4690-8316-c48ab3889afc%40linaro.org?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h index 792fc7c..b316b20 100644 --- a/platform/linux-generic/include/odp_packet_internal.h +++ b/platform/linux-generic/include/odp_packet_internal.h @@ -135,7 +135,7 @@ static inline odp_packet_hdr_t *odp_packet_hdr(odp_packet_t pkt) /** * Parse packet and set internal metadata */ -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t l2_offset); #ifdef __cplusplus } diff --git a/platform/linux-generic/source/odp_packet.c b/platform/linux-generic/source/odp_packet.c index 5f07374..700ac39 100644 --- a/platform/linux-generic/source/odp_packet.c +++ b/platform/linux-generic/source/odp_packet.c @@ -138,8 +138,9 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, size_t offset) * @param pkt Packet handle * @param len Packet length in bytes * @param frame_offset Byte offset to L2 header + * @return 1 if the packet is valid, 0 otherwise */ -void odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) +int odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) { odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); odp_ethhdr_t *eth; @@ -154,9 +155,9 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) pkt_hdr->frame_offset = frame_offset; pkt_hdr->frame_len = len; - if (odp_unlikely(len < ODP_ETH_LEN_MIN)) { + if (odp_unlikely(len < ODP_ETHHDR_LEN)) { pkt_hdr->error_flags.frame_len = 1; - return; + return 0; } else if (len > ODP_ETH_LEN_MAX) { pkt_hdr->input_flags.jumbo = 1; } @@ -235,6 +236,7 @@ void odp_packet_parse(odp_packet_t pkt, size_t len, size_t frame_offset) } break; } + return 1; } static inline uint8_t parse_ipv4(odp_packet_hdr_t *pkt_hdr, odp_ipv4hdr_t *ipv4, diff --git a/platform/linux-generic/source/odp_packet_netmap.c b/platform/linux-generic/source/odp_packet_netmap.c index 1cbd84c..0dc109c 100644 --- a/platform/linux-generic/source/odp_packet_netmap.c +++ b/platform/linux-generic/source/odp_packet_netmap.c @@ -308,7 +308,11 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, odp_packet_t pkt_table[], /* Initialize, parse and set packet header data */ odp_packet_init(pkt); - odp_packet_parse(pkt, payload_len, pkt_nm->l2_offset); + if (!odp_packet_parse(pkt, payload_len, pkt_nm->l2_offset)) { + ODP_DBG("Packet parsing failed\n"); + odp_packet_init(pkt); + continue; + } pkt_table[nb_rx] = pkt; pkt = ODP_PACKET_INVALID; diff --git a/platform/linux-generic/source/odp_packet_socket.c b/platform/linux-generic/source/odp_packet_socket.c index aaf2605..65796de 100644 --- a/platform/linux-generic/source/odp_packet_socket.c +++ b/platform/linux-generic/source/odp_packet_socket.c @@ -224,7 +224,11 @@ int recv_pkt_sock(pkt_sock_t *const pkt_sock, continue; /* Parse and set packet header data */ - odp_packet_parse(pkt, recv_bytes, pkt_sock->frame_offset); + if (!odp_packet_parse(pkt, recv_bytes, pkt_sock->frame_offset)) { + ODP_DBG("Packet parsing failed\n"); + odp_packet_init(pkt); + continue; + } pkt_table[nb_rx] = pkt; pkt = ODP_PACKET_INVALID; @@ -331,8 +335,12 @@ int recv_pkt_sock(pkt_sock_t * const pkt_sock, } /* Parse and set packet header data */ - odp_packet_parse(pkt_table[i], msgvec[i].msg_len, - pkt_sock->frame_offset); + if (!odp_packet_parse(pkt_table[i], msgvec[i].msg_len, + pkt_sock->frame_offset)) { + ODP_DBG("Packet parsing failed\n"); + odp_packet_init(pkt); + continue; + } pkt_table[nb_rx] = pkt_table[i]; nb_rx++; @@ -489,7 +497,11 @@ static inline unsigned pkt_mmap_v2_rx(int sock, struct ring *ring, rx_user_ready(ppd.raw); /* Parse and set packet header data */ - odp_packet_parse(pkt_table[i], pkt_len, frame_offset); + if (!odp_packet_parse(pkt_table[i], pkt_len, frame_offset)) { + ODP_DBG("Packet parsing failed\n"); + odp_packet_init(pkt_table[i]); + continue; + } frame_num = next_frame_num; i++;
Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> --- platform/linux-generic/include/odp_packet_internal.h | 2 +- platform/linux-generic/source/odp_packet.c | 8 +++++--- platform/linux-generic/source/odp_packet_netmap.c | 6 +++++- platform/linux-generic/source/odp_packet_socket.c | 20 ++++++++++++++++---- 4 files changed, 27 insertions(+), 9 deletions(-)