Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/pktio/netmap.c
line 95
@@ -786,6 +824,78 @@ static int netmap_recv(pktio_entry_t *pktio_entry, int
index,
return num_rx;
}
+static int netmap_recv_tmo(pktio_entry_t *pktio_entry, int index,
+ odp_packet_t pkt_table[], int num, uint64_t usecs)
+{
+ struct timeval timeout;
+ int ret;
+ int maxfd;
+ fd_set readfds;
+
+ timeout.tv_sec = usecs / 1000 / 1000;
+ timeout.tv_usec = usecs % (1000 * 1000);
+ FD_ZERO(&readfds);
+ maxfd = netmap_fd_set(pktio_entry, index, &readfds);
+
+ ret = netmap_recv(pktio_entry, index, pkt_table, num);
+ if (ret != 0)
+ return ret;
+
+ if (select(maxfd + 1, &readfds, NULL, NULL,
+ usecs == ODP_PKTIN_WAIT ? NULL : &timeout) == 0)
+ return 0;
+
+ return netmap_recv(pktio_entry, index, pkt_table, num);
+}
+
+static int netmap_recv_mq_tmo(pktio_entry_t *pktio_entry[], int index[],
+ int num_q, odp_packet_t pkt_table[], int num,
+ unsigned *from, uint64_t usecs)
+{
+ struct timeval timeout;
+ int i;
+ int ret;
+ int maxfd = -1, maxfd2;
+ fd_set readfds;
+
+ timeout.tv_sec = usecs / 1000 / 1000;
+ timeout.tv_usec = usecs % (1000 * 1000);
+
+ FD_ZERO(&readfds);
+
+ for (i = 0; i < num_q; i++) {
+ maxfd2 = netmap_fd_set(pktio_entry[i], index[i], &readfds);
+ if (maxfd2 > maxfd)
+ maxfd = maxfd2;
+ }
+
+ for (i = 0; i < num_q; i++) {
+ ret = netmap_recv(pktio_entry[i], index[i], pkt_table, num);
Comment:
Since this loop is a performance optimization I would move it before the loop
which sets the read descriptors. No use to set the descriptors if packets are
already available.
> Matias Elo(matiaselo) wrote:
> 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_r157676244
updated_at 2017-12-19 07:00:31
