JannePeltonen 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:
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_r162593060
updated_at 2018-01-19 11:27:24
