Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/odp_packet_io.c
@@ -1577,10 +1577,27 @@ 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;
+
+ if (wait == ODP_PKTIN_WAIT)
+ usecs = ODP_PKTIN_WAIT;
+
+ return entry->s.ops->recv_tmo(entry, queue.index, packets, num,
+ usecs);
Comment:
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_r157419588
updated_at 2017-12-18 10:59:06
