Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/pktio/socket_mmap.c
line 64
@@ -202,6 +202,12 @@ static inline unsigned pkt_mmap_v2_rx(pktio_entry_t
*pktio_entry,
pkt_buf = (uint8_t *)ppd.raw + ppd.v2->tp_h.tp_mac;
pkt_len = ppd.v2->tp_h.tp_snaplen;
+ if (odp_unlikely(pkt_len > pkt_sock->mtu)) {
Comment:
At this point the API is quite clear:
> Maximum frame length in bytes that the packet IO interface can receive.
> JannePeltonen wrote
> Why do these extra long packets need to be dropped?
>
> Maybe odp_pktin_maxlen() is not defined clearly enough regarding unexpectedly
> long packets. Does it merely promise that packets up to the maxlen can always
> be received (as I thought) or does it also promise that no bigger packets
> than that ever come out of the pktin? Would the latter be even useful?
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Ok, clever. A comment might be useful here.
>>> Matias Elo(matiaselo) wrote:
>>> Problem is that you cannot increase drop counters manually with socket
>>> pktio. The counters are read directly from ethtool or sysfs. ODP_DBG() may
>>> be the best option here.
>>>> muvarov wrote
>>>> inconsistent with counters. You do not increase drop counter for pktio
>>>> and silently drop packet. I would add at least ODP_DBG() so people on
>>>> debug can see what is happening.
>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>> OK. From the original code's use of `%` I assumed that was the case.
>>>>> Perhaps that's why this is a fix. :)
>>>>>> Matias Elo(matiaselo) wrote:
>>>>>> Will fix.
>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>> ring->rd_num is not guaranteed to be a power of two. At least on my
>>>>>>> test system (28 lcores) it is not.
>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>> Will fix.
>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>> This array is used to update `stats.out_octets` correctly if
>>>>>>>>> `enq_multi()` fails to enqueue all packets. Without it a second for
>>>>>>>>> loop would be required inside `if (ret > 0)` clause below.
>>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>>> The most specific error type an application can see is
>>>>>>>>>> odp_packet_has_l2_error(), so it cannot trust packet contents anyway.
>>>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>>>> Will fix.
>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>> The error bit simply says the packet was truncated to max length.
>>>>>>>>>>>> Perhaps the application is only interested in the first few bytes
>>>>>>>>>>>> of a packet?
>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>> `ring->frame_num = next_frame(first_frame_num + nb_tx - 1,
>>>>>>>>>>>>> frame_count);` is an alternative here.
>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>> Since `ring->rd_num` is a power of two, is the concern here that
>>>>>>>>>>>>>> the `%` operator might result in a division if the compiler
>>>>>>>>>>>>>> doesn't know that it's a power of two? In that case:
>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>> return ++cur_frame & (frame_count - 1);
>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>> solves that problem and avoids conditional branching.
>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>> If we're cleaning up, might as well delete that extraneous `+`
>>>>>>>>>>>>>>> in the above line: `TPACKET_ALIGNMENT + + (pz - 1)) & (-pz);`
>>>>>>>>>>>>>>> I'm surprised the compiler doesn't flag that since it's
>>>>>>>>>>>>>>> normally so picky about issuing warnings.
>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>> Not sure I understand what `out_octets_tbl` is accumulating
>>>>>>>>>>>>>>>> here. If I'm sending three 100 byte packets this will set
>>>>>>>>>>>>>>>> `out_octets_tbl[0]` to 100, `out_octets_tbl[1]` to 200, and
>>>>>>>>>>>>>>>> `out_octets_tbl[2]` to 300.
>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>> Since you're doing miscellaneous cleanups anyway, perhaps
>>>>>>>>>>>>>>>>> changing `len` to `num` here should be considered? `len` is
>>>>>>>>>>>>>>>>> confusing since it doesn't represent packet length.
>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>>>>>>>>>>> The odp_pktin_maxlen() API states:
>>>>>>>>>>>>>>>>>> > Maximum frame length in bytes that the packet IO interface
>>>>>>>>>>>>>>>>>> > can receive.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I interpret this so that the application shouln't see
>>>>>>>>>>>>>>>>>> packets which exceed pktin max length.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> muvarov wrote
>>>>>>>>>>>>>>>>>>> same here.
>>>>>>>>>>>>>>>>>>>> muvarov wrote
>>>>>>>>>>>>>>>>>>>> maybe to just set error bit and deliver this packet to
>>>>>>>>>>>>>>>>>>>> application?
https://github.com/Linaro/odp/pull/397#discussion_r162599656
updated_at 2018-01-19 11:33:00
