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