Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/pktio/socket_mmap.c
line 71
@@ -614,6 +628,78 @@ static int sock_mmap_recv(pktio_entry_t *pktio_entry, int
index ODP_UNUSED,
return ret;
}
+static int sock_mmap_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 = sock_mmap_fd_set(pktio_entry, index, &readfds);
+
+ ret = sock_mmap_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 sock_mmap_recv(pktio_entry, index, pkt_table, num);
+}
+
+static int sock_mmap_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 = sock_mmap_fd_set(pktio_entry[i], index[i], &readfds);
+ if (maxfd2 > maxfd)
+ maxfd = maxfd2;
+ }
+
+ for (i = 0; i < num_q; i++) {
+ ret = sock_mmap_recv(pktio_entry[i], index[i], pkt_table, num);
Comment:
Same thing with the loop order than with netmap.
> Matias Elo(matiaselo) wrote:
> Couple comment lines describing the different stages of this function
> wouldn't hurt when someone is reading this code later on.
>> Matias Elo(matiaselo) wrote:
>> Same thing with the loop order than with netmap.
>>> Matias Elo(matiaselo) wrote:
>>> 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_r157677836
updated_at 2017-12-19 07:13:32
