Juha-Matti Tilli(jmtilli) replied on github web page: platform/linux-generic/odp_packet_io.c @@ -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: I'm somewhat worried that somebody could adjust SLEEP_NSEC and then the entire thing fails. What about this: <pre> #if SLEEP_NSEC == 1000 uint64_t usecs = wait; #else uint64_t usecs = (wait * SLEEP_NSEC + 999) / 1000; #endif </pre> For the return value, you mean <code>odp_pktin_wait_time()</code> and not <code>odp_pktin_recv_mq_tmo()</code>, right? Because I don't see how the return value of the latter could depend on <code>SLEEP_NSEC</code> definition. Also, if we modify <code>odp_pktin_wait_time()</code> to always return microseconds, then <code>odp_pktin_recv_mq_tmo()</code> suddenly starts to work incorrectly with <code>SLEEP_NSEC != 1000</code> as there is a <code>wait--</code> line in there that does this just before sleeping one microsecond, exactly. > Matias Elo(matiaselo) wrote: > 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_r157748580 updated_at 2017-12-19 13:17:09