Solarflare PMD submission question

Solarflare PMD submission question

[Andrew Rybchenko]

Hi,

we would like to include Solarflare libefx-based PMD in the DPDK 17.02 
and start the upstreaming process.
The driver supports Solarflare SFN7xxx and SFN8xxx families of 10/40 
Gbps adapters.
The driver has base driver. It is just fresh version of the same code 
which is used in the FreeBSD [1], illumos [2] and some other Solarflare 
drivers.
The question is how to submit the base driver which is pretty big. Mail 
size of the patch which imports it is about 2 Mb.
Further changes in the base driver will go in small patches (as it is 
done, for example, in the FreeBSD).
The PMD itself is split into small and, I hope, readable and nice patches.

[1] https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/
[2] 
https://github.com/illumos/illumos-gate/tree/master/usr/src/uts/common/io/sfxge/common/

Andrew.

Re: Solarflare PMD submission question

[Thomas Monjalon]

Hi,

First of all, welcome to DPDK!

2016-10-27 09:34, Andrew Rybchenko:
> Hi,
> 
> we would like to include Solarflare libefx-based PMD in the DPDK 17.02 
> and start the upstreaming process.
> The driver supports Solarflare SFN7xxx and SFN8xxx families of 10/40 
> Gbps adapters.
> The driver has base driver. It is just fresh version of the same code 
> which is used in the FreeBSD [1], illumos [2] and some other Solarflare 
> drivers.

Unfortunately it is common to have some big base drivers in DPDK.
Note that some PMD rely on their kernel counterpart for the control path.
It is a way to avoid code duplication.
As far as I understand, it is easier to share queues with DPDK from kernel
when the device supports an IOMMU.

> The question is how to submit the base driver which is pretty big. Mail 
> size of the patch which imports it is about 2 Mb.

First answer is a question:
Have you thought about cooperating with the kernel driver for your PMD?
If you really cannot use this approach, then we have to maintain this
whole base driver in DPDK.
It will be easier to read, understand and reference if it is a bit split.
Could you try to send it as 10 to 20 patches explaining the role of each
part and giving some design details?

It would be also really appreciated to provide a design documentation
in doc/guides/nics. Are the datasheets open? A link in the doc would help.

> Further changes in the base driver will go in small patches (as it is 
> done, for example, in the FreeBSD).
> The PMD itself is split into small and, I hope, readable and nice patches.

Good to know. Thanks

Please be prepare to work on several iterations of the patch series.

PS: the mailing list put emails exceeding 300KB into a moderation queue.

Re: Solarflare PMD submission question

[Andrew Rybchenko]

Thomas,

On 10/27/2016 01:37 PM, Thomas Monjalon wrote:
> First of all, welcome to DPDK!

Thanks!

> 2016-10-27 09:34, Andrew Rybchenko:
>> we would like to include Solarflare libefx-based PMD in the DPDK 17.02
>> and start the upstreaming process.
>> The driver supports Solarflare SFN7xxx and SFN8xxx families of 10/40
>> Gbps adapters.
>> The driver has base driver. It is just fresh version of the same code
>> which is used in the FreeBSD [1], illumos [2] and some other Solarflare
>> drivers.
> Unfortunately it is common to have some big base drivers in DPDK.
> Note that some PMD rely on their kernel counterpart for the control path.
> It is a way to avoid code duplication.

Linux kernel sfc driver has control path functionality, but technically
it is a different code.

> As far as I understand, it is easier to share queues with DPDK from kernel
> when the device supports an IOMMU.
>
>> The question is how to submit the base driver which is pretty big. Mail
>> size of the patch which imports it is about 2 Mb.
> First answer is a question:
> Have you thought about cooperating with the kernel driver for your PMD?

Yes, we considered it, but decided that we need pure userspace driver since
the approach has its advantages: no specific dependencies from kernel,
the same PMD for Linux and FreeBSD etc.

> If you really cannot use this approach, then we have to maintain this
> whole base driver in DPDK.
> It will be easier to read, understand and reference if it is a bit split.
> Could you try to send it as 10 to 20 patches explaining the role of each
> part and giving some design details?

First of all I'd like to double check that it is clear that we discuss 
libefx
(base driver in terms of DPDK) import here. The PMD itself is already split
in 20+ patches.
The only thing which comes to my mind is to split libefx import on subsystem
basis (few files per subsystem). It is artificial and added files will 
be abandoned
until the patch which adds them into build. It could be something like:
  1. External interfaces definition
  2. Internal interfaces definition
  3. Registers definition (hardware interface)
  4. Management CPU interface definition (it is one file, but still big 
650K)
  5. Management CPU interface implementation
and so on for NIC global controls, interrupts, event queue, transmit, 
receive,
  filtering etc.

> It would be also really appreciated to provide a design documentation
> in doc/guides/nics. Are the datasheets open? A link in the doc would help.

We have a documentation which grows together with supported features,
but it is rather for users. Important design decisions (not so many) are
documented nearby corresponding code. Unfortunately there is no open
datasheets. Management CPU interface definition has comments.

> Please be prepare to work on several iterations of the patch series.

We have already passed a number of iterations internally, so it will not 
frighten.

> PS: the mailing list put emails exceeding 300KB into a moderation queue.


Nice to know that it is not completely rejected, since even if we split as

described above, we still have one candidate which will end-up in moderation

queue.


Thanks,

Andrew.

Re: Solarflare PMD submission question

[Thomas Monjalon]

2016-10-28 13:50, Andrew Rybchenko:
> First of all I'd like to double check that it is clear that we discuss 
> libefx
> (base driver in terms of DPDK) import here. The PMD itself is already split
> in 20+ patches.

I don't know libefx. In DPDK, a base driver is often a subdirectory
inside the PMD. Will it be the case?

> The only thing which comes to my mind is to split libefx import on subsystem
> basis (few files per subsystem). It is artificial and added files will 
> be abandoned
> until the patch which adds them into build. It could be something like:
>   1. External interfaces definition
>   2. Internal interfaces definition
>   3. Registers definition (hardware interface)
>   4. Management CPU interface definition (it is one file, but still big 
> 650K)
>   5. Management CPU interface implementation
> and so on for NIC global controls, interrupts, event queue, transmit, 
> receive,
>   filtering etc.

Yes it is artificial.
The most valuable would be a transversal logical split, kind of feature
per feature, in order to explain how the device works.
Such commit is also the opportunity to explain acronyms and so on.

> > It would be also really appreciated to provide a design documentation
> > in doc/guides/nics. Are the datasheets open? A link in the doc would help.
> 
> We have a documentation which grows together with supported features,
> but it is rather for users. Important design decisions (not so many) are
> documented nearby corresponding code. Unfortunately there is no open
> datasheets. Management CPU interface definition has comments.

Without neither a datasheet, nor a comprehensive code introduction, it is
almost impossible to dive in your code. So it misses the point about bringing
it to an Open Source project.
Please do the the effort to bring some knowledge to the community.

Thanks

Re: Solarflare PMD submission question

[Andrew Rybchenko]

On 10/28/2016 03:33 PM, Thomas Monjalon wrote:
> 2016-10-28 13:50, Andrew Rybchenko:
>> First of all I'd like to double check that it is clear that we discuss
>> libefx
>> (base driver in terms of DPDK) import here. The PMD itself is already split
>> in 20+ patches.
> I don't know libefx. In DPDK, a base driver is often a subdirectory
> inside the PMD. Will it be the case?

Yes. Just to be absolutely sure: are the discussed requirements to split
applicable to base driver import?

Andrew.

Re: Solarflare PMD submission question

[Thomas Monjalon]

2016-10-28 16:05, Andrew Rybchenko:
> On 10/28/2016 03:33 PM, Thomas Monjalon wrote:
> > 2016-10-28 13:50, Andrew Rybchenko:
> >> First of all I'd like to double check that it is clear that we discuss
> >> libefx
> >> (base driver in terms of DPDK) import here. The PMD itself is already split
> >> in 20+ patches.
> > I don't know libefx. In DPDK, a base driver is often a subdirectory
> > inside the PMD. Will it be the case?
> 
> Yes. Just to be absolutely sure: are the discussed requirements to split
> applicable to base driver import?

Yes I'm talking about the base driver. But they are not some requirements.
Take them as advices.
The first priority is to welcome your new driver.
The second priority is to make sure that it is open and readable enough.

Re: Solarflare PMD submission question

[Andrew Rybchenko]

On 10/28/2016 03:33 PM, Thomas Monjalon wrote:
> 2016-10-28 13:50, Andrew Rybchenko:
>> The only thing which comes to my mind is to split libefx import on subsystem
>> basis (few files per subsystem). It is artificial and added files will
>> be abandoned
>> until the patch which adds them into build. It could be something like:
>>    1. External interfaces definition
>>    2. Internal interfaces definition
>>    3. Registers definition (hardware interface)
>>    4. Management CPU interface definition (it is one file, but still big
>> 650K)
>>    5. Management CPU interface implementation
>> and so on for NIC global controls, interrupts, event queue, transmit,
>> receive,
>>    filtering etc.
> Yes it is artificial.
> The most valuable would be a transversal logical split, kind of feature
> per feature, in order to explain how the device works.

I'm not the main author of the libefx and personally would consider it 
very useful.
 From the other hand I understand that it is a huge amount of work to 
make it.

> Such commit is also the opportunity to explain acronyms and so on.

Good. We'll go this way and 'll do my best to make it useful to understand
overall structure of the code and how the device works.

>>> It would be also really appreciated to provide a design documentation
>>> in doc/guides/nics. Are the datasheets open? A link in the doc would help.
>> We have a documentation which grows together with supported features,
>> but it is rather for users. Important design decisions (not so many) are
>> documented nearby corresponding code. Unfortunately there is no open
>> datasheets. Management CPU interface definition has comments.
> Without neither a datasheet, nor a comprehensive code introduction, it is
> almost impossible to dive in your code. So it misses the point about bringing
> it to an Open Source project.
> Please do the the effort to bring some knowledge to the community.

I've raised this internally and see what extra documentation we can 
provide to
the community. But this may take some time and I hope it is OK to post 
patches
in the interim. I use the management CPU interface (MCDI) definition 
mentioned
above when I add features. It is shared by all drivers: [1], [2], [3].

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/sfc/mcdi_pcol.h
[2] 
https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/efx_regs_mcdi.h?view=markup
[3] 
https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/io/sfxge/common/efx_regs_mcdi.h

Andrew.

Re: Solarflare PMD submission question

[Andrew Rybchenko]

On 10/28/2016 05:43 PM, Andrew Rybchenko wrote:
> On 10/28/2016 03:33 PM, Thomas Monjalon wrote:
>> 2016-10-28 13:50, Andrew Rybchenko:
>>> The only thing which comes to my mind is to split libefx import on 
>>> subsystem
>>> basis (few files per subsystem). It is artificial and added files will
>>> be abandoned
>>> until the patch which adds them into build. It could be something like:
>>>    1. External interfaces definition
>>>    2. Internal interfaces definition
>>>    3. Registers definition (hardware interface)
>>>    4. Management CPU interface definition (it is one file, but still 
>>> big
>>> 650K)
>>>    5. Management CPU interface implementation
>>> and so on for NIC global controls, interrupts, event queue, transmit,
>>> receive,
>>>    filtering etc.
>> Yes it is artificial.
>> The most valuable would be a transversal logical split, kind of feature
>> per feature, in order to explain how the device works.
>
> I'm not the main author of the libefx and personally would consider it 
> very useful.
> From the other hand I understand that it is a huge amount of work to 
> make it.
>
>> Such commit is also the opportunity to explain acronyms and so on.
>
> Good. We'll go this way and 'll do my best to make it useful to 
> understand
> overall structure of the code and how the device works.

Now we have a split of the base driver import in big feature steps. The 
base driver is split into 28 patches. Just only 1 patch exceeds 300K 
boundary (which add MCDI definitions header).

Before submitting 56 patches I'd like to double-check that checkpatch.pl 
errors (for example, because of assignments in the 'if' condition, 
parenthesis around return value) is not a show-stopper for base driver 
import.

Andrew.

Re: Solarflare PMD submission question

[Thomas Monjalon]

2016-11-18 19:50, Andrew Rybchenko:
> Now we have a split of the base driver import in big feature steps. The 
> base driver is split into 28 patches. Just only 1 patch exceeds 300K 
> boundary (which add MCDI definitions header).

Good

> Before submitting 56 patches I'd like to double-check that checkpatch.pl 
> errors (for example, because of assignments in the 'if' condition, 
> parenthesis around return value) is not a show-stopper for base driver 
> import.

You can run checkpatches.sh or send the patches to checkpatch@dpdk.org.
The script check-git-log.sh can also guide you for the expected formatting.

Re: Solarflare PMD submission question

[Andrew Rybchenko]

On 11/21/2016 11:19 AM, Thomas Monjalon wrote:
>> Before submitting 56 patches I'd like to double-check that checkpatch.pl
>> errors (for example, because of assignments in the 'if' condition,
>> parenthesis around return value) is not a show-stopper for base driver
>> import.
> You can run checkpatches.sh or send the patches to checkpatch@dpdk.org.
> The script check-git-log.sh can also guide you for the expected formatting.

Yes, I did it and it helped me to find and fix some coding standard 
violations.

The problem with libefx (base driver) is that it is existing code which 
follows FreeBSD and illumos coding conventions which contradict to 
checkpatches.sh sometimes (e.g. require parenthesis around return 
value). Other example of error produced by checkpatches.sh is assign in 
if. It is widely used in the code to assign return code value and 
compare it vs 0 in one line. It is not a coding standard conflict, but 
it is very wide-spread in the code (so changing it will produce too many 
changes not strictly required/useful).

So, may I repeat my question if it is a show-stopper for base driver or 
acceptable.

Thanks,
Andrew.

Re: Solarflare PMD submission question

[Thomas Monjalon]

2016-11-21 11:46, Andrew Rybchenko:
> On 11/21/2016 11:19 AM, Thomas Monjalon wrote:
> >> Before submitting 56 patches I'd like to double-check that checkpatch.pl
> >> errors (for example, because of assignments in the 'if' condition,
> >> parenthesis around return value) is not a show-stopper for base driver
> >> import.
> > You can run checkpatches.sh or send the patches to checkpatch@dpdk.org.
> > The script check-git-log.sh can also guide you for the expected formatting.
> 
> Yes, I did it and it helped me to find and fix some coding standard 
> violations.
> 
> The problem with libefx (base driver) is that it is existing code which 
> follows FreeBSD and illumos coding conventions which contradict to 
> checkpatches.sh sometimes (e.g. require parenthesis around return 
> value). Other example of error produced by checkpatches.sh is assign in 
> if. It is widely used in the code to assign return code value and 
> compare it vs 0 in one line. It is not a coding standard conflict, but 
> it is very wide-spread in the code (so changing it will produce too many 
> changes not strictly required/useful).
> 
> So, may I repeat my question if it is a show-stopper for base driver or 
> acceptable.

I would vote to accept these minor style warnings for the base driver.
Ferruh, any comment?

Re: Solarflare PMD submission question

[Ferruh Yigit]

On 11/21/2016 8:59 AM, Thomas Monjalon wrote:
> 2016-11-21 11:46, Andrew Rybchenko:
>> On 11/21/2016 11:19 AM, Thomas Monjalon wrote:
>>>> Before submitting 56 patches I'd like to double-check that checkpatch.pl
>>>> errors (for example, because of assignments in the 'if' condition,
>>>> parenthesis around return value) is not a show-stopper for base driver
>>>> import.
>>> You can run checkpatches.sh or send the patches to checkpatch@dpdk.org.
>>> The script check-git-log.sh can also guide you for the expected formatting.
>>
>> Yes, I did it and it helped me to find and fix some coding standard 
>> violations.
>>
>> The problem with libefx (base driver) is that it is existing code which 
>> follows FreeBSD and illumos coding conventions which contradict to 
>> checkpatches.sh sometimes (e.g. require parenthesis around return 
>> value). Other example of error produced by checkpatches.sh is assign in 
>> if. It is widely used in the code to assign return code value and 
>> compare it vs 0 in one line. It is not a coding standard conflict, but 
>> it is very wide-spread in the code (so changing it will produce too many 
>> changes not strictly required/useful).
>>
>> So, may I repeat my question if it is a show-stopper for base driver or 
>> acceptable.
> 
> I would vote to accept these minor style warnings for the base driver.
> Ferruh, any comment?
> 

For _base driver_, I am also OK for having checkpatch warnings.

Re: Solarflare PMD submission question

[Andrew Rybchenko]

On 11/21/2016 01:30 PM, Ferruh Yigit wrote:
> On 11/21/2016 8:59 AM, Thomas Monjalon wrote:
>> 2016-11-21 11:46, Andrew Rybchenko:
>>> On 11/21/2016 11:19 AM, Thomas Monjalon wrote:
>>>>> Before submitting 56 patches I'd like to double-check that checkpatch.pl
>>>>> errors (for example, because of assignments in the 'if' condition,
>>>>> parenthesis around return value) is not a show-stopper for base driver
>>>>> import.
>>>> You can run checkpatches.sh or send the patches to checkpatch@dpdk.org.
>>>> The script check-git-log.sh can also guide you for the expected formatting.
>>> Yes, I did it and it helped me to find and fix some coding standard
>>> violations.
>>>
>>> The problem with libefx (base driver) is that it is existing code which
>>> follows FreeBSD and illumos coding conventions which contradict to
>>> checkpatches.sh sometimes (e.g. require parenthesis around return
>>> value). Other example of error produced by checkpatches.sh is assign in
>>> if. It is widely used in the code to assign return code value and
>>> compare it vs 0 in one line. It is not a coding standard conflict, but
>>> it is very wide-spread in the code (so changing it will produce too many
>>> changes not strictly required/useful).
>>>
>>> So, may I repeat my question if it is a show-stopper for base driver or
>>> acceptable.
>> I would vote to accept these minor style warnings for the base driver.
>> Ferruh, any comment?
>>
> For _base driver_, I am also OK for having checkpatch warnings.

In term of checkpatshes.sh return value in parenthesis is an error (not 
warning). Hopefully it changes nothing.

Andrew.