Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/odp_packet_io.c
line 16
@@ -1577,10 +1577,29 @@ int odp_pktin_recv_tmo(odp_pktin_queue_t queue,
odp_packet_t packets[], int num,
odp_time_t t1, t2;
struct timespec ts;
int started = 0;
+ pktio_entry_t *entry;
ts.tv_sec = 0;
ts.tv_nsec = SLEEP_NSEC;
+ entry = get_pktio_entry(queue.pktio);
+ if (entry == NULL) {
+ ODP_DBG("pktio entry %d does not exist\n", queue.pktio);
+ return -1;
+ }
+
+ if (entry->s.ops->recv_tmo) {
+ uint64_t usecs = (wait * SLEEP_NSEC + 999) / 1000;
Comment:
The motivation for a separate `odp_pktin_wait_time()` function was to avoid
having to do time conversions in the fast path functions. As
`odp_pktin_wait_time()` already returns the time in microseconds no additional
conversions are needed here. Same thing for `odp_pktin_recv_mq_tmo()`.
Currently, the value returned by `odp_pktin_recv_mq_tmo()` depends on
`SLEEP_NSEC` definition. This should be modified so that the returned value is
always in microseconds.
> Matias Elo(matiaselo) wrote:
> Every pktio type we currenly have uses microseconds for timeout, so you could
> change `recv_tmo()` and `recv_mq_tmo()` to use microseconds instead of
> nanoseconds. This removes the need for per operation conversions.
>> Matias Elo(matiaselo) wrote:
>> > Are you entirely sure about this? This is a function pointer, and if two
>> > pktio devices have the same function pointer for recv_mq_tmo(), the other
>> > function pointers including the recv() function pointer should be
>> > identical as well. We do pass the device (entry[i]) to the function
>> > pointed to by recv_mq_tmo as well, so it should be able to update the
>> > correct counters. The ops structure is const, after all...
>>
>> I stand corrected, didn't notice the pktio_entry_t array parameter. A bit
>> confusing as `recv_mq_tmo()` behaves differently than pretty much every
>> other pktio op.
>>> muvarov wrote
>>> just pass nsec to drivers or you will have double calculation like in
>>> netmap code.
>>>> Juha-Matti Tilli(jmtilli) wrote:
>>>> Are you entirely sure about this? This is a function pointer, and if two
>>>> pktio devices have the same function pointer for recv_mq_tmo(), the other
>>>> function pointers including the recv() function pointer should be
>>>> identical as well. We do pass the device (entry[i]) to the function
>>>> pointed to by recv_mq_tmo as well, so it should be able to update the
>>>> correct counters. The ops structure is const, after all...
>>>>
>>>> So, when receiving packets from a particular device using netmap, we do
>>>> call netmap_recv() and pass to it the pktio_entry[i] and also index[i] for
>>>> the queue index. This is the same that the odp_pktin_recv_mq_tmo()
>>>> function already did. It calls odp_pktin_recv() in a loop, and the latter
>>>> function calls entry->s.ops->recv(entry, queue.index, packets, num); which
>>>> to me seems to be exactly the same thing -- you pass exactly the same
>>>> entry and the same queue.index. Same function, same arguments, I can't see
>>>> how the behavior would be different...
>>>>
>>>> Now, if somebody opposes the .recv_mq_tmo function pointer due to added
>>>> code line count, I might perhaps agree. But it seems to be the only way
>>>> the DPDK support for genuine sleep could be added, so I see some value in
>>>> it.
>>>>> Juha-Matti Tilli(jmtilli) wrote:
>>>>> Several options: (1) I could implement a binary search algorithm that
>>>>> incrementally checks various usec values and uses odp_pktin_wait_time()
>>>>> to check which one of these trial attempts result in the same sleep wait
>>>>> value, but that would be slow; (2) I could implement
>>>>> odp_pktin_wait_time_reverse() that does the conversion in the other
>>>>> direction, but that would be an API change; (3) I could just do the
>>>>> conversion and pass usec values instead of sleep wait values. I carefully
>>>>> considered each and selected (3), but do you think (2) or (1) would be
>>>>> better?
>>>>>> Juha-Matti Tilli(jmtilli) wrote:
>>>>>> Will fix.
>>>>>>> Juha-Matti Tilli(jmtilli) wrote:
>>>>>>> I disagree. SLEEP_NSEC is a private #define in odp_packet_io.c so what
>>>>>>> the wait parameter actually means cannot be interpreted in the drivers.
>>>>>>> (Believe me, I tried it!) Thus, the conversion from the sleep wait
>>>>>>> parameter to microseconds is needed.
>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>> As I noted in the previous comment, I think this check isn't enough.
>>>>>>>> The input queues would have to be from the same pktio device for this
>>>>>>>> to work.
>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>> Internal functions should not use odp_ prefix.
>>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>>> You should pass the `wait` parameter to `entry->s.ops->recv_tmo()`
>>>>>>>>>> and not do any conversions here.
>>>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>>>> ODP_TIME_SEC_IN_NS would work here.
https://github.com/Linaro/odp/pull/341#discussion_r157674172
updated_at 2017-12-19 06:40:39
