Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/pktio/socket_mmap.c
line 30
@@ -148,6 +155,11 @@ static uint8_t *pkt_mmap_vlan_insert(uint8_t *l2_hdr_ptr,
return l2_hdr_ptr;
}
+static inline unsigned next_frame(unsigned cur_frame, unsigned frame_count)
+{
+ return odp_unlikely(cur_frame + 1 >= frame_count) ? 0 : cur_frame + 1;
Comment:
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_r162334802
updated_at 2018-01-18 13:02:31
