Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/pktio/socket.c
line 60
@@ -693,6 +693,88 @@ static int sock_mmsg_recv(pktio_entry_t *pktio_entry, int
index ODP_UNUSED,
return nb_rx;
}
+static int sock_fd_set(pktio_entry_t *pktio_entry, int index ODP_UNUSED,
+ fd_set *readfds)
+{
+ pkt_sock_t *pkt_sock = &pktio_entry->s.pkt_sock;
+ const int sockfd = pkt_sock->sockfd;
+
+ FD_SET(sockfd, readfds);
+ return sockfd;
+}
+
+static int sock_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_fd_set(pktio_entry, index, &readfds);
+
+ ret = sock_mmsg_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_mmsg_recv(pktio_entry, index, pkt_table, num);
+}
+
+static int sock_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_fd_set(pktio_entry[i], index[i], &readfds);
+ if (maxfd2 > maxfd)
+ maxfd = maxfd2;
+ }
+
+ for (i = 0; i < num_q; i++) {
+ ret = sock_mmsg_recv(pktio_entry[i], index[i], pkt_table, num);
Comment:
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_r157676448
updated_at 2017-12-19 07:01:30
