Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/pktio/dpdk.c
line 70
@@ -1372,10 +1382,10 @@ static int dpdk_recv(pktio_entry_t *pktio_entry, int
index,
if (!pkt_dpdk->lockless_rx)
odp_ticketlock_lock(&pkt_dpdk->rx_lock[index]);
/**
- * ixgbe_pmd has a minimum supported RX burst size ('min_rx_burst'). If
- * 'num' < 'min_rx_burst', 'min_rx_burst' is used as rte_eth_rx_burst()
- * argument and the possibly received extra packets are cached for the
- * next dpdk_recv_queue() call to use.
+ * ixgbe and i40e drivers have a minimum supported RX burst size
Comment:
@nagarahalli Where do you see these kinds of checks? As far as I can see this
is only relevant to dpdk pktio and if there are similar checks elsewhere they
should probably be removed.
I agree that this issue should be discussed outside of this PR as these patches
are only simple bug fixes to the current implementation.
> nagarahalli wrote
> @matiaselo I see these kind of validations in data plane functions at
> multiple places. If we combine all of them, it is significant improvement (I
> see 1% improvement in Linux-dpdk/l2fwd application by removing all the
> validation checks in pkt I/O module alone). But on an individual basis, you
> will not see much of a difference.
>
> It might be few device drivers, but from ODP perspective, it is a capability
> of the device that should be exposed through capability API. We could do it
> outside of this PR as well.
>> Matias Elo(matiaselo) wrote:
>> @nagarahalli I don't see a new capability parameter being very useful as
>> these two dpdk drivers are likely the only devices which would use it. At
>> least we haven't come across any other network devices which have this kind
>> of limitation.
>>
>> I ran a quick test and the two additional if statements caused by rx caching
>> have a minuscule performance impact.
>>> nagarahalli wrote
>>> @matiaselo Agree on the simple applications. But for real applications or
>>> for performance benchmarking, this adds additional cycles. If this is made
>>> as part of capability, application can decide the burst size upfront.
>>>> Matias Elo(matiaselo) wrote:
>>>> @lumag That would be the best solution but I don't see it happening
>>>> anytime soon. The number of rx queues returned by rte_eth_dev_info_get()
>>>> is in principle correct. Here the limiting factor is RSS capability and as
>>>> far as I can see there is no way to query this information.
>>>>> Matias Elo(matiaselo) wrote:
>>>>> I'm not sure I got your point but anyway dev_info is type 'struct
>>>>> rte_eth_dev_info' and comes from dpdk, so nothing can be added there.
>>>>>
>>>>> As @lumag noted this fix is required because of a dpdk defect. With ixgbe
>>>>> drivers rte_eth_dev_info_get() returns a max_rx_queues value which is not
>>>>> valid (at least when using RSS). This is the only driver a have observer
>>>>> this issue with.
>>>>>
>>>>> The number 16 is explained in the comment line (1139) just above.
>>>>>> Matias Elo(matiaselo) wrote:
>>>>>> @Bill-Fischofer-Linaro Yes, the application has to request at least
>>>>>> 'min_rx_burst' packets but it may receive less.
>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>> @nagarahalli This dpdk driver "feature" can be completely hidden from
>>>>>>> odp applications (and it already is), so I don't support adding a new
>>>>>>> device capability parameter. This would make writing simple
>>>>>>> applications more complex. E.g. an application wouldn't be able to
>>>>>>> receive one packet at a time anymore.
>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>> Again, shouldn't this sort of configuration be a field in the
>>>>>>>> `dev_info` struct rather than having to be special-cased for each
>>>>>>>> driver? Also, must 16 be a "magic number" here?
>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>> It looks like this should be fixed inside DPDK rather than here.
>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>> Presumably these drivers have a way of flushing short bursts? If the
>>>>>>>>>> `min_rx_burst` is 4 and the other side sends an odd number of
>>>>>>>>>> packets, presumably the receiver application isn't left hanging
>>>>>>>>>> forever waiting for the rest of a burst that will never arrive?
>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>> Wouldn't it be simpler (and more extensible) to have `min_rx_burst`
>>>>>>>>>>> as a field in the `dev_info` struct?
>>>>>>>>>>>> nagarahalli wrote
>>>>>>>>>>>> 'min_rx_burst' should be added to the capability as it is a
>>>>>>>>>>>> restriction from the device. The application should adjust the
>>>>>>>>>>>> 'num' according to the capability it reads from the pkt I/O.
https://github.com/Linaro/odp/pull/287#discussion_r150487575
updated_at 2017-11-13 09:46:55
