[dpdk-dev] [PATCH v5 00/15] common ethdev linkstatus functions

[Ferruh Yigit]

On 1/19/2018 4:35 PM, Ferruh Yigit wrote:
> On 1/17/2018 4:18 PM, Ferruh Yigit wrote:
>> On 1/17/2018 4:05 PM, Stephen Hemminger wrote:
>>> On Wed, 17 Jan 2018 14:32:17 +0000
>>> Ferruh Yigit <ferruh.yigit at intel.com> wrote:
>>>
>>>> On 1/17/2018 7:56 AM, Andrew Rybchenko wrote:
>>>>> On 01/16/2018 09:37 PM, Stephen Hemminger wrote:  
>>>>>> While reviewing drivers, noticed a lot of unnecessary
>>>>>> duplication of code in drivers for handling the eth_dev link status
>>>>>> information. While consolidating this, it also became obvious that
>>>>>> some drivers behave differently for no good reason.
>>>>>>
>>>>>> It also was a good chance to introduce atomic exchange primitives
>>>>>> in EAL because there are other places using cmpset where not
>>>>>> necessary (such as bonding).
>>>>>>
>>>>>> Mostly only compile tested only, don't have all of the hardware
>>>>>> available (except ixgbe and virtio) to test.
>>>>>>
>>>>>> Note: the eth_dev_link_update function return value is inconsistent
>>>>>> across drivers. Should be changed to be void.  
>>>>>
>>>>> I would say "link_update" callback return value is inconsistent across
>>>>> drivers. I'm not sure which direction is right here: make it consistent
>>>>> or make it void. Also any changes in link information could be
>>>>> important. As I understand it should not happen without up/down,
>>>>> but bugs with loss of intermediate transitions are definitely possible.
>>>>> So, notifying about any changes in link information is definitely safer.
>>>>> May be not now.  
>>>>
>>>> Again, why not return previous link status, it is simple enough to prevent
>>>> inconsistent usage.
>>>>
>>>> rte_eth_link_get() already discards the return value, so won't be a problem there.
>>>>
>>>> For the cases PMD would like know about link changes, they will need to
>>>> implement almost same link_update function with a return value, so why not use
>>>> existing link_update function?
>>>>
>>>> Like been in virtio, link_update() used in interrupt handler, and calls a
>>>> callback process if status changes. When link_update() return status changed to
>>>> void, I guess they will need to implement another version of the link_update
>>>> with return and use it.
>>>
>>> The interrupt and non-interrupt model are different.
>>
>> Yes. But for virtio specific usage:
>>
>> virtio_interrupt_handler()
>>   virtio_dev_link_update() == 0
>>     _rte_eth_dev_callback_process()
>>
>> meantime same exact virtio_dev_link_update() used as:
>>   .link_update             = virtio_dev_link_update,
>>
>> so updating virtio_dev_link_update() to not return status change, will update
>> logic in virtio_interrupt_handler(), no?
> 
> I would like to see this patch in, because it is useful and almost done. The
> concern I mentioned above effects virtio.

Let's go step by step. I will update patchset to keep same behavior in drivers
but switch to new APIs to prevent code duplication.

In next step we can define the return value and implement missing PMDs.

> 
> Can virtio maintainers check if it is OK to get this as it is please?
> 
>>
>>> Also the driver internally may want to do something different, this is about
>>> the return value for dev_ops->link_update.  
>>
>> Agreed, driver may do something different. And the function needs to be
>> implemented will be very close to dev_ops->link_update. I thought making
>> dev_ops->link_update more generic can prevent duplication there. And aligns with
>> virtio usage..
>>
>>> The code in rte_eth_dev never
>>> used the return value. The bonding driver was expecting it to work but it
>>> doesn't.
>>
>> Agreed.
>>
>>> Anyway drivers shouldn't in general be directly calling other
>>> devices eth_dev_ops
>>
>> I guess now there are a few overlay PMDs does this.
>>
>