| Message ID | 1396348796-43137-1-git-send-email-ciprian.barbu@linaro.org |
|---|---|
| State | Under Review |
| Headers | show |
*I disagree strongly with these changes!*
How can payload_len be larger than max_frame_len? What what is
max_frame_len? There is no hard max frame length on Ethernet although some
MAC's might refuse to send frames longer than some MAC-specific value and
some network equipment won't forward them properly.
If payload_len is computed (I don't have the complete source code here),
then you can't compute a length that is not contained within the packet
(i.e. larger than the *actual* frame length).
I repeatedly state that we should not increase the payload_len just because
it is smaller than some link specific value. See my other comment on this
subject. You are also claiming that there is valid data in a valid buffer,
extending beyond the reported packet size. This might not be true. What
happens if the application reads the data that is not there or writes to
the buffer. We would be reading garbage and might be overwriting some other
data structure.
Last I don't think we should call debug macros if these would should occur
and be detected. These are typical cases where you just increase a
statistic counter and possibly mark the packet descriptor (typical HW
and/or driver level responsibilities). That's all. But in these specific
cases I think we should do nothing.
+ if (odp_unlikely(payload_len >
pkt_nm->max_frame_len)) {
ODP_ERR("Data partially lost %u %lu!\n",
payload_len, pkt_nm->max_frame_len);
payload_len = pkt_nm->max_frame_len;
+ } else if (odp_unlikely(payload_len <
ODP_ETH_LEN_MIN)) {
+ if (odp_unlikely(pkt_nm->netmap_mode !=
ODP_NETMAP_MODE_SW))
+ ODP_DBG("Frame appears to be
truncated: %u\n",
+
(unsigned)payload_len);
+ payload_len = ODP_ETH_LEN_MIN;
On 1 April 2014 12:39, Ciprian Barbu <ciprian.barbu@linaro.org> wrote:
> This patch sets the minimum eth frame len for packets arrived from netmap.
> Additionally, if the packet was not received from the SW ring a debug
> message
> is printed.
>
> Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org>
> ---
> platform/linux-generic/include/odp_packet_netmap.h | 5 +----
> platform/linux-generic/source/odp_packet_netmap.c | 10 ++++++++--
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/platform/linux-generic/include/odp_packet_netmap.h
> b/platform/linux-generic/include/odp_packet_netmap.h
> index 960ccbf..9d5fb88 100644
> --- a/platform/linux-generic/include/odp_packet_netmap.h
> +++ b/platform/linux-generic/include/odp_packet_netmap.h
> @@ -34,12 +34,9 @@ typedef struct {
> struct nm_desc_t *nm_desc;
> struct netmap_ring *rxring;
> struct netmap_ring *txring;
> -
> - /********************************/
> odp_queue_t tx_access; /* Used for exclusive access to send
> packets */
> uint32_t if_flags;
> - uint32_t if_reqcap;
> - uint32_t if_curcap;
> + int netmap_mode;
> char ifname[32];
> } pkt_netmap_t;
>
> diff --git a/platform/linux-generic/source/odp_packet_netmap.c
> b/platform/linux-generic/source/odp_packet_netmap.c
> index 1cbd84c..2d459b9 100644
> --- a/platform/linux-generic/source/odp_packet_netmap.c
> +++ b/platform/linux-generic/source/odp_packet_netmap.c
> @@ -131,6 +131,7 @@ int setup_pkt_netmap(pkt_netmap_t * const pkt_nm, char
> *netdev,
> pkt_nm->buf_size = odp_buffer_size(buf);
> /* max frame len taking into account the l2-offset */
> pkt_nm->max_frame_len = pkt_nm->buf_size - pkt_nm->l2_offset;
> + pkt_nm->netmap_mode = nm_params->netmap_mode;
>
> odp_buffer_free(buf);
>
> @@ -293,10 +294,15 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm,
> odp_packet_t pkt_table[],
> rxring->head = nm_ring_next(rxring, cur);
> rxring->cur = nm_ring_next(rxring, cur);
>
> - if (payload_len > pkt_nm->max_frame_len) {
> + if (odp_unlikely(payload_len >
> pkt_nm->max_frame_len)) {
> ODP_ERR("Data partially lost %u %lu!\n",
> payload_len,
> pkt_nm->max_frame_len);
> payload_len = pkt_nm->max_frame_len;
> + } else if (odp_unlikely(payload_len <
> ODP_ETH_LEN_MIN)) {
> + if (odp_unlikely(pkt_nm->netmap_mode !=
> ODP_NETMAP_MODE_SW))
> + ODP_DBG("Frame appears to be
> truncated: %u\n",
> +
> (unsigned)payload_len);
> + payload_len = ODP_ETH_LEN_MIN;
> }
>
> pkt_buf = odp_packet_buf_addr(pkt);
> @@ -361,7 +367,7 @@ int send_pkt_netmap(pkt_netmap_t * const pkt_nm,
> odp_packet_t pkt_table[],
> txbuf = NETMAP_BUF(txring, slot->buf_idx);
>
> pkt = pkt_table[i];
> - frame = odp_packet_l2(pkt);
> + frame = odp_packet_start(pkt);
> frame_len = odp_packet_get_len(pkt);
>
> memcpy(txbuf, frame, frame_len);
> --
> 1.8.3.2
>
> --
> You received this message because you are subscribed to the Google Groups
> "LNG ODP Sub-team - lng-odp@linaro.org" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to lng-odp+unsubscribe@linaro.org.
> To post to this group, send email to lng-odp@linaro.org.
> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/.
> To view this discussion on the web visit
> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/1396348796-43137-1-git-send-email-ciprian.barbu%40linaro.org
> .
> For more options, visit https://groups.google.com/a/linaro.org/d/optout.
>
How can the MAC (or whomever) put a too large packet into a too small buffer? If the RX buffer is too small, the packet will be truncated (i.e. the reported length will not exceed the buffer size), some flag set in the packet descriptor indicating the truncation and most likely a statistics counter updated. I have never seen a MAC that reports a packet size larger than the RX buffer size. Have you actually seen the occurrence of this situation? On 1 April 2014 16:02, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 04/01/2014 05:41 PM, Ola Liljedahl wrote: > >> *I disagree strongly with these changes!* >> >> >> How can payload_len be larger than max_frame_len? What what is >> max_frame_len? >> > > if you compiled odp with small packets in the pool: > odp_buffer_pool_t odp_buffer_pool_create(const char *name, > void *base_addr, uint64_t size, > size_t buf_size, size_t buf_align, > int buf_type) > > buf_size parameter. > > Maxim. > >> There is no hard max frame length on Ethernet although some MAC's might >> refuse to send frames longer than some MAC-specific value and some network >> equipment won't forward them properly. >> If payload_len is computed (I don't have the complete source code here), >> then you can't compute a length that is not contained within the packet >> (i.e. larger than the *actual* frame length). >> >> >> I repeatedly state that we should not increase the payload_len just >> because it is smaller than some link specific value. See my other comment >> on this subject. You are also claiming that there is valid data in a valid >> buffer, extending beyond the reported packet size. This might not be true. >> What happens if the application reads the data that is not there or writes >> to the buffer. We would be reading garbage and might be overwriting some >> other data structure. >> >> Last I don't think we should call debug macros if these would should >> occur and be detected. These are typical cases where you just increase a >> statistic counter and possibly mark the packet descriptor (typical HW >> and/or driver level responsibilities). That's all. But in these specific >> cases I think we should do nothing. >> >> + if (odp_unlikely(payload_len > >> pkt_nm->max_frame_len)) { >> ODP_ERR("Data partially lost %u %lu!\n", >> payload_len, >> pkt_nm->max_frame_len); >> payload_len = pkt_nm->max_frame_len; >> + } else if (odp_unlikely(payload_len < >> ODP_ETH_LEN_MIN)) { >> + if (odp_unlikely(pkt_nm->netmap_mode != >> ODP_NETMAP_MODE_SW)) >> + ODP_DBG("Frame appears to be >> truncated: %u\n", >> + (unsigned)payload_len); >> + payload_len = ODP_ETH_LEN_MIN; >> >> >> On 1 April 2014 12:39, Ciprian Barbu <ciprian.barbu@linaro.org <mailto: >> ciprian.barbu@linaro.org>> wrote: >> >> This patch sets the minimum eth frame len for packets arrived from >> netmap. >> Additionally, if the packet was not received from the SW ring a >> debug message >> is printed. >> >> Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org >> <mailto:ciprian.barbu@linaro.org>> >> >> --- >> platform/linux-generic/include/odp_packet_netmap.h | 5 +---- >> platform/linux-generic/source/odp_packet_netmap.c | 10 ++++++++-- >> 2 files changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp_packet_netmap.h >> b/platform/linux-generic/include/odp_packet_netmap.h >> index 960ccbf..9d5fb88 100644 >> --- a/platform/linux-generic/include/odp_packet_netmap.h >> +++ b/platform/linux-generic/include/odp_packet_netmap.h >> @@ -34,12 +34,9 @@ typedef struct { >> struct nm_desc_t *nm_desc; >> struct netmap_ring *rxring; >> struct netmap_ring *txring; >> - >> - /********************************/ >> odp_queue_t tx_access; /* Used for exclusive access to >> send packets */ >> uint32_t if_flags; >> - uint32_t if_reqcap; >> - uint32_t if_curcap; >> + int netmap_mode; >> char ifname[32]; >> } pkt_netmap_t; >> >> diff --git a/platform/linux-generic/source/odp_packet_netmap.c >> b/platform/linux-generic/source/odp_packet_netmap.c >> index 1cbd84c..2d459b9 100644 >> --- a/platform/linux-generic/source/odp_packet_netmap.c >> +++ b/platform/linux-generic/source/odp_packet_netmap.c >> @@ -131,6 +131,7 @@ int setup_pkt_netmap(pkt_netmap_t * const >> pkt_nm, char *netdev, >> pkt_nm->buf_size = odp_buffer_size(buf); >> /* max frame len taking into account the l2-offset */ >> pkt_nm->max_frame_len = pkt_nm->buf_size - pkt_nm->l2_offset; >> + pkt_nm->netmap_mode = nm_params->netmap_mode; >> >> odp_buffer_free(buf); >> >> @@ -293,10 +294,15 @@ int recv_pkt_netmap(pkt_netmap_t * const >> pkt_nm, odp_packet_t pkt_table[], >> rxring->head = nm_ring_next(rxring, cur); >> rxring->cur = nm_ring_next(rxring, cur); >> >> - if (payload_len > pkt_nm->max_frame_len) { >> + if (odp_unlikely(payload_len > >> pkt_nm->max_frame_len)) { >> ODP_ERR("Data partially lost %u >> %lu!\n", >> payload_len, >> pkt_nm->max_frame_len); >> payload_len = pkt_nm->max_frame_len; >> + } else if (odp_unlikely(payload_len < >> ODP_ETH_LEN_MIN)) { >> + if >> (odp_unlikely(pkt_nm->netmap_mode != ODP_NETMAP_MODE_SW)) >> + ODP_DBG("Frame appears to >> be truncated: %u\n", >> + (unsigned)payload_len); >> + payload_len = ODP_ETH_LEN_MIN; >> } >> >> pkt_buf = odp_packet_buf_addr(pkt); >> @@ -361,7 +367,7 @@ int send_pkt_netmap(pkt_netmap_t * const >> pkt_nm, odp_packet_t pkt_table[], >> txbuf = NETMAP_BUF(txring, slot->buf_idx); >> >> pkt = pkt_table[i]; >> - frame = odp_packet_l2(pkt); >> + frame = odp_packet_start(pkt); >> frame_len = odp_packet_get_len(pkt); >> >> memcpy(txbuf, frame, frame_len); >> -- >> 1.8.3.2 >> >> -- >> 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/ >> 1396348796-43137-1-git-send-email-ciprian.barbu%40linaro.org. >> 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/CAPiYAf41gau3Aj50DTraNz1H1zMAp >> 3ciJ33B5U3ZkVjn-F3j4Q%40mail.gmail.com <https://groups.google.com/a/ >> linaro.org/d/msgid/lng-odp/CAPiYAf41gau3Aj50DTraNz1H1zMAp >> 3ciJ33B5U3ZkVjn-F3j4Q%40mail.gmail.com?utm_medium=email&utm_source=footer >> >. >> >> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >> > >
Hi, On Tue, Apr 1, 2014 at 6:33 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: > How can the MAC (or whomever) put a too large packet into a too small > buffer? If the RX buffer is too small, the packet will be truncated (i.e. > the reported length will not exceed the buffer size), some flag set in the > packet descriptor indicating the truncation and most likely a statistics > counter updated. I have never seen a MAC that reports a packet size larger > than the RX buffer size. Have you actually seen the occurrence of this > situation? > > > > On 1 April 2014 16:02, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > >> On 04/01/2014 05:41 PM, Ola Liljedahl wrote: >> >>> *I disagree strongly with these changes!* >>> >>> >>> How can payload_len be larger than max_frame_len? What what is >>> max_frame_len? >>> >> >> if you compiled odp with small packets in the pool: >> odp_buffer_pool_t odp_buffer_pool_create(const char *name, >> void *base_addr, uint64_t size, >> size_t buf_size, size_t buf_align, >> int buf_type) >> >> buf_size parameter. > > >> Maxim. >> >>> There is no hard max frame length on Ethernet although some MAC's might >>> refuse to send frames longer than some MAC-specific value and some network >>> equipment won't forward them properly. >>> If payload_len is computed (I don't have the complete source code here), >>> then you can't compute a length that is not contained within the packet >>> (i.e. larger than the *actual* frame length). >>> >>> >>> I repeatedly state that we should not increase the payload_len just >>> because it is smaller than some link specific value. See my other comment >>> on this subject. You are also claiming that there is valid data in a valid >>> buffer, extending beyond the reported packet size. This might not be true. >>> What happens if the application reads the data that is not there or writes >>> to the buffer. We would be reading garbage and might be overwriting some >>> other data structure. >>> >> You're right about the padding, it should be filled with zeros in case we go with this option. You don't agree with this changes and I understand, but I'm trying to push forward since we are not reaching a conclusion. If I understand correctly you were in agreement with the previous (v3) patch I sent. However Petri suggested the other approach, so I've sent out a version that would fit his idea for fixing this problem. I'm not saying we should go with either of them just yet, but I do prefer Petri's suggestion more than tweaking the odp_packet_parse function at this moment, simply because I don't see a major problem with leaving that function as it is for the moment and concentrate on fixing the problem with netmap. It's not ideal, but we've been arguing on this issue for quite some time now and frankly I would prefer if you and Petri reached a common ground soon. /Ciprian > >>> Last I don't think we should call debug macros if these would should >>> occur and be detected. These are typical cases where you just increase a >>> statistic counter and possibly mark the packet descriptor (typical HW >>> and/or driver level responsibilities). That's all. But in these specific >>> cases I think we should do nothing. >>> >>> + if (odp_unlikely(payload_len > >>> pkt_nm->max_frame_len)) { >>> ODP_ERR("Data partially lost %u %lu!\n", >>> payload_len, >>> pkt_nm->max_frame_len); >>> payload_len = pkt_nm->max_frame_len; >>> + } else if (odp_unlikely(payload_len < >>> ODP_ETH_LEN_MIN)) { >>> + if (odp_unlikely(pkt_nm->netmap_mode != >>> ODP_NETMAP_MODE_SW)) >>> + ODP_DBG("Frame appears to be >>> truncated: %u\n", >>> + (unsigned)payload_len); >>> + payload_len = ODP_ETH_LEN_MIN; >>> >>> >>> On 1 April 2014 12:39, Ciprian Barbu <ciprian.barbu@linaro.org <mailto: >>> ciprian.barbu@linaro.org>> wrote: >>> >>> This patch sets the minimum eth frame len for packets arrived from >>> netmap. >>> Additionally, if the packet was not received from the SW ring a >>> debug message >>> is printed. >>> >>> Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org >>> <mailto:ciprian.barbu@linaro.org>> >>> >>> --- >>> platform/linux-generic/include/odp_packet_netmap.h | 5 +---- >>> platform/linux-generic/source/odp_packet_netmap.c | 10 ++++++++-- >>> 2 files changed, 9 insertions(+), 6 deletions(-) >>> >>> diff --git a/platform/linux-generic/include/odp_packet_netmap.h >>> b/platform/linux-generic/include/odp_packet_netmap.h >>> index 960ccbf..9d5fb88 100644 >>> --- a/platform/linux-generic/include/odp_packet_netmap.h >>> +++ b/platform/linux-generic/include/odp_packet_netmap.h >>> @@ -34,12 +34,9 @@ typedef struct { >>> struct nm_desc_t *nm_desc; >>> struct netmap_ring *rxring; >>> struct netmap_ring *txring; >>> - >>> - /********************************/ >>> odp_queue_t tx_access; /* Used for exclusive access to >>> send packets */ >>> uint32_t if_flags; >>> - uint32_t if_reqcap; >>> - uint32_t if_curcap; >>> + int netmap_mode; >>> char ifname[32]; >>> } pkt_netmap_t; >>> >>> diff --git a/platform/linux-generic/source/odp_packet_netmap.c >>> b/platform/linux-generic/source/odp_packet_netmap.c >>> index 1cbd84c..2d459b9 100644 >>> --- a/platform/linux-generic/source/odp_packet_netmap.c >>> +++ b/platform/linux-generic/source/odp_packet_netmap.c >>> @@ -131,6 +131,7 @@ int setup_pkt_netmap(pkt_netmap_t * const >>> pkt_nm, char *netdev, >>> pkt_nm->buf_size = odp_buffer_size(buf); >>> /* max frame len taking into account the l2-offset */ >>> pkt_nm->max_frame_len = pkt_nm->buf_size - pkt_nm->l2_offset; >>> + pkt_nm->netmap_mode = nm_params->netmap_mode; >>> >>> odp_buffer_free(buf); >>> >>> @@ -293,10 +294,15 @@ int recv_pkt_netmap(pkt_netmap_t * const >>> pkt_nm, odp_packet_t pkt_table[], >>> rxring->head = nm_ring_next(rxring, cur); >>> rxring->cur = nm_ring_next(rxring, cur); >>> >>> - if (payload_len > pkt_nm->max_frame_len) { >>> + if (odp_unlikely(payload_len > >>> pkt_nm->max_frame_len)) { >>> ODP_ERR("Data partially lost %u >>> %lu!\n", >>> payload_len, >>> pkt_nm->max_frame_len); >>> payload_len = pkt_nm->max_frame_len; >>> + } else if (odp_unlikely(payload_len < >>> ODP_ETH_LEN_MIN)) { >>> + if >>> (odp_unlikely(pkt_nm->netmap_mode != ODP_NETMAP_MODE_SW)) >>> + ODP_DBG("Frame appears to >>> be truncated: %u\n", >>> + (unsigned)payload_len); >>> + payload_len = ODP_ETH_LEN_MIN; >>> } >>> >>> pkt_buf = odp_packet_buf_addr(pkt); >>> @@ -361,7 +367,7 @@ int send_pkt_netmap(pkt_netmap_t * const >>> pkt_nm, odp_packet_t pkt_table[], >>> txbuf = NETMAP_BUF(txring, slot->buf_idx); >>> >>> pkt = pkt_table[i]; >>> - frame = odp_packet_l2(pkt); >>> + frame = odp_packet_start(pkt); >>> frame_len = odp_packet_get_len(pkt); >>> >>> memcpy(txbuf, frame, frame_len); >>> -- >>> 1.8.3.2 >>> >>> -- >>> 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/ >>> 1396348796-43137-1-git-send-email-ciprian.barbu%40linaro.org. >>> 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/CAPiYAf41gau3Aj50DTraNz1H1zMAp >>> 3ciJ33B5U3ZkVjn-F3j4Q%40mail.gmail.com <https://groups.google.com/a/ >>> linaro.org/d/msgid/lng-odp/CAPiYAf41gau3Aj50DTraNz1H1zMAp >>> 3ciJ33B5U3ZkVjn-F3j4Q%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/CAPiYAf5qS39o0vnQZPwNTTTwQSznNKTTfYPyeMCD4KwOHrYotg%40mail.gmail.com<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CAPiYAf5qS39o0vnQZPwNTTTwQSznNKTTfYPyeMCD4KwOHrYotg%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
On 2014-04-01 21:25, Ciprian Barbu wrote: > Hi, > > > On Tue, Apr 1, 2014 at 6:33 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote: > > > How can the MAC (or whomever) put a too large packet into a too small > > buffer? If the RX buffer is too small, the packet will be truncated (i.e. > > the reported length will not exceed the buffer size), some flag set in the > > packet descriptor indicating the truncation and most likely a statistics > > counter updated. I have never seen a MAC that reports a packet size larger > > than the RX buffer size. Have you actually seen the occurrence of this > > situation? > > > > > > > > On 1 April 2014 16:02, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > > >> On 04/01/2014 05:41 PM, Ola Liljedahl wrote: > >> > >>> *I disagree strongly with these changes!* > >>> > >>> > >>> How can payload_len be larger than max_frame_len? What what is > >>> max_frame_len? > >>> > >> > >> if you compiled odp with small packets in the pool: > >> odp_buffer_pool_t odp_buffer_pool_create(const char *name, > >> void *base_addr, uint64_t size, > >> size_t buf_size, size_t buf_align, > >> int buf_type) > >> > >> buf_size parameter. > > > > > >> Maxim. > >> > >>> There is no hard max frame length on Ethernet although some MAC's might > >>> refuse to send frames longer than some MAC-specific value and some network > >>> equipment won't forward them properly. > >>> If payload_len is computed (I don't have the complete source code here), > >>> then you can't compute a length that is not contained within the packet > >>> (i.e. larger than the *actual* frame length). > >>> > >>> > >>> I repeatedly state that we should not increase the payload_len just > >>> because it is smaller than some link specific value. See my other comment > >>> on this subject. You are also claiming that there is valid data in a valid > >>> buffer, extending beyond the reported packet size. This might not be true. > >>> What happens if the application reads the data that is not there or writes > >>> to the buffer. We would be reading garbage and might be overwriting some > >>> other data structure. > >>> > >> > You're right about the padding, it should be filled with zeros in case we > go with this option. > > You don't agree with this changes and I understand, but I'm trying to push > forward since we are not reaching a conclusion. If I understand correctly > you were in agreement with the previous (v3) patch I sent. However Petri > suggested the other approach, so I've sent out a version that would fit his > idea for fixing this problem. I'm not saying we should go with either of > them just yet, but I do prefer Petri's suggestion more than tweaking the > odp_packet_parse function at this moment, simply because I don't see a > major problem with leaving that function as it is for the moment and > concentrate on fixing the problem with netmap. It's not ideal, but we've > been arguing on this issue for quite some time now and frankly I would > prefer if you and Petri reached a common ground soon. Isn't this something that should be defined in the architectural document? Cheers, Anders > > /Ciprian > > > > > >>> Last I don't think we should call debug macros if these would should > >>> occur and be detected. These are typical cases where you just increase a > >>> statistic counter and possibly mark the packet descriptor (typical HW > >>> and/or driver level responsibilities). That's all. But in these specific > >>> cases I think we should do nothing. > >>> > >>> + if (odp_unlikely(payload_len > > >>> pkt_nm->max_frame_len)) { > >>> ODP_ERR("Data partially lost %u %lu!\n", > >>> payload_len, > >>> pkt_nm->max_frame_len); > >>> payload_len = pkt_nm->max_frame_len; > >>> + } else if (odp_unlikely(payload_len < > >>> ODP_ETH_LEN_MIN)) { > >>> + if (odp_unlikely(pkt_nm->netmap_mode != > >>> ODP_NETMAP_MODE_SW)) > >>> + ODP_DBG("Frame appears to be > >>> truncated: %u\n", > >>> + (unsigned)payload_len); > >>> + payload_len = ODP_ETH_LEN_MIN; > >>> > >>> > >>> On 1 April 2014 12:39, Ciprian Barbu <ciprian.barbu@linaro.org <mailto: > >>> ciprian.barbu@linaro.org>> wrote: > >>> > >>> This patch sets the minimum eth frame len for packets arrived from > >>> netmap. > >>> Additionally, if the packet was not received from the SW ring a > >>> debug message > >>> is printed. > >>> > >>> Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org > >>> <mailto:ciprian.barbu@linaro.org>> > >>> > >>> --- > >>> platform/linux-generic/include/odp_packet_netmap.h | 5 +---- > >>> platform/linux-generic/source/odp_packet_netmap.c | 10 ++++++++-- > >>> 2 files changed, 9 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/platform/linux-generic/include/odp_packet_netmap.h > >>> b/platform/linux-generic/include/odp_packet_netmap.h > >>> index 960ccbf..9d5fb88 100644 > >>> --- a/platform/linux-generic/include/odp_packet_netmap.h > >>> +++ b/platform/linux-generic/include/odp_packet_netmap.h > >>> @@ -34,12 +34,9 @@ typedef struct { > >>> struct nm_desc_t *nm_desc; > >>> struct netmap_ring *rxring; > >>> struct netmap_ring *txring; > >>> - > >>> - /********************************/ > >>> odp_queue_t tx_access; /* Used for exclusive access to > >>> send packets */ > >>> uint32_t if_flags; > >>> - uint32_t if_reqcap; > >>> - uint32_t if_curcap; > >>> + int netmap_mode; > >>> char ifname[32]; > >>> } pkt_netmap_t; > >>> > >>> diff --git a/platform/linux-generic/source/odp_packet_netmap.c > >>> b/platform/linux-generic/source/odp_packet_netmap.c > >>> index 1cbd84c..2d459b9 100644 > >>> --- a/platform/linux-generic/source/odp_packet_netmap.c > >>> +++ b/platform/linux-generic/source/odp_packet_netmap.c > >>> @@ -131,6 +131,7 @@ int setup_pkt_netmap(pkt_netmap_t * const > >>> pkt_nm, char *netdev, > >>> pkt_nm->buf_size = odp_buffer_size(buf); > >>> /* max frame len taking into account the l2-offset */ > >>> pkt_nm->max_frame_len = pkt_nm->buf_size - pkt_nm->l2_offset; > >>> + pkt_nm->netmap_mode = nm_params->netmap_mode; > >>> > >>> odp_buffer_free(buf); > >>> > >>> @@ -293,10 +294,15 @@ int recv_pkt_netmap(pkt_netmap_t * const > >>> pkt_nm, odp_packet_t pkt_table[], > >>> rxring->head = nm_ring_next(rxring, cur); > >>> rxring->cur = nm_ring_next(rxring, cur); > >>> > >>> - if (payload_len > pkt_nm->max_frame_len) { > >>> + if (odp_unlikely(payload_len > > >>> pkt_nm->max_frame_len)) { > >>> ODP_ERR("Data partially lost %u > >>> %lu!\n", > >>> payload_len, > >>> pkt_nm->max_frame_len); > >>> payload_len = pkt_nm->max_frame_len; > >>> + } else if (odp_unlikely(payload_len < > >>> ODP_ETH_LEN_MIN)) { > >>> + if > >>> (odp_unlikely(pkt_nm->netmap_mode != ODP_NETMAP_MODE_SW)) > >>> + ODP_DBG("Frame appears to > >>> be truncated: %u\n", > >>> + (unsigned)payload_len); > >>> + payload_len = ODP_ETH_LEN_MIN; > >>> } > >>> > >>> pkt_buf = odp_packet_buf_addr(pkt); > >>> @@ -361,7 +367,7 @@ int send_pkt_netmap(pkt_netmap_t * const > >>> pkt_nm, odp_packet_t pkt_table[], > >>> txbuf = NETMAP_BUF(txring, slot->buf_idx); > >>> > >>> pkt = pkt_table[i]; > >>> - frame = odp_packet_l2(pkt); > >>> + frame = odp_packet_start(pkt); > >>> frame_len = odp_packet_get_len(pkt); > >>> > >>> memcpy(txbuf, frame, frame_len); > >>> -- > >>> 1.8.3.2 > >>> > >>> -- > >>> 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/ > >>> 1396348796-43137-1-git-send-email-ciprian.barbu%40linaro.org. > >>> 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/CAPiYAf41gau3Aj50DTraNz1H1zMAp > >>> 3ciJ33B5U3ZkVjn-F3j4Q%40mail.gmail.com <https://groups.google.com/a/ > >>> linaro.org/d/msgid/lng-odp/CAPiYAf41gau3Aj50DTraNz1H1zMAp > >>> 3ciJ33B5U3ZkVjn-F3j4Q%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/CAPiYAf5qS39o0vnQZPwNTTTwQSznNKTTfYPyeMCD4KwOHrYotg%40mail.gmail.com<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CAPiYAf5qS39o0vnQZPwNTTTwQSznNKTTfYPyeMCD4KwOHrYotg%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/CADJJ_HYUj_E3XJyc4%2BBD7Lz4N8A-08DWc1vxW9TKQEicO%2BBb4Q%40mail.gmail.com. > For more options, visit https://groups.google.com/a/linaro.org/d/optout.
diff --git a/platform/linux-generic/include/odp_packet_netmap.h b/platform/linux-generic/include/odp_packet_netmap.h index 960ccbf..9d5fb88 100644 --- a/platform/linux-generic/include/odp_packet_netmap.h +++ b/platform/linux-generic/include/odp_packet_netmap.h @@ -34,12 +34,9 @@ typedef struct { struct nm_desc_t *nm_desc; struct netmap_ring *rxring; struct netmap_ring *txring; - - /********************************/ odp_queue_t tx_access; /* Used for exclusive access to send packets */ uint32_t if_flags; - uint32_t if_reqcap; - uint32_t if_curcap; + int netmap_mode; char ifname[32]; } pkt_netmap_t; diff --git a/platform/linux-generic/source/odp_packet_netmap.c b/platform/linux-generic/source/odp_packet_netmap.c index 1cbd84c..2d459b9 100644 --- a/platform/linux-generic/source/odp_packet_netmap.c +++ b/platform/linux-generic/source/odp_packet_netmap.c @@ -131,6 +131,7 @@ int setup_pkt_netmap(pkt_netmap_t * const pkt_nm, char *netdev, pkt_nm->buf_size = odp_buffer_size(buf); /* max frame len taking into account the l2-offset */ pkt_nm->max_frame_len = pkt_nm->buf_size - pkt_nm->l2_offset; + pkt_nm->netmap_mode = nm_params->netmap_mode; odp_buffer_free(buf); @@ -293,10 +294,15 @@ int recv_pkt_netmap(pkt_netmap_t * const pkt_nm, odp_packet_t pkt_table[], rxring->head = nm_ring_next(rxring, cur); rxring->cur = nm_ring_next(rxring, cur); - if (payload_len > pkt_nm->max_frame_len) { + if (odp_unlikely(payload_len > pkt_nm->max_frame_len)) { ODP_ERR("Data partially lost %u %lu!\n", payload_len, pkt_nm->max_frame_len); payload_len = pkt_nm->max_frame_len; + } else if (odp_unlikely(payload_len < ODP_ETH_LEN_MIN)) { + if (odp_unlikely(pkt_nm->netmap_mode != ODP_NETMAP_MODE_SW)) + ODP_DBG("Frame appears to be truncated: %u\n", + (unsigned)payload_len); + payload_len = ODP_ETH_LEN_MIN; } pkt_buf = odp_packet_buf_addr(pkt); @@ -361,7 +367,7 @@ int send_pkt_netmap(pkt_netmap_t * const pkt_nm, odp_packet_t pkt_table[], txbuf = NETMAP_BUF(txring, slot->buf_idx); pkt = pkt_table[i]; - frame = odp_packet_l2(pkt); + frame = odp_packet_start(pkt); frame_len = odp_packet_get_len(pkt); memcpy(txbuf, frame, frame_len);
This patch sets the minimum eth frame len for packets arrived from netmap. Additionally, if the packet was not received from the SW ring a debug message is printed. Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> --- platform/linux-generic/include/odp_packet_netmap.h | 5 +---- platform/linux-generic/source/odp_packet_netmap.c | 10 ++++++++-- 2 files changed, 9 insertions(+), 6 deletions(-)