Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/pktio/loop.c
@@ -177,13 +177,15 @@ static int loopback_send(pktio_entry_t *pktio_entry, int
index ODP_UNUSED,
int i;
int ret;
uint32_t bytes = 0;
+ uint32_t out_octets_tbl[len];
if (odp_unlikely(len > QUEUE_MULTI_MAX))
len = QUEUE_MULTI_MAX;
for (i = 0; i < len; ++i) {
hdr_tbl[i] = packet_to_buf_hdr(pkt_tbl[i]);
bytes += odp_packet_len(pkt_tbl[i]);
+ out_octets_tbl[i] = bytes;
Comment:
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_r162431372
updated_at 2018-01-18 18:36:56
