| Message ID | 1396005641-19127-1-git-send-email-ciprian.barbu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 28 March 2014 16:50, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> > > Here is a simpler version, no return code, just check if the minimum > eth header is present. logs before Signed-off please :)
On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: > Signed-off-by: Ciprian Barbu <cipria...@linaro.org <javascript:>> > > Here is a simpler version, no return code, just check if the minimum > eth header is present. > --- > platform/linux-generic/source/odp_packet.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/source/odp_packet.c > b/platform/linux-generic/source/odp_packet.c > index 5f07374..b990222 100644 > --- a/platform/linux-generic/source/odp_packet.c > +++ b/platform/linux-generic/source/odp_packet.c > @@ -154,7 +154,7 @@ 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)) { > We should still check against that min frame size (< ODP_ETH_LEN_MIN byte) and mark those frames, but continue parsing - just be extra careful not to overrun when it's a short frame. -Petri > pkt_hdr->error_flags.frame_len = 1; > return; > } else if (len > ODP_ETH_LEN_MAX) { > -- > 1.8.3.2 > >
On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < petri.savolainen@linaro.org> wrote: > > > On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: > >> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >> >> Here is a simpler version, no return code, just check if the minimum >> eth header is present. >> --- >> platform/linux-generic/source/odp_packet.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/source/odp_packet.c >> b/platform/linux-generic/source/odp_packet.c >> index 5f07374..b990222 100644 >> --- a/platform/linux-generic/source/odp_packet.c >> +++ b/platform/linux-generic/source/odp_packet.c >> @@ -154,7 +154,7 @@ 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)) { >> > > We should still check against that min frame size (< ODP_ETH_LEN_MIN byte) > and mark those frames, but continue parsing - just be extra careful not to > overrun when it's a short frame. > > So you actually suggest that if a packet less than ODP_ETH_LEN_MIN but greater than ODP_ETHHDR_LEN should still be marked as frame_len error? In my case I get ICMP packets coming from the network stack with no padding, they have valid headers, but are smaller than 60. Ola was suggesting to check only against the minimum header len. -Petri > > >> pkt_hdr->error_flags.frame_len = 1; >> return; >> } else if (len > ODP_ETH_LEN_MAX) { >> -- >> 1.8.3.2 >> >>
On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: > > > > On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen <petri.sa...@linaro.org<javascript:> > > wrote: > >> >> >> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >> >>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>> >>> Here is a simpler version, no return code, just check if the minimum >>> eth header is present. >>> --- >>> platform/linux-generic/source/odp_packet.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/platform/linux-generic/source/odp_packet.c >>> b/platform/linux-generic/source/odp_packet.c >>> index 5f07374..b990222 100644 >>> --- a/platform/linux-generic/source/odp_packet.c >>> +++ b/platform/linux-generic/source/odp_packet.c >>> @@ -154,7 +154,7 @@ 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)) { >>> >> >> We should still check against that min frame size (< ODP_ETH_LEN_MIN >> byte) and mark those frames, but continue parsing - just be extra careful >> not to overrun when it's a short frame. >> >> So you actually suggest that if a packet less than ODP_ETH_LEN_MIN but > greater than ODP_ETHHDR_LEN should still be marked as frame_len error? In > my case I get ICMP packets coming from the network stack with no padding, > they have valid headers, but are smaller than 60. Ola was suggesting to > check only against the minimum header len. > > In case of ethernet HW it would be a HW/link error that user would want to know about. You are emulating ethernet link layer with netmap, so may be it's better to play along and allocate always at least 64 byte buffers (no need to memset the tail of the buffer, so there's no extra overhead). -Petri
On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < petri.savolainen@linaro.org> wrote: > > > On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: > >> >> >> >> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen <petri.sa...@linaro.org >> > wrote: >> >>> >>> >>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>> >>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>> >>>> Here is a simpler version, no return code, just check if the minimum >>>> eth header is present. >>>> --- >>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>> b/platform/linux-generic/source/odp_packet.c >>>> index 5f07374..b990222 100644 >>>> --- a/platform/linux-generic/source/odp_packet.c >>>> +++ b/platform/linux-generic/source/odp_packet.c >>>> @@ -154,7 +154,7 @@ 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)) { >>>> >>> >>> We should still check against that min frame size (< ODP_ETH_LEN_MIN >>> byte) and mark those frames, but continue parsing - just be extra careful >>> not to overrun when it's a short frame. >>> >>> So you actually suggest that if a packet less than ODP_ETH_LEN_MIN but >> greater than ODP_ETHHDR_LEN should still be marked as frame_len error? In >> my case I get ICMP packets coming from the network stack with no padding, >> they have valid headers, but are smaller than 60. Ola was suggesting to >> check only against the minimum header len. >> >> > In case of ethernet HW it would be a HW/link error that user would want to > know about. You are emulating ethernet link layer with netmap, so may be > it's better to play along and allocate always at least 64 byte buffers (no > need to memset the tail of the buffer, so there's no extra overhead). > > Well that was my initial idea as well, but we still have this ongoing discussion about odp_packet_parse and there are good arguments so far. One of them was that packets could come from sources other than the physical medium. Maybe we would like to use the packet parse function for frames that we send to the wire. > -Petri > > -- > 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/4be99791-99da-445d-8fb2-5c35f5649960%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/4be99791-99da-445d-8fb2-5c35f5649960%40linaro.org?utm_medium=email&utm_source=footer> > . > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: > > > > On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen <petri.sa...@linaro.org<javascript:> > > wrote: > >> >> >> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >> >>> >>> >>> >>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>> petri.sa...@linaro.org> wrote: >>> >>>> >>>> >>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>> >>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>> >>>>> Here is a simpler version, no return code, just check if the minimum >>>>> eth header is present. >>>>> --- >>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>> b/platform/linux-generic/source/odp_packet.c >>>>> index 5f07374..b990222 100644 >>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>> @@ -154,7 +154,7 @@ 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)) { >>>>> >>>> >>>> We should still check against that min frame size (< ODP_ETH_LEN_MIN >>>> byte) and mark those frames, but continue parsing - just be extra careful >>>> not to overrun when it's a short frame. >>>> >>>> So you actually suggest that if a packet less than ODP_ETH_LEN_MIN but >>> greater than ODP_ETHHDR_LEN should still be marked as frame_len error? In >>> my case I get ICMP packets coming from the network stack with no padding, >>> they have valid headers, but are smaller than 60. Ola was suggesting to >>> check only against the minimum header len. >>> >>> >> In case of ethernet HW it would be a HW/link error that user would want >> to know about. You are emulating ethernet link layer with netmap, so may be >> it's better to play along and allocate always at least 64 byte buffers (no >> need to memset the tail of the buffer, so there's no extra overhead). >> >> Well that was my initial idea as well, but we still have this ongoing > discussion about odp_packet_parse and there are good arguments so far. One > of them was that packets could come from sources other than the physical > medium. Maybe we would like to use the packet parse function for frames > that we send to the wire. > The packet parse function imitates what SoC HW classifier does (on an ethernet interface input, _not_ output). It goes through (first levels of) packet headers and fills in metadata into the packet descriptor. The frame_len error flags tells to the application that the frame was truncated _on the wire_. The frame is fine when netmap receives it (length min 64 bytes), but netmap decides to report only payload length to the upper layer. You should give the original length to ODP (as raw sockets and real HW does). Netmap is used to connect ODP to ethernet interface, not to terminate the protocol. So, it's fine to allocate min 64 bytes, copy only the payload and report length min 64 bytes to packet_parse (into ODP). So, round up the length back up to min frame size after netmap receive. Packet_parse can be improved with not to give up so easily on truncated packets, but those are very rare (and may be dropped out by HW/Linux/netmap already). So it's not so relevant. What you are trying to do now is to open door for truly truncated frames (14-64 bytes on wire). -Petri
On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < petri.savolainen@linaro.org> wrote: > > > On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: > >> >> >> >> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen <petri.sa...@linaro.org >> > wrote: >> >>> >>> >>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>> >>>> >>>> >>>> >>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>> petri.sa...@linaro.org> wrote: >>>> >>>>> >>>>> >>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>> >>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>> >>>>>> Here is a simpler version, no return code, just check if the minimum >>>>>> eth header is present. >>>>>> --- >>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>> index 5f07374..b990222 100644 >>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>> >>>>> >>>>> We should still check against that min frame size (< ODP_ETH_LEN_MIN >>>>> byte) and mark those frames, but continue parsing - just be extra careful >>>>> not to overrun when it's a short frame. >>>>> >>>>> So you actually suggest that if a packet less than ODP_ETH_LEN_MIN but >>>> greater than ODP_ETHHDR_LEN should still be marked as frame_len error? In >>>> my case I get ICMP packets coming from the network stack with no padding, >>>> they have valid headers, but are smaller than 60. Ola was suggesting to >>>> check only against the minimum header len. >>>> >>>> >>> In case of ethernet HW it would be a HW/link error that user would want >>> to know about. You are emulating ethernet link layer with netmap, so may be >>> it's better to play along and allocate always at least 64 byte buffers (no >>> need to memset the tail of the buffer, so there's no extra overhead). >>> >>> Well that was my initial idea as well, but we still have this ongoing >> discussion about odp_packet_parse and there are good arguments so far. One >> of them was that packets could come from sources other than the physical >> medium. Maybe we would like to use the packet parse function for frames >> that we send to the wire. >> > > The packet parse function imitates what SoC HW classifier does (on an > ethernet interface input, _not_ output). It goes through (first levels of) > packet headers and fills in metadata into the packet descriptor. The > frame_len error flags tells to the application that the frame was truncated > _on the wire_. The frame is fine when netmap receives it (length min 64 > bytes), but netmap decides to report only payload length to the upper > layer. You should give the original length to ODP (as raw sockets and real > HW does). Netmap is used to connect ODP to ethernet interface, not to > terminate the protocol. So, it's fine to allocate min 64 bytes, copy only > the payload and report length min 64 bytes to packet_parse (into ODP). So, > round up the length back up to min frame size after netmap receive. > > Packet_parse can be improved with not to give up so easily on truncated > packets, but those are very rare (and may be dropped out by HW/Linux/netmap > already). So it's not so relevant. What you are trying to do now is to > open door for truly truncated frames (14-64 bytes on wire). > Ok then I have no problem leaving odp_packet_parse as it was and just round up the packet size to 60 in the netmap pkt I/O code. The odp_packet_parse function will probably not be used in SoC implementation, since there are HW blocks to do that and we can work on odp_packet_parse improvements later if deemed necessary. I would like Ola's opinion though since he made the observation about packet parsing not being robust enough. > -Petri > > > > > > -- > 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/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
*"So, it's fine to allocate min 64 bytes, copy only the payload and report length min 64 bytes to packet_parse (into ODP). So, round up the length back up to min frame size after netmap receive."* I don't get this. If the payload is less than 64 (60 without CRC) bytes, why report 64 (60) bytes? SW should be able to handle a packet starting with an Ethernet that is less than 60 bytes. On 31 March 2014 11:21, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > > > > On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < > petri.savolainen@linaro.org> wrote: > >> >> >> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >> >>> >>> >>> >>> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < >>> petri.sa...@linaro.org> wrote: >>> >>>> >>>> >>>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>>> petri.sa...@linaro.org> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>>> >>>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>>> >>>>>>> Here is a simpler version, no return code, just check if the minimum >>>>>>> eth header is present. >>>>>>> --- >>>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>>> index 5f07374..b990222 100644 >>>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>>> >>>>>> >>>>>> We should still check against that min frame size (< ODP_ETH_LEN_MIN >>>>>> byte) and mark those frames, but continue parsing - just be extra careful >>>>>> not to overrun when it's a short frame. >>>>>> >>>>>> So you actually suggest that if a packet less than ODP_ETH_LEN_MIN >>>>> but greater than ODP_ETHHDR_LEN should still be marked as frame_len error? >>>>> In my case I get ICMP packets coming from the network stack with no >>>>> padding, they have valid headers, but are smaller than 60. Ola was >>>>> suggesting to check only against the minimum header len. >>>>> >>>>> >>>> In case of ethernet HW it would be a HW/link error that user would want >>>> to know about. You are emulating ethernet link layer with netmap, so may be >>>> it's better to play along and allocate always at least 64 byte buffers (no >>>> need to memset the tail of the buffer, so there's no extra overhead). >>>> >>>> Well that was my initial idea as well, but we still have this ongoing >>> discussion about odp_packet_parse and there are good arguments so far. One >>> of them was that packets could come from sources other than the physical >>> medium. Maybe we would like to use the packet parse function for frames >>> that we send to the wire. >>> >> >> The packet parse function imitates what SoC HW classifier does (on an >> ethernet interface input, _not_ output). It goes through (first levels of) >> packet headers and fills in metadata into the packet descriptor. The >> frame_len error flags tells to the application that the frame was truncated >> _on the wire_. The frame is fine when netmap receives it (length min 64 >> bytes), but netmap decides to report only payload length to the upper >> layer. You should give the original length to ODP (as raw sockets and real >> HW does). Netmap is used to connect ODP to ethernet interface, not to >> terminate the protocol. So, it's fine to allocate min 64 bytes, copy only >> the payload and report length min 64 bytes to packet_parse (into ODP). So, >> round up the length back up to min frame size after netmap receive. >> >> Packet_parse can be improved with not to give up so easily on truncated >> packets, but those are very rare (and may be dropped out by HW/Linux/netmap >> already). So it's not so relevant. What you are trying to do now is to >> open door for truly truncated frames (14-64 bytes on wire). >> > > Ok then I have no problem leaving odp_packet_parse as it was and just > round up the packet size to 60 in the netmap pkt I/O code. The > odp_packet_parse function will probably not be used in SoC implementation, > since there are HW blocks to do that and we can work on odp_packet_parse > improvements later if deemed necessary. > > I would like Ola's opinion though since he made the observation about > packet parsing not being robust enough. > > >> -Petri >> >> >> >> >> >> -- >> 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/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?utm_medium=email&utm_source=footer> >> . >> >> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >> > >
On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: > > > *"So, it's fine to allocate min 64 bytes, copy only the payload and report > length min 64 bytes to packet_parse (into ODP). So, round up the length > back up to min frame size after netmap receive."* > I don't get this. If the payload is less than 64 (60 without CRC) bytes, > why report 64 (60) bytes? SW should be able to handle a packet starting > with an Ethernet that is less than 60 bytes. > It makes sense in my special case where I get frames coming directly from the Linux network stack (received by netmap from the SW ring) to make it look like it came directly from the wire but only if it is guarantied or at least highly probable that no NIC would deliver frames to the upper layers that are smaller than 64 (60 without FCS). This unfortunately discards the discussion about having a packet parsing function that is robust and reusable. > > > > On 31 March 2014 11:21, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > >> >> >> >> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < >> petri.savolainen@linaro.org> wrote: >> >>> >>> >>> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >>> >>>> >>>> >>>> >>>> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < >>>> petri.sa...@linaro.org> wrote: >>>> >>>>> >>>>> >>>>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>>>> petri.sa...@linaro.org> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>>>> >>>>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>>>> >>>>>>>> Here is a simpler version, no return code, just check if the >>>>>>>> minimum >>>>>>>> eth header is present. >>>>>>>> --- >>>>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>>>> index 5f07374..b990222 100644 >>>>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>>>> >>>>>>> >>>>>>> We should still check against that min frame size (< ODP_ETH_LEN_MIN >>>>>>> byte) and mark those frames, but continue parsing - just be extra careful >>>>>>> not to overrun when it's a short frame. >>>>>>> >>>>>>> So you actually suggest that if a packet less than ODP_ETH_LEN_MIN >>>>>> but greater than ODP_ETHHDR_LEN should still be marked as frame_len error? >>>>>> In my case I get ICMP packets coming from the network stack with no >>>>>> padding, they have valid headers, but are smaller than 60. Ola was >>>>>> suggesting to check only against the minimum header len. >>>>>> >>>>>> >>>>> In case of ethernet HW it would be a HW/link error that user would >>>>> want to know about. You are emulating ethernet link layer with netmap, so >>>>> may be it's better to play along and allocate always at least 64 byte >>>>> buffers (no need to memset the tail of the buffer, so there's no extra >>>>> overhead). >>>>> >>>>> Well that was my initial idea as well, but we still have this ongoing >>>> discussion about odp_packet_parse and there are good arguments so far. One >>>> of them was that packets could come from sources other than the physical >>>> medium. Maybe we would like to use the packet parse function for frames >>>> that we send to the wire. >>>> >>> >>> The packet parse function imitates what SoC HW classifier does (on an >>> ethernet interface input, _not_ output). It goes through (first levels of) >>> packet headers and fills in metadata into the packet descriptor. The >>> frame_len error flags tells to the application that the frame was truncated >>> _on the wire_. The frame is fine when netmap receives it (length min 64 >>> bytes), but netmap decides to report only payload length to the upper >>> layer. You should give the original length to ODP (as raw sockets and real >>> HW does). Netmap is used to connect ODP to ethernet interface, not to >>> terminate the protocol. So, it's fine to allocate min 64 bytes, copy only >>> the payload and report length min 64 bytes to packet_parse (into ODP). So, >>> round up the length back up to min frame size after netmap receive. >>> >>> Packet_parse can be improved with not to give up so easily on truncated >>> packets, but those are very rare (and may be dropped out by HW/Linux/netmap >>> already). So it's not so relevant. What you are trying to do now is to >>> open door for truly truncated frames (14-64 bytes on wire). >>> >> >> Ok then I have no problem leaving odp_packet_parse as it was and just >> round up the packet size to 60 in the netmap pkt I/O code. The >> odp_packet_parse function will probably not be used in SoC implementation, >> since there are HW blocks to do that and we can work on odp_packet_parse >> improvements later if deemed necessary. >> >> I would like Ola's opinion though since he made the observation about >> packet parsing not being robust enough. >> >> >>> -Petri >>> >>> >>> >>> >>> >>> -- >>> 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/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?utm_medium=email&utm_source=footer> >>> . >>> >>> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >>> >> >> >
On 03/31/2014 05:59 PM, Ciprian Barbu wrote: > > > > On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl > <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>> wrote: > > /"So, it's fine to allocate min 64 bytes, copy only the payload > and report length min 64 bytes to packet_parse (into ODP). So, > round up the length back up to min frame size after netmap receive." > > / > I don't get this. If the payload is less than 64 (60 without CRC) > bytes, why report 64 (60) bytes? SW should be able to handle a > packet starting with an Ethernet that is less than 60 bytes. > > > It makes sense in my special case where I get frames coming directly > from the Linux network stack (received by netmap from the SW ring) to > make it look like it came directly from the wire but only if it is > guarantied or at least highly probable that no NIC would deliver > frames to the upper layers that are smaller than 64 (60 without FCS). > This unfortunately discards the discussion about having a packet > parsing function that is robust and reusable. Now I'm a little bit lost here... Why do we need packets from Linux network stack to ODP? ODP has to work directly with network card. Also for example socket interface drops all alien packets (with not host mac addr): ./platform/linux-generic/source/odp_packet_socket.c /* frame not explicitly for us, reuse pkt buf for next frame */ if (odp_unlikely(sll.sll_pkttype != PACKET_HOST)) continue; So the question is - for what reason do we need that kind of packets recieved to odp? Best regards, Maxim. > / > / > > > On 31 March 2014 11:21, Ciprian Barbu <ciprian.barbu@linaro.org > <mailto:ciprian.barbu@linaro.org>> wrote: > > > > > On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen > <petri.savolainen@linaro.org > <mailto:petri.savolainen@linaro.org>> wrote: > > > > On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: > > > > > On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen > <petri.sa...@linaro.org> wrote: > > > > On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian > Barbu wrote: > > > > > On Fri, Mar 28, 2014 at 1:32 PM, Petri > Savolainen <petri.sa...@linaro.org> wrote: > > > > On Friday, 28 March 2014 13:20:40 UTC+2, > Ciprian Barbu wrote: > > Signed-off-by: Ciprian Barbu > <cipria...@linaro.org> > > Here is a simpler version, no return > code, just check if the minimum > eth header is present. > --- > platform/linux-generic/source/odp_packet.c > | 2 +- > 1 file changed, 1 insertion(+), 1 > deletion(-) > > diff --git > a/platform/linux-generic/source/odp_packet.c > b/platform/linux-generic/source/odp_packet.c > > index 5f07374..b990222 100644 > --- > a/platform/linux-generic/source/odp_packet.c > > +++ > b/platform/linux-generic/source/odp_packet.c > > @@ -154,7 +154,7 @@ 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)) { > > > We should still check against that min > frame size (< ODP_ETH_LEN_MIN byte) and > mark those frames, but continue parsing - > just be extra careful not to overrun when > it's a short frame. > > So you actually suggest that if a packet less > than ODP_ETH_LEN_MIN but greater than > ODP_ETHHDR_LEN should still be marked as > frame_len error? In my case I get ICMP packets > coming from the network stack with no padding, > they have valid headers, but are smaller than > 60. Ola was suggesting to check only against > the minimum header len. > > In case of ethernet HW it would be a HW/link error > that user would want to know about. You are > emulating ethernet link layer with netmap, so may > be it's better to play along and allocate always > at least 64 byte buffers (no need to memset the > tail of the buffer, so there's no extra overhead). > > Well that was my initial idea as well, but we still > have this ongoing discussion about odp_packet_parse > and there are good arguments so far. One of them was > that packets could come from sources other than the > physical medium. Maybe we would like to use the packet > parse function for frames that we send to the wire. > > > The packet parse function imitates what SoC HW classifier > does (on an ethernet interface input, _not_ output). It > goes through (first levels of) packet headers and fills in > metadata into the packet descriptor. The frame_len error > flags tells to the application that the frame was > truncated _on the wire_. The frame is fine when netmap > receives it (length min 64 bytes), but netmap decides to > report only payload length to the upper layer. You should > give the original length to ODP (as raw sockets and real > HW does). Netmap is used to connect ODP to ethernet > interface, not to terminate the protocol. So, it's fine to > allocate min 64 bytes, copy only the payload and report > length min 64 bytes to packet_parse (into ODP). So, round > up the length back up to min frame size after netmap receive. > > Packet_parse can be improved with not to give up so easily > on truncated packets, but those are very rare (and may be > dropped out by HW/Linux/netmap already). So it's not so > relevant. What you are trying to do now is to open door > for truly truncated frames (14-64 bytes on wire). > > > Ok then I have no problem leaving odp_packet_parse as it was > and just round up the packet size to 60 in the netmap pkt I/O > code. The odp_packet_parse function will probably not be used > in SoC implementation, since there are HW blocks to do that > and we can work on odp_packet_parse improvements later if > deemed necessary. > > I would like Ola's opinion though since he made the > observation about packet parsing not being robust enough. > > > -Petri > > > > > > -- > You received this message because you are subscribed to > the Google Groups "LNG ODP Sub-team - lng-odp@linaro.org > <mailto: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 > <mailto:lng-odp+unsubscribe@linaro.org>. > To post to this group, send email to lng-odp@linaro.org > <mailto: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/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org > <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?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 > <mailto:lng-odp+unsubscribe@linaro.org>. > To post to this group, send email to lng-odp@linaro.org > <mailto: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_HZPMdwpqB%3DrHBBFqDMU_Rdc%3Dg9ngbT7auJmHb%2BRBQwqXQ%40mail.gmail.com > <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZPMdwpqB%3DrHBBFqDMU_Rdc%3Dg9ngbT7auJmHb%2BRBQwqXQ%40mail.gmail.com?utm_medium=email&utm_source=footer>. > For more options, visit https://groups.google.com/a/linaro.org/d/optout.
On Mon, Mar 31, 2014 at 6:19 PM, Maxim Uvarov <maxim.uvarov@linaro.org>wrote: > On 03/31/2014 05:59 PM, Ciprian Barbu wrote: > >> >> >> >> On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl <ola.liljedahl@linaro.org<mailto: >> ola.liljedahl@linaro.org>> wrote: >> >> /"So, it's fine to allocate min 64 bytes, copy only the payload >> >> and report length min 64 bytes to packet_parse (into ODP). So, >> round up the length back up to min frame size after netmap receive." >> >> / >> >> I don't get this. If the payload is less than 64 (60 without CRC) >> bytes, why report 64 (60) bytes? SW should be able to handle a >> packet starting with an Ethernet that is less than 60 bytes. >> >> >> It makes sense in my special case where I get frames coming directly from >> the Linux network stack (received by netmap from the SW ring) to make it >> look like it came directly from the wire but only if it is guarantied or at >> least highly probable that no NIC would deliver frames to the upper layers >> that are smaller than 64 (60 without FCS). This unfortunately discards the >> discussion about having a packet parsing function that is robust and >> reusable. >> > > Now I'm a little bit lost here... Why do we need packets from Linux > network stack to ODP? ODP has to work directly with network card. > Also for example socket interface drops all alien packets (with not host > mac addr): > The netmap packet I/O example demonstrates how to use this feature of netmap, and bridges the stack and network card in order to keep the stack working. It's been like this since the beginning and we talked about it. It also helps in demonstrating ODP accelerated libpcap, although in the demo we haven't used it. But the way I see it is as an example of how to use this feature of netmap together with ODP. We could avoid the problem by simply removing this feature, although I don't prefer it. > > ./platform/linux-generic/source/odp_packet_socket.c > /* frame not explicitly for us, reuse pkt buf for next > frame */ > if (odp_unlikely(sll.sll_pkttype != PACKET_HOST)) > continue; > > So the question is - for what reason do we need that kind of packets > recieved to odp? > > Best regards, > Maxim. > > / >> >> / >> >> >> On 31 March 2014 11:21, Ciprian Barbu <ciprian.barbu@linaro.org >> <mailto:ciprian.barbu@linaro.org>> wrote: >> >> >> >> >> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen >> <petri.savolainen@linaro.org >> <mailto:petri.savolainen@linaro.org>> wrote: >> >> >> >> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >> >> >> >> >> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen >> <petri.sa...@linaro.org> wrote: >> >> >> >> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian >> Barbu wrote: >> >> >> >> >> On Fri, Mar 28, 2014 at 1:32 PM, Petri >> Savolainen <petri.sa...@linaro.org> wrote: >> >> >> >> On Friday, 28 March 2014 13:20:40 UTC+2, >> Ciprian Barbu wrote: >> >> Signed-off-by: Ciprian Barbu >> <cipria...@linaro.org> >> >> Here is a simpler version, no return >> code, just check if the minimum >> eth header is present. >> --- >> platform/linux-generic/source/ >> odp_packet.c >> | 2 +- >> 1 file changed, 1 insertion(+), 1 >> deletion(-) >> >> diff --git >> a/platform/linux-generic/ >> source/odp_packet.c >> b/platform/linux-generic/ >> source/odp_packet.c >> >> index 5f07374..b990222 100644 >> --- >> a/platform/linux-generic/ >> source/odp_packet.c >> >> +++ >> b/platform/linux-generic/ >> source/odp_packet.c >> >> @@ -154,7 +154,7 @@ 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)) { >> >> >> We should still check against that min >> frame size (< ODP_ETH_LEN_MIN byte) and >> mark those frames, but continue parsing - >> just be extra careful not to overrun when >> it's a short frame. >> >> So you actually suggest that if a packet less >> than ODP_ETH_LEN_MIN but greater than >> ODP_ETHHDR_LEN should still be marked as >> frame_len error? In my case I get ICMP packets >> coming from the network stack with no padding, >> they have valid headers, but are smaller than >> 60. Ola was suggesting to check only against >> the minimum header len. >> >> In case of ethernet HW it would be a HW/link error >> that user would want to know about. You are >> emulating ethernet link layer with netmap, so may >> be it's better to play along and allocate always >> at least 64 byte buffers (no need to memset the >> tail of the buffer, so there's no extra overhead). >> >> Well that was my initial idea as well, but we still >> have this ongoing discussion about odp_packet_parse >> and there are good arguments so far. One of them was >> that packets could come from sources other than the >> physical medium. Maybe we would like to use the packet >> parse function for frames that we send to the wire. >> >> >> The packet parse function imitates what SoC HW classifier >> does (on an ethernet interface input, _not_ output). It >> goes through (first levels of) packet headers and fills in >> metadata into the packet descriptor. The frame_len error >> flags tells to the application that the frame was >> truncated _on the wire_. The frame is fine when netmap >> receives it (length min 64 bytes), but netmap decides to >> report only payload length to the upper layer. You should >> give the original length to ODP (as raw sockets and real >> HW does). Netmap is used to connect ODP to ethernet >> interface, not to terminate the protocol. So, it's fine to >> allocate min 64 bytes, copy only the payload and report >> length min 64 bytes to packet_parse (into ODP). So, round >> up the length back up to min frame size after netmap receive. >> >> Packet_parse can be improved with not to give up so easily >> on truncated packets, but those are very rare (and may be >> dropped out by HW/Linux/netmap already). So it's not so >> relevant. What you are trying to do now is to open door >> for truly truncated frames (14-64 bytes on wire). >> >> >> Ok then I have no problem leaving odp_packet_parse as it was >> and just round up the packet size to 60 in the netmap pkt I/O >> code. The odp_packet_parse function will probably not be used >> in SoC implementation, since there are HW blocks to do that >> and we can work on odp_packet_parse improvements later if >> deemed necessary. >> >> I would like Ola's opinion though since he made the >> observation about packet parsing not being robust enough. >> >> >> -Petri >> >> >> >> >> >> -- You received this message because you are >> subscribed to >> the Google Groups "LNG ODP Sub-team - lng-odp@linaro.org >> <mailto: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 >> <mailto:lng-odp+unsubscribe@linaro.org>. >> >> To post to this group, send email to lng-odp@linaro.org >> <mailto: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/ >> 77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org >> <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/ >> 77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?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 <mailto:lng-odp+unsubscribe@ >> linaro.org>. >> To post to this group, send email to lng-odp@linaro.org <mailto: >> 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_HZPMdwpqB%3DrHBBFqDMU_ >> Rdc%3Dg9ngbT7auJmHb%2BRBQwqXQ%40mail.gmail.com < >> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/ >> CADJJ_HZPMdwpqB%3DrHBBFqDMU_Rdc%3Dg9ngbT7auJmHb%2BRBQwqXQ% >> 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/5339877B.6050303%40linaro.org. > > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
On 03/31/2014 07:34 PM, Ciprian Barbu wrote: > > > > On Mon, Mar 31, 2014 at 6:19 PM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 03/31/2014 05:59 PM, Ciprian Barbu wrote: > > > > > On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl > <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org> > <mailto:ola.liljedahl@linaro.org > <mailto:ola.liljedahl@linaro.org>>> wrote: > > /"So, it's fine to allocate min 64 bytes, copy only the > payload > > and report length min 64 bytes to packet_parse (into ODP). So, > round up the length back up to min frame size after netmap > receive." > > / > > I don't get this. If the payload is less than 64 (60 > without CRC) > bytes, why report 64 (60) bytes? SW should be able to handle a > packet starting with an Ethernet that is less than 60 bytes. > > > It makes sense in my special case where I get frames coming > directly from the Linux network stack (received by netmap from > the SW ring) to make it look like it came directly from the > wire but only if it is guarantied or at least highly probable > that no NIC would deliver frames to the upper layers that are > smaller than 64 (60 without FCS). This unfortunately discards > the discussion about having a packet parsing function that is > robust and reusable. > > > Now I'm a little bit lost here... Why do we need packets from > Linux network stack to ODP? ODP has to work directly with network > card. > Also for example socket interface drops all alien packets (with > not host mac addr): > > > The netmap packet I/O example demonstrates how to use this feature of > netmap, and bridges the stack and network card in order to keep the > stack working. It's been like this since the beginning and we talked > about it. It also helps in demonstrating ODP accelerated libpcap, > although in the demo we haven't used it. But the way I see it is as an > example of how to use this feature of netmap together with ODP. We > could avoid the problem by simply removing this feature, although I > don't prefer it. Ah, ok remember it :) then maybe add alight here: /* For now copy the data in the mbuf, worry about zero-copy later */ memcpy(l2_hdr, p, payload_len); if (payload_len < 64) payload_len = 64; ??? /* Initialize, parse and set packet header data */ odp_packet_init(pkt); odp_packet_parse(pkt, payload_len, pkt_nm->l2_offset); So that small packets will be recieved with leading zeros. The same as NIC does? Maxim. > > ./platform/linux-generic/source/odp_packet_socket.c > /* frame not explicitly for us, reuse pkt buf for > next frame */ > if (odp_unlikely(sll.sll_pkttype != PACKET_HOST)) > continue; > > So the question is - for what reason do we need that kind of > packets recieved to odp? > > Best regards, > Maxim. > > / > > / > > > On 31 March 2014 11:21, Ciprian Barbu > <ciprian.barbu@linaro.org <mailto:ciprian.barbu@linaro.org> > <mailto:ciprian.barbu@linaro.org > <mailto:ciprian.barbu@linaro.org>>> wrote: > > > > > On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen > <petri.savolainen@linaro.org > <mailto:petri.savolainen@linaro.org> > <mailto:petri.savolainen@linaro.org > <mailto:petri.savolainen@linaro.org>>> wrote: > > > > On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian > Barbu wrote: > > > > > On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen > <petri.sa...@linaro.org > <mailto:petri.sa...@linaro.org>> wrote: > > > > On Friday, 28 March 2014 16:01:14 UTC+2, > Ciprian > Barbu wrote: > > > > > On Fri, Mar 28, 2014 at 1:32 PM, Petri > Savolainen <petri.sa...@linaro.org > <mailto:petri.sa...@linaro.org>> wrote: > > > > On Friday, 28 March 2014 13:20:40 > UTC+2, > Ciprian Barbu wrote: > > Signed-off-by: Ciprian Barbu > <cipria...@linaro.org > <mailto:cipria...@linaro.org>> > > Here is a simpler version, no > return > code, just check if the minimum > eth header is present. > --- > platform/linux-generic/source/odp_packet.c > | 2 +- > 1 file changed, 1 insertion(+), 1 > deletion(-) > > diff --git > a/platform/linux-generic/source/odp_packet.c > b/platform/linux-generic/source/odp_packet.c > > index 5f07374..b990222 100644 > --- > a/platform/linux-generic/source/odp_packet.c > > +++ > b/platform/linux-generic/source/odp_packet.c > > @@ -154,7 +154,7 @@ 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)) { > > > We should still check against that min > frame size (< ODP_ETH_LEN_MIN > byte) and > mark those frames, but continue > parsing - > just be extra careful not to > overrun when > it's a short frame. > > So you actually suggest that if a > packet less > than ODP_ETH_LEN_MIN but greater than > ODP_ETHHDR_LEN should still be marked as > frame_len error? In my case I get ICMP > packets > coming from the network stack with no > padding, > they have valid headers, but are > smaller than > 60. Ola was suggesting to check only > against > the minimum header len. > > In case of ethernet HW it would be a > HW/link error > that user would want to know about. You are > emulating ethernet link layer with netmap, > so may > be it's better to play along and allocate > always > at least 64 byte buffers (no need to > memset the > tail of the buffer, so there's no extra > overhead). > > Well that was my initial idea as well, but we > still > have this ongoing discussion about > odp_packet_parse > and there are good arguments so far. One of > them was > that packets could come from sources other > than the > physical medium. Maybe we would like to use > the packet > parse function for frames that we send to the > wire. > > > The packet parse function imitates what SoC HW > classifier > does (on an ethernet interface input, _not_ > output). It > goes through (first levels of) packet headers and > fills in > metadata into the packet descriptor. The frame_len > error > flags tells to the application that the frame was > truncated _on the wire_. The frame is fine when netmap > receives it (length min 64 bytes), but netmap > decides to > report only payload length to the upper layer. You > should > give the original length to ODP (as raw sockets > and real > HW does). Netmap is used to connect ODP to ethernet > interface, not to terminate the protocol. So, it's > fine to > allocate min 64 bytes, copy only the payload and > report > length min 64 bytes to packet_parse (into ODP). > So, round > up the length back up to min frame size after > netmap receive. > > Packet_parse can be improved with not to give up > so easily > on truncated packets, but those are very rare (and > may be > dropped out by HW/Linux/netmap already). So it's > not so > relevant. What you are trying to do now is to open > door > for truly truncated frames (14-64 bytes on wire). > > > Ok then I have no problem leaving odp_packet_parse as > it was > and just round up the packet size to 60 in the netmap > pkt I/O > code. The odp_packet_parse function will probably not > be used > in SoC implementation, since there are HW blocks to do > that > and we can work on odp_packet_parse improvements later if > deemed necessary. > > I would like Ola's opinion though since he made the > observation about packet parsing not being robust enough. > > > -Petri > > > > > > -- You received this message because > you are subscribed to > the Google Groups "LNG ODP Sub-team - > lng-odp@linaro.org <mailto:lng-odp@linaro.org> > <mailto:lng-odp@linaro.org > <mailto: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 > <mailto:lng-odp%2Bunsubscribe@linaro.org> > <mailto:lng-odp+unsubscribe@linaro.org > <mailto:lng-odp%2Bunsubscribe@linaro.org>>. > > To post to this group, send email to > lng-odp@linaro.org <mailto:lng-odp@linaro.org> > <mailto:lng-odp@linaro.org > <mailto: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/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org > > <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?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 > <mailto: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 > <mailto:lng-odp%2Bunsubscribe@linaro.org> > <mailto:lng-odp+unsubscribe@linaro.org > <mailto:lng-odp%2Bunsubscribe@linaro.org>>. > To post to this group, send email to lng-odp@linaro.org > <mailto:lng-odp@linaro.org> <mailto:lng-odp@linaro.org > <mailto: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_HZPMdwpqB%3DrHBBFqDMU_Rdc%3Dg9ngbT7auJmHb%2BRBQwqXQ%40mail.gmail.com > <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZPMdwpqB%3DrHBBFqDMU_Rdc%3Dg9ngbT7auJmHb%2BRBQwqXQ%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 > <mailto: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 > <mailto:lng-odp%2Bunsubscribe@linaro.org>. > To post to this group, send email to lng-odp@linaro.org > <mailto: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/5339877B.6050303%40linaro.org. > > > For more options, visit > https://groups.google.com/a/linaro.org/d/optout. > >
On 03/31/2014 07:55 PM, Maxim Uvarov wrote: > On 03/31/2014 07:34 PM, Ciprian Barbu wrote: >> >> >> >> On Mon, Mar 31, 2014 at 6:19 PM, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: >> >> On 03/31/2014 05:59 PM, Ciprian Barbu wrote: >> >> >> >> >> On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl >> <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org> >> <mailto:ola.liljedahl@linaro.org >> <mailto:ola.liljedahl@linaro.org>>> wrote: >> >> /"So, it's fine to allocate min 64 bytes, copy only the >> payload >> >> and report length min 64 bytes to packet_parse (into >> ODP). So, >> round up the length back up to min frame size after netmap >> receive." >> >> / >> >> I don't get this. If the payload is less than 64 (60 >> without CRC) >> bytes, why report 64 (60) bytes? SW should be able to >> handle a >> packet starting with an Ethernet that is less than 60 bytes. >> >> >> It makes sense in my special case where I get frames coming >> directly from the Linux network stack (received by netmap from >> the SW ring) to make it look like it came directly from the >> wire but only if it is guarantied or at least highly probable >> that no NIC would deliver frames to the upper layers that are >> smaller than 64 (60 without FCS). This unfortunately discards >> the discussion about having a packet parsing function that is >> robust and reusable. >> >> >> Now I'm a little bit lost here... Why do we need packets from >> Linux network stack to ODP? ODP has to work directly with network >> card. >> Also for example socket interface drops all alien packets (with >> not host mac addr): >> >> >> The netmap packet I/O example demonstrates how to use this feature of >> netmap, and bridges the stack and network card in order to keep the >> stack working. It's been like this since the beginning and we talked >> about it. It also helps in demonstrating ODP accelerated libpcap, >> although in the demo we haven't used it. But the way I see it is as >> an example of how to use this feature of netmap together with ODP. We >> could avoid the problem by simply removing this feature, although I >> don't prefer it. > > Ah, ok remember it :) > > then maybe add alight here: alignment > > /* For now copy the data in the mbuf, > worry about zero-copy later */ > memcpy(l2_hdr, p, payload_len); > > if (payload_len < 64) payload_len = 64; ??? > > /* Initialize, parse and set packet header data */ > odp_packet_init(pkt); > odp_packet_parse(pkt, payload_len, pkt_nm->l2_offset); > > So that small packets will be recieved with leading zeros. The same as > NIC does? > > Maxim. > >> >> ./platform/linux-generic/source/odp_packet_socket.c >> /* frame not explicitly for us, reuse pkt buf for >> next frame */ >> if (odp_unlikely(sll.sll_pkttype != PACKET_HOST)) >> continue; >> >> So the question is - for what reason do we need that kind of >> packets recieved to odp? >> >> Best regards, >> Maxim. >> >> / >> >> / >> >> >> On 31 March 2014 11:21, Ciprian Barbu >> <ciprian.barbu@linaro.org <mailto:ciprian.barbu@linaro.org> >> <mailto:ciprian.barbu@linaro.org >> <mailto:ciprian.barbu@linaro.org>>> wrote: >> >> >> >> >> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen >> <petri.savolainen@linaro.org >> <mailto:petri.savolainen@linaro.org> >> <mailto:petri.savolainen@linaro.org >> <mailto:petri.savolainen@linaro.org>>> wrote: >> >> >> >> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian >> Barbu wrote: >> >> >> >> >> On Fri, Mar 28, 2014 at 4:18 PM, Petri >> Savolainen >> <petri.sa...@linaro.org >> <mailto:petri.sa...@linaro.org>> wrote: >> >> >> >> On Friday, 28 March 2014 16:01:14 UTC+2, >> Ciprian >> Barbu wrote: >> >> >> >> >> On Fri, Mar 28, 2014 at 1:32 PM, Petri >> Savolainen <petri.sa...@linaro.org >> <mailto:petri.sa...@linaro.org>> wrote: >> >> >> >> On Friday, 28 March 2014 13:20:40 >> UTC+2, >> Ciprian Barbu wrote: >> >> Signed-off-by: Ciprian Barbu >> <cipria...@linaro.org >> <mailto:cipria...@linaro.org>> >> >> Here is a simpler version, no >> return >> code, just check if the minimum >> eth header is present. >> --- >> platform/linux-generic/source/odp_packet.c >> | 2 +- >> 1 file changed, 1 >> insertion(+), 1 >> deletion(-) >> >> diff --git >> a/platform/linux-generic/source/odp_packet.c >> b/platform/linux-generic/source/odp_packet.c >> >> index 5f07374..b990222 100644 >> --- >> a/platform/linux-generic/source/odp_packet.c >> >> +++ >> b/platform/linux-generic/source/odp_packet.c >> >> @@ -154,7 +154,7 @@ 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)) { >> >> >> We should still check against >> that min >> frame size (< ODP_ETH_LEN_MIN >> byte) and >> mark those frames, but continue >> parsing - >> just be extra careful not to >> overrun when >> it's a short frame. >> >> So you actually suggest that if a >> packet less >> than ODP_ETH_LEN_MIN but greater than >> ODP_ETHHDR_LEN should still be marked as >> frame_len error? In my case I get ICMP >> packets >> coming from the network stack with no >> padding, >> they have valid headers, but are >> smaller than >> 60. Ola was suggesting to check only >> against >> the minimum header len. >> >> In case of ethernet HW it would be a >> HW/link error >> that user would want to know about. You are >> emulating ethernet link layer with netmap, >> so may >> be it's better to play along and allocate >> always >> at least 64 byte buffers (no need to >> memset the >> tail of the buffer, so there's no extra >> overhead). >> >> Well that was my initial idea as well, but we >> still >> have this ongoing discussion about >> odp_packet_parse >> and there are good arguments so far. One of >> them was >> that packets could come from sources other >> than the >> physical medium. Maybe we would like to use >> the packet >> parse function for frames that we send to the >> wire. >> >> >> The packet parse function imitates what SoC HW >> classifier >> does (on an ethernet interface input, _not_ >> output). It >> goes through (first levels of) packet headers and >> fills in >> metadata into the packet descriptor. The frame_len >> error >> flags tells to the application that the frame was >> truncated _on the wire_. The frame is fine when >> netmap >> receives it (length min 64 bytes), but netmap >> decides to >> report only payload length to the upper layer. You >> should >> give the original length to ODP (as raw sockets >> and real >> HW does). Netmap is used to connect ODP to ethernet >> interface, not to terminate the protocol. So, it's >> fine to >> allocate min 64 bytes, copy only the payload and >> report >> length min 64 bytes to packet_parse (into ODP). >> So, round >> up the length back up to min frame size after >> netmap receive. >> >> Packet_parse can be improved with not to give up >> so easily >> on truncated packets, but those are very rare (and >> may be >> dropped out by HW/Linux/netmap already). So it's >> not so >> relevant. What you are trying to do now is to open >> door >> for truly truncated frames (14-64 bytes on wire). >> >> >> Ok then I have no problem leaving odp_packet_parse as >> it was >> and just round up the packet size to 60 in the netmap >> pkt I/O >> code. The odp_packet_parse function will probably not >> be used >> in SoC implementation, since there are HW blocks to do >> that >> and we can work on odp_packet_parse improvements >> later if >> deemed necessary. >> >> I would like Ola's opinion though since he made the >> observation about packet parsing not being robust >> enough. >> >> >> -Petri >> >> >> >> >> >> -- You received this message because >> you are subscribed to >> the Google Groups "LNG ODP Sub-team - >> lng-odp@linaro.org <mailto:lng-odp@linaro.org> >> <mailto:lng-odp@linaro.org >> <mailto: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 >> <mailto:lng-odp%2Bunsubscribe@linaro.org> >> <mailto:lng-odp+unsubscribe@linaro.org >> <mailto:lng-odp%2Bunsubscribe@linaro.org>>. >> >> To post to this group, send email to >> lng-odp@linaro.org <mailto:lng-odp@linaro.org> >> <mailto:lng-odp@linaro.org >> <mailto: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/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org >> <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?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 >> <mailto: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 >> <mailto:lng-odp%2Bunsubscribe@linaro.org> >> <mailto:lng-odp+unsubscribe@linaro.org >> <mailto:lng-odp%2Bunsubscribe@linaro.org>>. >> To post to this group, send email to lng-odp@linaro.org >> <mailto:lng-odp@linaro.org> <mailto:lng-odp@linaro.org >> <mailto: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_HZPMdwpqB%3DrHBBFqDMU_Rdc%3Dg9ngbT7auJmHb%2BRBQwqXQ%40mail.gmail.com >> <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CADJJ_HZPMdwpqB%3DrHBBFqDMU_Rdc%3Dg9ngbT7auJmHb%2BRBQwqXQ%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 >> <mailto: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 >> <mailto:lng-odp%2Bunsubscribe@linaro.org>. >> To post to this group, send email to lng-odp@linaro.org >> <mailto: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/5339877B.6050303%40linaro.org. >> >> >> For more options, visit >> https://groups.google.com/a/linaro.org/d/optout. >> >> >
What I am trying to say is that your "upper layers" (parsing the Ethernet header) might be fed packets that do not come directly from a network interface but instead come from another SW layer which has removed encapsulation (headers) before the Ethernet packet then passing it to your layer-2 input routine. Think L2TP, MACSec (e.g. with SW decryption a la ARMv8) or EtherIP. Nothing can be said about the minimum frame/packet size here (if there ever was one relevant for the packet, that was before encapsulation was removed so now the packet is smaller and possibly "violates" that original requirement). I claim that the packet must be properly processed as long as it is at least the size of the basic Ethernet header (14 bytes). When the packet is not large enough to contain the header for the next layer, parsing stops. On 31 March 2014 15:59, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > > > > On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: > >> >> >> *"So, it's fine to allocate min 64 bytes, copy only the payload and >> report length min 64 bytes to packet_parse (into ODP). So, round up the >> length back up to min frame size after netmap receive."* >> I don't get this. If the payload is less than 64 (60 without CRC) bytes, >> why report 64 (60) bytes? SW should be able to handle a packet starting >> with an Ethernet that is less than 60 bytes. >> > > It makes sense in my special case where I get frames coming directly from > the Linux network stack (received by netmap from the SW ring) to make it > look like it came directly from the wire but only if it is guarantied or at > least highly probable that no NIC would deliver frames to the upper layers > that are smaller than 64 (60 without FCS). This unfortunately discards the > discussion about having a packet parsing function that is robust and > reusable. > > >> >> >> >> On 31 March 2014 11:21, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: >> >>> >>> >>> >>> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < >>> petri.savolainen@linaro.org> wrote: >>> >>>> >>>> >>>> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < >>>>> petri.sa...@linaro.org> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>>>>> petri.sa...@linaro.org> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>>>>> >>>>>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>>>>> >>>>>>>>> Here is a simpler version, no return code, just check if the >>>>>>>>> minimum >>>>>>>>> eth header is present. >>>>>>>>> --- >>>>>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>>>>> index 5f07374..b990222 100644 >>>>>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>>>>> >>>>>>>> >>>>>>>> We should still check against that min frame size (< >>>>>>>> ODP_ETH_LEN_MIN byte) and mark those frames, but continue parsing - just be >>>>>>>> extra careful not to overrun when it's a short frame. >>>>>>>> >>>>>>>> So you actually suggest that if a packet less than ODP_ETH_LEN_MIN >>>>>>> but greater than ODP_ETHHDR_LEN should still be marked as frame_len error? >>>>>>> In my case I get ICMP packets coming from the network stack with no >>>>>>> padding, they have valid headers, but are smaller than 60. Ola was >>>>>>> suggesting to check only against the minimum header len. >>>>>>> >>>>>>> >>>>>> In case of ethernet HW it would be a HW/link error that user would >>>>>> want to know about. You are emulating ethernet link layer with netmap, so >>>>>> may be it's better to play along and allocate always at least 64 byte >>>>>> buffers (no need to memset the tail of the buffer, so there's no extra >>>>>> overhead). >>>>>> >>>>>> Well that was my initial idea as well, but we still have this ongoing >>>>> discussion about odp_packet_parse and there are good arguments so far. One >>>>> of them was that packets could come from sources other than the physical >>>>> medium. Maybe we would like to use the packet parse function for frames >>>>> that we send to the wire. >>>>> >>>> >>>> The packet parse function imitates what SoC HW classifier does (on an >>>> ethernet interface input, _not_ output). It goes through (first levels of) >>>> packet headers and fills in metadata into the packet descriptor. The >>>> frame_len error flags tells to the application that the frame was truncated >>>> _on the wire_. The frame is fine when netmap receives it (length min 64 >>>> bytes), but netmap decides to report only payload length to the upper >>>> layer. You should give the original length to ODP (as raw sockets and real >>>> HW does). Netmap is used to connect ODP to ethernet interface, not to >>>> terminate the protocol. So, it's fine to allocate min 64 bytes, copy only >>>> the payload and report length min 64 bytes to packet_parse (into ODP). So, >>>> round up the length back up to min frame size after netmap receive. >>>> >>>> Packet_parse can be improved with not to give up so easily on truncated >>>> packets, but those are very rare (and may be dropped out by HW/Linux/netmap >>>> already). So it's not so relevant. What you are trying to do now is to >>>> open door for truly truncated frames (14-64 bytes on wire). >>>> >>> >>> Ok then I have no problem leaving odp_packet_parse as it was and just >>> round up the packet size to 60 in the netmap pkt I/O code. The >>> odp_packet_parse function will probably not be used in SoC implementation, >>> since there are HW blocks to do that and we can work on odp_packet_parse >>> improvements later if deemed necessary. >>> >>> I would like Ola's opinion though since he made the observation about >>> packet parsing not being robust enough. >>> >>> >>>> -Petri >>>> >>>> >>>> >>>> >>>> >>>> -- >>>> 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/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?utm_medium=email&utm_source=footer> >>>> . >>>> >>>> For more options, visit https://groups.google.com/a/linaro.org/d/optout >>>> . >>>> >>> >>> >> >
Hi, Ola, I think you have lost the context here. Could you down load the code and browse to odp_packet_parse(). It emulates the ethernet input HW, which does basic parsing for raw frames (before it gets e.g. passed to the (future) ODP classier or the application). In a real life, SoC HW will fill those flags and there won't be a parse function (except for filling in missing HW features, as always). So no matter how many protocols or tunnel headers there are in the frame, this function starts from first byte of the frame and raw frame length, that was passed from input HW. If you think IPC or other interfaces to pass packets to ODP that's a different story then (and a different parse function). Linux-generic have multiple options to connect a pktio interface to a Ethernet input port (raw socket, mmap socket and netmap). Since parse function works on raw frames, the length should be always >60/64. That's the case for all other input methods except netmap. If we modify parse to accept small frames from netmap, we will also accept truncated frames other sources. Real HW would not do that, without setting the frame_len error flag. -Petri On Tuesday, 1 April 2014 16:28:04 UTC+3, Ola Liljedahl wrote: > > What I am trying to say is that your "upper layers" (parsing the Ethernet > header) might be fed packets that do not come directly from a network > interface but instead come from another SW layer which has removed > encapsulation (headers) before the Ethernet packet then passing it to your > layer-2 input routine. Think L2TP, MACSec (e.g. with SW decryption a la > ARMv8) or EtherIP. Nothing can be said about the minimum frame/packet size > here (if there ever was one relevant for the packet, that was before > encapsulation was removed so now the packet is smaller and possibly > "violates" that original requirement). I claim that the packet must be > properly processed as long as it is at least the size of the basic Ethernet > header (14 bytes). When the packet is not large enough to contain the > header for the next layer, parsing stops. > > > On 31 March 2014 15:59, Ciprian Barbu <cipria...@linaro.org <javascript:>>wrote: > >> >> >> >> On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl <ola.li...@linaro.org<javascript:> >> > wrote: >> >>> >>> >>> *"So, it's fine to allocate min 64 bytes, copy only the payload and >>> report length min 64 bytes to packet_parse (into ODP). So, round up the >>> length back up to min frame size after netmap receive."* >>> I don't get this. If the payload is less than 64 (60 without CRC) bytes, >>> why report 64 (60) bytes? SW should be able to handle a packet starting >>> with an Ethernet that is less than 60 bytes. >>> >> >> It makes sense in my special case where I get frames coming directly from >> the Linux network stack (received by netmap from the SW ring) to make it >> look like it came directly from the wire but only if it is guarantied or at >> least highly probable that no NIC would deliver frames to the upper layers >> that are smaller than 64 (60 without FCS). This unfortunately discards the >> discussion about having a packet parsing function that is robust and >> reusable. >> >> >>> >>> >>> >>> On 31 March 2014 11:21, Ciprian Barbu <cipria...@linaro.org<javascript:> >>> > wrote: >>> >>>> >>>> >>>> >>>> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < >>>> petri.sa...@linaro.org <javascript:>> wrote: >>>> >>>>> >>>>> >>>>> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < >>>>>> petri.sa...@linaro.org> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>>>>>> >>>>>>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>>>>>> >>>>>>>>>> Here is a simpler version, no return code, just check if the >>>>>>>>>> minimum >>>>>>>>>> eth header is present. >>>>>>>>>> --- >>>>>>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>>>>>> index 5f07374..b990222 100644 >>>>>>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>>>>>> >>>>>>>>> >>>>>>>>> We should still check against that min frame size (< >>>>>>>>> ODP_ETH_LEN_MIN byte) and mark those frames, but continue parsing - just be >>>>>>>>> extra careful not to overrun when it's a short frame. >>>>>>>>> >>>>>>>>> So you actually suggest that if a packet less than ODP_ETH_LEN_MIN >>>>>>>> but greater than ODP_ETHHDR_LEN should still be marked as frame_len error? >>>>>>>> In my case I get ICMP packets coming from the network stack with no >>>>>>>> padding, they have valid headers, but are smaller than 60. Ola was >>>>>>>> suggesting to check only against the minimum header len. >>>>>>>> >>>>>>>> >>>>>>> In case of ethernet HW it would be a HW/link error that user would >>>>>>> want to know about. You are emulating ethernet link layer with netmap, so >>>>>>> may be it's better to play along and allocate always at least 64 byte >>>>>>> buffers (no need to memset the tail of the buffer, so there's no extra >>>>>>> overhead). >>>>>>> >>>>>>> Well that was my initial idea as well, but we still have this >>>>>> ongoing discussion about odp_packet_parse and there are good arguments so >>>>>> far. One of them was that packets could come from sources other than the >>>>>> physical medium. Maybe we would like to use the packet parse function for >>>>>> frames that we send to the wire. >>>>>> >>>>> >>>>> The packet parse function imitates what SoC HW classifier does (on an >>>>> ethernet interface input, _not_ output). It goes through (first levels of) >>>>> packet headers and fills in metadata into the packet descriptor. The >>>>> frame_len error flags tells to the application that the frame was truncated >>>>> _on the wire_. The frame is fine when netmap receives it (length min 64 >>>>> bytes), but netmap decides to report only payload length to the upper >>>>> layer. You should give the original length to ODP (as raw sockets and real >>>>> HW does). Netmap is used to connect ODP to ethernet interface, not to >>>>> terminate the protocol. So, it's fine to allocate min 64 bytes, copy only >>>>> the payload and report length min 64 bytes to packet_parse (into ODP). So, >>>>> round up the length back up to min frame size after netmap receive. >>>>> >>>>> Packet_parse can be improved with not to give up so easily on >>>>> truncated packets, but those are very rare (and may be dropped out by >>>>> HW/Linux/netmap already). So it's not so relevant. What you are trying to >>>>> do now is to open door for truly truncated frames (14-64 bytes on wire). >>>>> >>>> >>>> Ok then I have no problem leaving odp_packet_parse as it was and just >>>> round up the packet size to 60 in the netmap pkt I/O code. The >>>> odp_packet_parse function will probably not be used in SoC implementation, >>>> since there are HW blocks to do that and we can work on odp_packet_parse >>>> improvements later if deemed necessary. >>>> >>>> I would like Ola's opinion though since he made the observation about >>>> packet parsing not being robust enough. >>>> >>>> >>>>> -Petri >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> 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/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?utm_medium=email&utm_source=footer> >>>>> . >>>>> >>>>> For more options, visit >>>>> https://groups.google.com/a/linaro.org/d/optout. >>>>> >>>> >>>> >>> >> >
If this function is only used for parsing frames received from "a Netmap interface" (and not as a general Ethernet packet parser), then it might expect those frames to be minimum 60 bytes (CRC normally not included in the reported length). And we have small patch in the ODP Netmap driver to make sure all delivered packets are minimum 60 bytes. I am just averse to all attempts to artificially modify the packet size, both increase it if "it is too small" and to truncate the packet should it be "too large" (a situation I can't understand how it would come about, HW would truncate the packet and only write what will fit into the RX buffer). Understand what is the contract between different layers and avoid redundant checks and operations. Verify the contract instead using assert(). But go ahead and merge, I will have a look at the merged code instead. Maybe I will understand better. Please make sure you properly comment the purpose and function all these "abrovinks" (special fixes). On 2 April 2014 10:23, Petri Savolainen <petri.savolainen@linaro.org> wrote: > Hi, > > Ola, I think you have lost the context here. Could you down load the code > and browse to odp_packet_parse(). It emulates the ethernet input HW, which > does basic parsing for raw frames (before it gets e.g. passed to the > (future) ODP classier or the application). In a real life, SoC HW will fill > those flags and there won't be a parse function (except for filling in > missing HW features, as always). > > So no matter how many protocols or tunnel headers there are in the frame, > this function starts from first byte of the frame and raw frame length, > that was passed from input HW. If you think IPC or other interfaces to pass > packets to ODP that's a different story then (and a different parse > function). > > Linux-generic have multiple options to connect a pktio interface to a > Ethernet input port (raw socket, mmap socket and netmap). Since parse > function works on raw frames, the length should be always >60/64. That's > the case for all other input methods except netmap. If we modify parse to > accept small frames from netmap, we will also accept truncated frames other > sources. Real HW would not do that, without setting the frame_len error > flag. > > -Petri > > > On Tuesday, 1 April 2014 16:28:04 UTC+3, Ola Liljedahl wrote: >> >> What I am trying to say is that your "upper layers" (parsing the Ethernet >> header) might be fed packets that do not come directly from a network >> interface but instead come from another SW layer which has removed >> encapsulation (headers) before the Ethernet packet then passing it to your >> layer-2 input routine. Think L2TP, MACSec (e.g. with SW decryption a la >> ARMv8) or EtherIP. Nothing can be said about the minimum frame/packet size >> here (if there ever was one relevant for the packet, that was before >> encapsulation was removed so now the packet is smaller and possibly >> "violates" that original requirement). I claim that the packet must be >> properly processed as long as it is at least the size of the basic Ethernet >> header (14 bytes). When the packet is not large enough to contain the >> header for the next layer, parsing stops. >> >> >> On 31 March 2014 15:59, Ciprian Barbu <cipria...@linaro.org> wrote: >> >>> >>> >>> >>> On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl <ola.li...@linaro.org>wrote: >>> >>>> >>>> >>>> *"So, it's fine to allocate min 64 bytes, copy only the payload and >>>> report length min 64 bytes to packet_parse (into ODP). So, round up the >>>> length back up to min frame size after netmap receive."* >>>> I don't get this. If the payload is less than 64 (60 without CRC) >>>> bytes, why report 64 (60) bytes? SW should be able to handle a packet >>>> starting with an Ethernet that is less than 60 bytes. >>>> >>> >>> It makes sense in my special case where I get frames coming directly >>> from the Linux network stack (received by netmap from the SW ring) to make >>> it look like it came directly from the wire but only if it is guarantied or >>> at least highly probable that no NIC would deliver frames to the upper >>> layers that are smaller than 64 (60 without FCS). This unfortunately >>> discards the discussion about having a packet parsing function that is >>> robust and reusable. >>> >>> >>>> >>>> >>>> >>>> On 31 March 2014 11:21, Ciprian Barbu <cipria...@linaro.org> wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < >>>>> petri.sa...@linaro.org> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < >>>>>>> petri.sa...@linaro.org> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>>>>>>> >>>>>>>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>>>>>>> >>>>>>>>>>> Here is a simpler version, no return code, just check if the >>>>>>>>>>> minimum >>>>>>>>>>> eth header is present. >>>>>>>>>>> --- >>>>>>>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>>>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>>>>>>> index 5f07374..b990222 100644 >>>>>>>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>>>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>>>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> We should still check against that min frame size (< >>>>>>>>>> ODP_ETH_LEN_MIN byte) and mark those frames, but continue parsing - just be >>>>>>>>>> extra careful not to overrun when it's a short frame. >>>>>>>>>> >>>>>>>>>> So you actually suggest that if a packet less than >>>>>>>>> ODP_ETH_LEN_MIN but greater than ODP_ETHHDR_LEN should still be marked as >>>>>>>>> frame_len error? In my case I get ICMP packets coming from the network >>>>>>>>> stack with no padding, they have valid headers, but are smaller than 60. >>>>>>>>> Ola was suggesting to check only against the minimum header len. >>>>>>>>> >>>>>>>>> >>>>>>>> In case of ethernet HW it would be a HW/link error that user would >>>>>>>> want to know about. You are emulating ethernet link layer with netmap, so >>>>>>>> may be it's better to play along and allocate always at least 64 byte >>>>>>>> buffers (no need to memset the tail of the buffer, so there's no extra >>>>>>>> overhead). >>>>>>>> >>>>>>>> Well that was my initial idea as well, but we still have this >>>>>>> ongoing discussion about odp_packet_parse and there are good arguments so >>>>>>> far. One of them was that packets could come from sources other than the >>>>>>> physical medium. Maybe we would like to use the packet parse function for >>>>>>> frames that we send to the wire. >>>>>>> >>>>>> >>>>>> The packet parse function imitates what SoC HW classifier does (on an >>>>>> ethernet interface input, _not_ output). It goes through (first levels of) >>>>>> packet headers and fills in metadata into the packet descriptor. The >>>>>> frame_len error flags tells to the application that the frame was truncated >>>>>> _on the wire_. The frame is fine when netmap receives it (length min 64 >>>>>> bytes), but netmap decides to report only payload length to the upper >>>>>> layer. You should give the original length to ODP (as raw sockets and real >>>>>> HW does). Netmap is used to connect ODP to ethernet interface, not to >>>>>> terminate the protocol. So, it's fine to allocate min 64 bytes, copy only >>>>>> the payload and report length min 64 bytes to packet_parse (into ODP). So, >>>>>> round up the length back up to min frame size after netmap receive. >>>>>> >>>>>> Packet_parse can be improved with not to give up so easily on >>>>>> truncated packets, but those are very rare (and may be dropped out by >>>>>> HW/Linux/netmap already). So it's not so relevant. What you are trying to >>>>>> do now is to open door for truly truncated frames (14-64 bytes on wire). >>>>>> >>>>> >>>>> Ok then I have no problem leaving odp_packet_parse as it was and just >>>>> round up the packet size to 60 in the netmap pkt I/O code. The >>>>> odp_packet_parse function will probably not be used in SoC implementation, >>>>> since there are HW blocks to do that and we can work on odp_packet_parse >>>>> improvements later if deemed necessary. >>>>> >>>>> I would like Ola's opinion though since he made the observation about >>>>> packet parsing not being robust enough. >>>>> >>>>> >>>>>> -Petri >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> 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/77dd0fe8-653c-4ac4-8650- >>>>>> 266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?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/79269a85-8860-44c2-af98-2fc1b73fa9d7%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/79269a85-8860-44c2-af98-2fc1b73fa9d7%40linaro.org?utm_medium=email&utm_source=footer> > . > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
On Wed, Apr 2, 2014 at 12:10 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: > If this function is only used for parsing frames received from "a Netmap > interface" (and not as a general Ethernet packet parser), then it might > expect those frames to be minimum 60 bytes (CRC normally not included in > the reported length). And we have small patch in the ODP Netmap driver to > make sure all delivered packets are minimum 60 bytes. > The odp_packet_parse is also used for frames received using the socket packet I/O, but only for frames directly received, i.e. not used if there are other protocols encapsulated in the frames received. Also there is a special case where frames received from netmap can be smaller than 60/64, and that is for frames received from the software ring, that links the Linux network stack with the physical interface through netmap. Please see " netmap: a novel framework for fast packet I/O<http://info.iet.unipi.it/~luigi/papers/20120503-netmap-atc12.pdf>" chapter 4, the diagram on page 5. Having a patch in netmap also makes, I haven't thought of that until now. > > I am just averse to all attempts to artificially modify the packet size, > both increase it if "it is too small" and to truncate the packet should it > be "too large" (a situation I can't understand how it would come about, HW > would truncate the packet and only write what will fit into the RX buffer). > The situation where received buffers can be too large appears because the data is received in netmap rings (which are in fact the regular rings used by the device driver) and then copied to ODP buffers. It is possible in some cases that the ODP buffer pool was configured with a too low size and so we would write data over the boundaries. The same check should be done in the function pkt_mmap_v2_rx (platform/linux-generic/source/odp_packet_socket.c). > > Understand what is the contract between different layers and avoid > redundant checks and operations. Verify the contract instead using assert(). > Using asserts sounds good. > > But go ahead and merge, I will have a look at the merged code instead. > Maybe I will understand better. Please make sure you properly comment the > purpose and function all these "abrovinks" (special fixes). > The patches should always apply to the master branch and so anyone can look at it. Just take a fresh clone of the lng/odp.git repository, copy the contents of the patch and apply it with "git apply <patch>". Skipping the review process and applying the patch for later review is not a good idea. I will try and reach Luigi Rizzo and ask him about this behavior, maybe it's worth patching netmap instead, and prepare a new patch for the comments above. /Ciprian > > On 2 April 2014 10:23, Petri Savolainen <petri.savolainen@linaro.org>wrote: > >> Hi, >> >> Ola, I think you have lost the context here. Could you down load the code >> and browse to odp_packet_parse(). It emulates the ethernet input HW, which >> does basic parsing for raw frames (before it gets e.g. passed to the >> (future) ODP classier or the application). In a real life, SoC HW will fill >> those flags and there won't be a parse function (except for filling in >> missing HW features, as always). >> >> So no matter how many protocols or tunnel headers there are in the frame, >> this function starts from first byte of the frame and raw frame length, >> that was passed from input HW. If you think IPC or other interfaces to pass >> packets to ODP that's a different story then (and a different parse >> function). >> >> Linux-generic have multiple options to connect a pktio interface to a >> Ethernet input port (raw socket, mmap socket and netmap). Since parse >> function works on raw frames, the length should be always >60/64. That's >> the case for all other input methods except netmap. If we modify parse to >> accept small frames from netmap, we will also accept truncated frames other >> sources. Real HW would not do that, without setting the frame_len error >> flag. >> >> -Petri >> >> >> On Tuesday, 1 April 2014 16:28:04 UTC+3, Ola Liljedahl wrote: >>> >>> What I am trying to say is that your "upper layers" (parsing the >>> Ethernet header) might be fed packets that do not come directly from a >>> network interface but instead come from another SW layer which has removed >>> encapsulation (headers) before the Ethernet packet then passing it to your >>> layer-2 input routine. Think L2TP, MACSec (e.g. with SW decryption a la >>> ARMv8) or EtherIP. Nothing can be said about the minimum frame/packet size >>> here (if there ever was one relevant for the packet, that was before >>> encapsulation was removed so now the packet is smaller and possibly >>> "violates" that original requirement). I claim that the packet must be >>> properly processed as long as it is at least the size of the basic Ethernet >>> header (14 bytes). When the packet is not large enough to contain the >>> header for the next layer, parsing stops. >>> >>> >>> On 31 March 2014 15:59, Ciprian Barbu <cipria...@linaro.org> wrote: >>> >>>> >>>> >>>> >>>> On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl <ola.li...@linaro.org>wrote: >>>> >>>>> >>>>> >>>>> *"So, it's fine to allocate min 64 bytes, copy only the payload and >>>>> report length min 64 bytes to packet_parse (into ODP). So, round up the >>>>> length back up to min frame size after netmap receive."* >>>>> I don't get this. If the payload is less than 64 (60 without CRC) >>>>> bytes, why report 64 (60) bytes? SW should be able to handle a packet >>>>> starting with an Ethernet that is less than 60 bytes. >>>>> >>>> >>>> It makes sense in my special case where I get frames coming directly >>>> from the Linux network stack (received by netmap from the SW ring) to make >>>> it look like it came directly from the wire but only if it is guarantied or >>>> at least highly probable that no NIC would deliver frames to the upper >>>> layers that are smaller than 64 (60 without FCS). This unfortunately >>>> discards the discussion about having a packet parsing function that is >>>> robust and reusable. >>>> >>>> >>>>> >>>>> >>>>> >>>>> On 31 March 2014 11:21, Ciprian Barbu <cipria...@linaro.org> wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < >>>>>> petri.sa...@linaro.org> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < >>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>>>>>>>> >>>>>>>>>>>> Here is a simpler version, no return code, just check if the >>>>>>>>>>>> minimum >>>>>>>>>>>> eth header is present. >>>>>>>>>>>> --- >>>>>>>>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>> index 5f07374..b990222 100644 >>>>>>>>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> We should still check against that min frame size (< >>>>>>>>>>> ODP_ETH_LEN_MIN byte) and mark those frames, but continue parsing - just be >>>>>>>>>>> extra careful not to overrun when it's a short frame. >>>>>>>>>>> >>>>>>>>>>> So you actually suggest that if a packet less than >>>>>>>>>> ODP_ETH_LEN_MIN but greater than ODP_ETHHDR_LEN should still be marked as >>>>>>>>>> frame_len error? In my case I get ICMP packets coming from the network >>>>>>>>>> stack with no padding, they have valid headers, but are smaller than 60. >>>>>>>>>> Ola was suggesting to check only against the minimum header len. >>>>>>>>>> >>>>>>>>>> >>>>>>>>> In case of ethernet HW it would be a HW/link error that user would >>>>>>>>> want to know about. You are emulating ethernet link layer with netmap, so >>>>>>>>> may be it's better to play along and allocate always at least 64 byte >>>>>>>>> buffers (no need to memset the tail of the buffer, so there's no extra >>>>>>>>> overhead). >>>>>>>>> >>>>>>>>> Well that was my initial idea as well, but we still have this >>>>>>>> ongoing discussion about odp_packet_parse and there are good arguments so >>>>>>>> far. One of them was that packets could come from sources other than the >>>>>>>> physical medium. Maybe we would like to use the packet parse function for >>>>>>>> frames that we send to the wire. >>>>>>>> >>>>>>> >>>>>>> The packet parse function imitates what SoC HW classifier does (on >>>>>>> an ethernet interface input, _not_ output). It goes through (first levels >>>>>>> of) packet headers and fills in metadata into the packet descriptor. The >>>>>>> frame_len error flags tells to the application that the frame was truncated >>>>>>> _on the wire_. The frame is fine when netmap receives it (length min 64 >>>>>>> bytes), but netmap decides to report only payload length to the upper >>>>>>> layer. You should give the original length to ODP (as raw sockets and real >>>>>>> HW does). Netmap is used to connect ODP to ethernet interface, not to >>>>>>> terminate the protocol. So, it's fine to allocate min 64 bytes, copy only >>>>>>> the payload and report length min 64 bytes to packet_parse (into ODP). So, >>>>>>> round up the length back up to min frame size after netmap receive. >>>>>>> >>>>>>> Packet_parse can be improved with not to give up so easily on >>>>>>> truncated packets, but those are very rare (and may be dropped out by >>>>>>> HW/Linux/netmap already). So it's not so relevant. What you are trying to >>>>>>> do now is to open door for truly truncated frames (14-64 bytes on wire). >>>>>>> >>>>>> >>>>>> Ok then I have no problem leaving odp_packet_parse as it was and just >>>>>> round up the packet size to 60 in the netmap pkt I/O code. The >>>>>> odp_packet_parse function will probably not be used in SoC implementation, >>>>>> since there are HW blocks to do that and we can work on odp_packet_parse >>>>>> improvements later if deemed necessary. >>>>>> >>>>>> I would like Ola's opinion though since he made the observation about >>>>>> packet parsing not being robust enough. >>>>>> >>>>>> >>>>>>> -Petri >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> 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/ >>>>>>> 77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?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/79269a85-8860-44c2-af98-2fc1b73fa9d7%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/79269a85-8860-44c2-af98-2fc1b73fa9d7%40linaro.org?utm_medium=email&utm_source=footer> >> . >> >> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >> > >
"so we would write data over the boundaries." ??? If the RX buffers in the ODP pool are too small for some packets, it is the responsibility of the HW or SW that writes packets into these buffer to only write the amount of data that would fit. Alternatively you make sure *in advance* that the RX buffers are large enough (the MAC may have a configuration on max frame size and all RX buffers are assumed to be of this size, possibly Netmap emulates this behavior). You can't patch (truncate) the packet size afterwards (which I believe you are doing), after the data write has overwritten a buffer that was too small to hold the compete packet. Then corruption might already have occurred. On 2 April 2014 11:42, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > > > > On Wed, Apr 2, 2014 at 12:10 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: > >> If this function is only used for parsing frames received from "a Netmap >> interface" (and not as a general Ethernet packet parser), then it might >> expect those frames to be minimum 60 bytes (CRC normally not included in >> the reported length). And we have small patch in the ODP Netmap driver to >> make sure all delivered packets are minimum 60 bytes. >> > > The odp_packet_parse is also used for frames received using the socket > packet I/O, but only for frames directly received, i.e. not used if there > are other protocols encapsulated in the frames received. Also there is a > special case where frames received from netmap can be smaller than 60/64, > and that is for frames received from the software ring, that links the > Linux network stack with the physical interface through netmap. Please see " > netmap: a novel framework for fast packet I/O<http://info.iet.unipi.it/~luigi/papers/20120503-netmap-atc12.pdf>" > chapter 4, the diagram on page 5. > > Having a patch in netmap also makes, I haven't thought of that until now. > >> >> I am just averse to all attempts to artificially modify the packet size, >> both increase it if "it is too small" and to truncate the packet should it >> be "too large" (a situation I can't understand how it would come about, HW >> would truncate the packet and only write what will fit into the RX buffer). >> > > The situation where received buffers can be too large appears because the > data is received in netmap rings (which are in fact the regular rings used > by the device driver) and then copied to ODP buffers. It is possible in > some cases that the ODP buffer pool was configured with a too low size and > so we would write data over the boundaries. The same check should be done > in the function pkt_mmap_v2_rx > (platform/linux-generic/source/odp_packet_socket.c). > >> >> Understand what is the contract between different layers and avoid >> redundant checks and operations. Verify the contract instead using assert(). >> > > Using asserts sounds good. > >> >> But go ahead and merge, I will have a look at the merged code instead. >> Maybe I will understand better. Please make sure you properly comment the >> purpose and function all these "abrovinks" (special fixes). >> > > The patches should always apply to the master branch and so anyone can > look at it. Just take a fresh clone of the lng/odp.git repository, copy the > contents of the patch and apply it with "git apply <patch>". Skipping the > review process and applying the patch for later review is not a good idea. > > I will try and reach Luigi Rizzo and ask him about this behavior, maybe > it's worth patching netmap instead, and prepare a new patch for the > comments above. > > /Ciprian > > >> >> On 2 April 2014 10:23, Petri Savolainen <petri.savolainen@linaro.org>wrote: >> >>> Hi, >>> >>> Ola, I think you have lost the context here. Could you down load the >>> code and browse to odp_packet_parse(). It emulates the ethernet input HW, >>> which does basic parsing for raw frames (before it gets e.g. passed to the >>> (future) ODP classier or the application). In a real life, SoC HW will fill >>> those flags and there won't be a parse function (except for filling in >>> missing HW features, as always). >>> >>> So no matter how many protocols or tunnel headers there are in the >>> frame, this function starts from first byte of the frame and raw frame >>> length, that was passed from input HW. If you think IPC or other interfaces >>> to pass packets to ODP that's a different story then (and a different parse >>> function). >>> >>> Linux-generic have multiple options to connect a pktio interface to a >>> Ethernet input port (raw socket, mmap socket and netmap). Since parse >>> function works on raw frames, the length should be always >60/64. That's >>> the case for all other input methods except netmap. If we modify parse to >>> accept small frames from netmap, we will also accept truncated frames other >>> sources. Real HW would not do that, without setting the frame_len error >>> flag. >>> >>> -Petri >>> >>> >>> On Tuesday, 1 April 2014 16:28:04 UTC+3, Ola Liljedahl wrote: >>>> >>>> What I am trying to say is that your "upper layers" (parsing the >>>> Ethernet header) might be fed packets that do not come directly from a >>>> network interface but instead come from another SW layer which has removed >>>> encapsulation (headers) before the Ethernet packet then passing it to your >>>> layer-2 input routine. Think L2TP, MACSec (e.g. with SW decryption a la >>>> ARMv8) or EtherIP. Nothing can be said about the minimum frame/packet size >>>> here (if there ever was one relevant for the packet, that was before >>>> encapsulation was removed so now the packet is smaller and possibly >>>> "violates" that original requirement). I claim that the packet must be >>>> properly processed as long as it is at least the size of the basic Ethernet >>>> header (14 bytes). When the packet is not large enough to contain the >>>> header for the next layer, parsing stops. >>>> >>>> >>>> On 31 March 2014 15:59, Ciprian Barbu <cipria...@linaro.org> wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl <ola.li...@linaro.org>wrote: >>>>> >>>>>> >>>>>> >>>>>> *"So, it's fine to allocate min 64 bytes, copy only the payload and >>>>>> report length min 64 bytes to packet_parse (into ODP). So, round up the >>>>>> length back up to min frame size after netmap receive."* >>>>>> I don't get this. If the payload is less than 64 (60 without CRC) >>>>>> bytes, why report 64 (60) bytes? SW should be able to handle a packet >>>>>> starting with an Ethernet that is less than 60 bytes. >>>>>> >>>>> >>>>> It makes sense in my special case where I get frames coming directly >>>>> from the Linux network stack (received by netmap from the SW ring) to make >>>>> it look like it came directly from the wire but only if it is guarantied or >>>>> at least highly probable that no NIC would deliver frames to the upper >>>>> layers that are smaller than 64 (60 without FCS). This unfortunately >>>>> discards the discussion about having a packet parsing function that is >>>>> robust and reusable. >>>>> >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 31 March 2014 11:21, Ciprian Barbu <cipria...@linaro.org> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < >>>>>>> petri.sa...@linaro.org> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < >>>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>>>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>>>>>>>>> >>>>>>>>>>>>> Here is a simpler version, no return code, just check if the >>>>>>>>>>>>> minimum >>>>>>>>>>>>> eth header is present. >>>>>>>>>>>>> --- >>>>>>>>>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>>> index 5f07374..b990222 100644 >>>>>>>>>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> We should still check against that min frame size (< >>>>>>>>>>>> ODP_ETH_LEN_MIN byte) and mark those frames, but continue parsing - just be >>>>>>>>>>>> extra careful not to overrun when it's a short frame. >>>>>>>>>>>> >>>>>>>>>>>> So you actually suggest that if a packet less than >>>>>>>>>>> ODP_ETH_LEN_MIN but greater than ODP_ETHHDR_LEN should still be marked as >>>>>>>>>>> frame_len error? In my case I get ICMP packets coming from the network >>>>>>>>>>> stack with no padding, they have valid headers, but are smaller than 60. >>>>>>>>>>> Ola was suggesting to check only against the minimum header len. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> In case of ethernet HW it would be a HW/link error that user >>>>>>>>>> would want to know about. You are emulating ethernet link layer with >>>>>>>>>> netmap, so may be it's better to play along and allocate always at least 64 >>>>>>>>>> byte buffers (no need to memset the tail of the buffer, so there's no extra >>>>>>>>>> overhead). >>>>>>>>>> >>>>>>>>>> Well that was my initial idea as well, but we still have this >>>>>>>>> ongoing discussion about odp_packet_parse and there are good arguments so >>>>>>>>> far. One of them was that packets could come from sources other than the >>>>>>>>> physical medium. Maybe we would like to use the packet parse function for >>>>>>>>> frames that we send to the wire. >>>>>>>>> >>>>>>>> >>>>>>>> The packet parse function imitates what SoC HW classifier does (on >>>>>>>> an ethernet interface input, _not_ output). It goes through (first levels >>>>>>>> of) packet headers and fills in metadata into the packet descriptor. The >>>>>>>> frame_len error flags tells to the application that the frame was truncated >>>>>>>> _on the wire_. The frame is fine when netmap receives it (length min 64 >>>>>>>> bytes), but netmap decides to report only payload length to the upper >>>>>>>> layer. You should give the original length to ODP (as raw sockets and real >>>>>>>> HW does). Netmap is used to connect ODP to ethernet interface, not to >>>>>>>> terminate the protocol. So, it's fine to allocate min 64 bytes, copy only >>>>>>>> the payload and report length min 64 bytes to packet_parse (into ODP). So, >>>>>>>> round up the length back up to min frame size after netmap receive. >>>>>>>> >>>>>>>> Packet_parse can be improved with not to give up so easily on >>>>>>>> truncated packets, but those are very rare (and may be dropped out by >>>>>>>> HW/Linux/netmap already). So it's not so relevant. What you are trying to >>>>>>>> do now is to open door for truly truncated frames (14-64 bytes on wire). >>>>>>>> >>>>>>> >>>>>>> Ok then I have no problem leaving odp_packet_parse as it was and >>>>>>> just round up the packet size to 60 in the netmap pkt I/O code. The >>>>>>> odp_packet_parse function will probably not be used in SoC implementation, >>>>>>> since there are HW blocks to do that and we can work on odp_packet_parse >>>>>>> improvements later if deemed necessary. >>>>>>> >>>>>>> I would like Ola's opinion though since he made the observation >>>>>>> about packet parsing not being robust enough. >>>>>>> >>>>>>> >>>>>>>> -Petri >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> 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/ >>>>>>>> 77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?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/79269a85-8860-44c2-af98-2fc1b73fa9d7%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/79269a85-8860-44c2-af98-2fc1b73fa9d7%40linaro.org?utm_medium=email&utm_source=footer> >>> . >>> >>> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >>> >> >> >
On Wed, Apr 2, 2014 at 1:09 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: > "so we would write data over the boundaries." ??? > If the RX buffers in the ODP pool are too small for some packets, it is > the responsibility of the HW or SW that writes packets into these buffer to > only write the amount of data that would fit. Alternatively you make sure *in > advance* that the RX buffers are large enough (the MAC may have a > configuration on max frame size and all RX buffers are assumed to be of > this size, possibly Netmap emulates this behavior). You can't patch > (truncate) the packet size afterwards (which I believe you are doing), > after the data write has overwritten a buffer that was too small to hold > the compete packet. Then corruption might already have occurred. > I think you're wrong here, I will search the netmap code to be sure, but the way it goes, as far as I understand it, is like this: - the device driver uses rings of buffers for RX (as well as TX), that contain both payload and some metadata for managing the buffers - netmap maps the same memory used to hold the payload in the rings and additionally creates it's own set of rings containing space for only the metadata, the payload is referenced with pointers - whatever checks / configuration that the device drivers makes in order to impose a maximum frame length apply for netmap too (because it uses the same buffers) - when a packet is received, the device driver copies the payload in the available space, truncating if necessary, and informs netmap - netmap then does it's own internal management to complete a request - the control then reaches to odp, where it checks the received payload length and truncates if the room in ODP buffers is not big enough. There is no prior corruption because the device driver takes care of that, netmap mainly maps the memory into userspace and handles the various requests to send/receive (plus some interrupt coalescing if I'm not mistaking) . Netmap has been developed with the purpose to invade as little as possible the device driver, letting little room for errors. The result is netmap aware device drivers, not the other way around. > > > On 2 April 2014 11:42, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > >> >> >> >> On Wed, Apr 2, 2014 at 12:10 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: >> >>> If this function is only used for parsing frames received from "a Netmap >>> interface" (and not as a general Ethernet packet parser), then it might >>> expect those frames to be minimum 60 bytes (CRC normally not included in >>> the reported length). And we have small patch in the ODP Netmap driver to >>> make sure all delivered packets are minimum 60 bytes. >>> >> >> The odp_packet_parse is also used for frames received using the socket >> packet I/O, but only for frames directly received, i.e. not used if there >> are other protocols encapsulated in the frames received. Also there is a >> special case where frames received from netmap can be smaller than 60/64, >> and that is for frames received from the software ring, that links the >> Linux network stack with the physical interface through netmap. Please see " >> netmap: a novel framework for fast packet I/O<http://info.iet.unipi.it/~luigi/papers/20120503-netmap-atc12.pdf>" >> chapter 4, the diagram on page 5. >> >> Having a patch in netmap also makes, I haven't thought of that until now. >> >>> >>> I am just averse to all attempts to artificially modify the packet size, >>> both increase it if "it is too small" and to truncate the packet should it >>> be "too large" (a situation I can't understand how it would come about, HW >>> would truncate the packet and only write what will fit into the RX buffer). >>> >> >> The situation where received buffers can be too large appears because the >> data is received in netmap rings (which are in fact the regular rings used >> by the device driver) and then copied to ODP buffers. It is possible in >> some cases that the ODP buffer pool was configured with a too low size and >> so we would write data over the boundaries. The same check should be done >> in the function pkt_mmap_v2_rx >> (platform/linux-generic/source/odp_packet_socket.c). >> >>> >>> Understand what is the contract between different layers and avoid >>> redundant checks and operations. Verify the contract instead using assert(). >>> >> >> Using asserts sounds good. >> >>> >>> But go ahead and merge, I will have a look at the merged code instead. >>> Maybe I will understand better. Please make sure you properly comment the >>> purpose and function all these "abrovinks" (special fixes). >>> >> >> The patches should always apply to the master branch and so anyone can >> look at it. Just take a fresh clone of the lng/odp.git repository, copy the >> contents of the patch and apply it with "git apply <patch>". Skipping the >> review process and applying the patch for later review is not a good idea. >> >> I will try and reach Luigi Rizzo and ask him about this behavior, maybe >> it's worth patching netmap instead, and prepare a new patch for the >> comments above. >> >> /Ciprian >> >> >>> >>> On 2 April 2014 10:23, Petri Savolainen <petri.savolainen@linaro.org>wrote: >>> >>>> Hi, >>>> >>>> Ola, I think you have lost the context here. Could you down load the >>>> code and browse to odp_packet_parse(). It emulates the ethernet input HW, >>>> which does basic parsing for raw frames (before it gets e.g. passed to the >>>> (future) ODP classier or the application). In a real life, SoC HW will fill >>>> those flags and there won't be a parse function (except for filling in >>>> missing HW features, as always). >>>> >>>> So no matter how many protocols or tunnel headers there are in the >>>> frame, this function starts from first byte of the frame and raw frame >>>> length, that was passed from input HW. If you think IPC or other interfaces >>>> to pass packets to ODP that's a different story then (and a different parse >>>> function). >>>> >>>> Linux-generic have multiple options to connect a pktio interface to a >>>> Ethernet input port (raw socket, mmap socket and netmap). Since parse >>>> function works on raw frames, the length should be always >60/64. That's >>>> the case for all other input methods except netmap. If we modify parse to >>>> accept small frames from netmap, we will also accept truncated frames other >>>> sources. Real HW would not do that, without setting the frame_len error >>>> flag. >>>> >>>> -Petri >>>> >>>> >>>> On Tuesday, 1 April 2014 16:28:04 UTC+3, Ola Liljedahl wrote: >>>>> >>>>> What I am trying to say is that your "upper layers" (parsing the >>>>> Ethernet header) might be fed packets that do not come directly from a >>>>> network interface but instead come from another SW layer which has removed >>>>> encapsulation (headers) before the Ethernet packet then passing it to your >>>>> layer-2 input routine. Think L2TP, MACSec (e.g. with SW decryption a la >>>>> ARMv8) or EtherIP. Nothing can be said about the minimum frame/packet size >>>>> here (if there ever was one relevant for the packet, that was before >>>>> encapsulation was removed so now the packet is smaller and possibly >>>>> "violates" that original requirement). I claim that the packet must be >>>>> properly processed as long as it is at least the size of the basic Ethernet >>>>> header (14 bytes). When the packet is not large enough to contain the >>>>> header for the next layer, parsing stops. >>>>> >>>>> >>>>> On 31 March 2014 15:59, Ciprian Barbu <cipria...@linaro.org> wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Mon, Mar 31, 2014 at 3:09 PM, Ola Liljedahl <ola.li...@linaro.org>wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> *"So, it's fine to allocate min 64 bytes, copy only the payload and >>>>>>> report length min 64 bytes to packet_parse (into ODP). So, round up the >>>>>>> length back up to min frame size after netmap receive."* >>>>>>> I don't get this. If the payload is less than 64 (60 without CRC) >>>>>>> bytes, why report 64 (60) bytes? SW should be able to handle a packet >>>>>>> starting with an Ethernet that is less than 60 bytes. >>>>>>> >>>>>> >>>>>> It makes sense in my special case where I get frames coming directly >>>>>> from the Linux network stack (received by netmap from the SW ring) to make >>>>>> it look like it came directly from the wire but only if it is guarantied or >>>>>> at least highly probable that no NIC would deliver frames to the upper >>>>>> layers that are smaller than 64 (60 without FCS). This unfortunately >>>>>> discards the discussion about having a packet parsing function that is >>>>>> robust and reusable. >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 31 March 2014 11:21, Ciprian Barbu <cipria...@linaro.org> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Mon, Mar 31, 2014 at 12:03 PM, Petri Savolainen < >>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Friday, 28 March 2014 16:49:13 UTC+2, Ciprian Barbu wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Fri, Mar 28, 2014 at 4:18 PM, Petri Savolainen < >>>>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Friday, 28 March 2014 16:01:14 UTC+2, Ciprian Barbu wrote: >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Fri, Mar 28, 2014 at 1:32 PM, Petri Savolainen < >>>>>>>>>>>> petri.sa...@linaro.org> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Friday, 28 March 2014 13:20:40 UTC+2, Ciprian Barbu wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Here is a simpler version, no return code, just check if the >>>>>>>>>>>>>> minimum >>>>>>>>>>>>>> eth header is present. >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> platform/linux-generic/source/odp_packet.c | 2 +- >>>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>>>>>>> >>>>>>>>>>>>>> diff --git a/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>>>> b/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>>>> index 5f07374..b990222 100644 >>>>>>>>>>>>>> --- a/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>>>> +++ b/platform/linux-generic/source/odp_packet.c >>>>>>>>>>>>>> @@ -154,7 +154,7 @@ 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)) { >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> We should still check against that min frame size (< >>>>>>>>>>>>> ODP_ETH_LEN_MIN byte) and mark those frames, but continue parsing - just be >>>>>>>>>>>>> extra careful not to overrun when it's a short frame. >>>>>>>>>>>>> >>>>>>>>>>>>> So you actually suggest that if a packet less than >>>>>>>>>>>> ODP_ETH_LEN_MIN but greater than ODP_ETHHDR_LEN should still be marked as >>>>>>>>>>>> frame_len error? In my case I get ICMP packets coming from the network >>>>>>>>>>>> stack with no padding, they have valid headers, but are smaller than 60. >>>>>>>>>>>> Ola was suggesting to check only against the minimum header len. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> In case of ethernet HW it would be a HW/link error that user >>>>>>>>>>> would want to know about. You are emulating ethernet link layer with >>>>>>>>>>> netmap, so may be it's better to play along and allocate always at least 64 >>>>>>>>>>> byte buffers (no need to memset the tail of the buffer, so there's no extra >>>>>>>>>>> overhead). >>>>>>>>>>> >>>>>>>>>>> Well that was my initial idea as well, but we still have this >>>>>>>>>> ongoing discussion about odp_packet_parse and there are good arguments so >>>>>>>>>> far. One of them was that packets could come from sources other than the >>>>>>>>>> physical medium. Maybe we would like to use the packet parse function for >>>>>>>>>> frames that we send to the wire. >>>>>>>>>> >>>>>>>>> >>>>>>>>> The packet parse function imitates what SoC HW classifier does (on >>>>>>>>> an ethernet interface input, _not_ output). It goes through (first levels >>>>>>>>> of) packet headers and fills in metadata into the packet descriptor. The >>>>>>>>> frame_len error flags tells to the application that the frame was truncated >>>>>>>>> _on the wire_. The frame is fine when netmap receives it (length min 64 >>>>>>>>> bytes), but netmap decides to report only payload length to the upper >>>>>>>>> layer. You should give the original length to ODP (as raw sockets and real >>>>>>>>> HW does). Netmap is used to connect ODP to ethernet interface, not to >>>>>>>>> terminate the protocol. So, it's fine to allocate min 64 bytes, copy only >>>>>>>>> the payload and report length min 64 bytes to packet_parse (into ODP). So, >>>>>>>>> round up the length back up to min frame size after netmap receive. >>>>>>>>> >>>>>>>>> Packet_parse can be improved with not to give up so easily on >>>>>>>>> truncated packets, but those are very rare (and may be dropped out by >>>>>>>>> HW/Linux/netmap already). So it's not so relevant. What you are trying to >>>>>>>>> do now is to open door for truly truncated frames (14-64 bytes on wire). >>>>>>>>> >>>>>>>> >>>>>>>> Ok then I have no problem leaving odp_packet_parse as it was and >>>>>>>> just round up the packet size to 60 in the netmap pkt I/O code. The >>>>>>>> odp_packet_parse function will probably not be used in SoC implementation, >>>>>>>> since there are HW blocks to do that and we can work on odp_packet_parse >>>>>>>> improvements later if deemed necessary. >>>>>>>> >>>>>>>> I would like Ola's opinion though since he made the observation >>>>>>>> about packet parsing not being robust enough. >>>>>>>> >>>>>>>> >>>>>>>>> -Petri >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> 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/ >>>>>>>>> 77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/77dd0fe8-653c-4ac4-8650-266ca03384c4%40linaro.org?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/79269a85-8860-44c2-af98-2fc1b73fa9d7%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/79269a85-8860-44c2-af98-2fc1b73fa9d7%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/source/odp_packet.c b/platform/linux-generic/source/odp_packet.c index 5f07374..b990222 100644 --- a/platform/linux-generic/source/odp_packet.c +++ b/platform/linux-generic/source/odp_packet.c @@ -154,7 +154,7 @@ 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; } else if (len > ODP_ETH_LEN_MAX) {
Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> Here is a simpler version, no return code, just check if the minimum eth header is present. --- platform/linux-generic/source/odp_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)