New driver (large patch) question.

New driver (large patch) question.

[Stephen Hurd]

I submitted a new driver on Friday, and it was rejected for being over 300k.

The rejection email suggested contacing dev-owner@dpdk.org, which I did on
Monday with no reply.

What's the process to submit patches larger than the mailing list size
limit?

-- 
Stephen Hurd
Principal Engineer - Software Development
Broadcom Corporation
949-926-8039
stephen.hurd@broadcom.com

Re: New driver (large patch) question.

[Thomas Monjalon]

Hi,

2016-03-01 19:56, Stephen Hurd:
> I submitted a new driver on Friday, and it was rejected for being over 300k.
> 
> The rejection email suggested contacing dev-owner@dpdk.org, which I did on
> Monday with no reply.
> 
> What's the process to submit patches larger than the mailing list size
> limit?

A patch has two lives:
1/ it must be reviewed and accepted
2/ it will stay in the git history for future reference

Those 2 periods require the patch to be well explained, with a
reasonnable scope and a human readable size.
The primary rule to think about is to introduce only one feature
per patch.
So the size should be naturally small and the mailing list don't need
to accept greater sizes.

To make it short, please split your driver in several introduction steps.

Re: New driver (large patch) question.

[Stephen Hemminger]

On Wed, 02 Mar 2016 11:21:26 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> Hi,
> 
> 2016-03-01 19:56, Stephen Hurd:
> > I submitted a new driver on Friday, and it was rejected for being over 300k.
> > 
> > The rejection email suggested contacing dev-owner@dpdk.org, which I did on
> > Monday with no reply.
> > 
> > What's the process to submit patches larger than the mailing list size
> > limit?
> 
> A patch has two lives:
> 1/ it must be reviewed and accepted
> 2/ it will stay in the git history for future reference
> 
> Those 2 periods require the patch to be well explained, with a
> reasonnable scope and a human readable size.
> The primary rule to think about is to introduce only one feature
> per patch.
> So the size should be naturally small and the mailing list don't need
> to accept greater sizes.
> 
> To make it short, please split your driver in several introduction steps.
> 

Too many of the DPDK drivers are bloated.
Recall the venerable paraphrase of Pascal, "I made this so long because I did not have time to make it shorter."
https://en.wikipedia.org/wiki/Wikipedia:Too_long;_didn%27t_read

Linux went through similar stages. Many drivers ended up being rewritten
for brevity (e1000, skge, tg3). Vendor drivers seem to want to engage all features
even if they have no value.

Re: New driver (large patch) question.

[Stephen Hurd]

The bulk of the patch is the hardware interface header file.  With all the
comments, it weighs in around 800k.  If I strip the comments, it's around
300k.  If I both strip all the comments and remove all the currently unused
structures, I can get the entire patch down just below 300k, but that makes
it much harder for someone to do further development.  I'm willing to do
that though if it's what's preferred.

The other large file (560k) is just a bunch of extra debug output that
makes it easier to debug issues.  It's normally not compiled, so it sounds
like it's not wanted either.

I'll submit without comments in the hardware interface file and take it
from there.

On Wed, Mar 2, 2016 at 8:24 AM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Wed, 02 Mar 2016 11:21:26 +0100
> Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
>
> > Hi,
> >
> > 2016-03-01 19:56, Stephen Hurd:
> > > I submitted a new driver on Friday, and it was rejected for being over
> 300k.
> > >
> > > The rejection email suggested contacing dev-owner@dpdk.org, which I
> did on
> > > Monday with no reply.
> > >
> > > What's the process to submit patches larger than the mailing list size
> > > limit?
> >
> > A patch has two lives:
> > 1/ it must be reviewed and accepted
> > 2/ it will stay in the git history for future reference
> >
> > Those 2 periods require the patch to be well explained, with a
> > reasonnable scope and a human readable size.
> > The primary rule to think about is to introduce only one feature
> > per patch.
> > So the size should be naturally small and the mailing list don't need
> > to accept greater sizes.
> >
> > To make it short, please split your driver in several introduction steps.
> >
>
> Too many of the DPDK drivers are bloated.
> Recall the venerable paraphrase of Pascal, "I made this so long because I
> did not have time to make it shorter."
> https://en.wikipedia.org/wiki/Wikipedia:Too_long;_didn%27t_read
>
> Linux went through similar stages. Many drivers ended up being rewritten
> for brevity (e1000, skge, tg3). Vendor drivers seem to want to engage all
> features
> even if they have no value.
>



-- 
Stephen Hurd
Principal Engineer - Software Development
Broadcom Corporation
949-926-8039
stephen.hurd@broadcom.com

Re: New driver (large patch) question.

[Thomas Monjalon]

2016-03-02 13:30, Stephen Hurd:
> The bulk of the patch is the hardware interface header file.  With all the
> comments, it weighs in around 800k.  If I strip the comments, it's around
> 300k.  If I both strip all the comments and remove all the currently unused
> structures, I can get the entire patch down just below 300k, but that makes
> it much harder for someone to do further development.  I'm willing to do
> that though if it's what's preferred.
> 
> The other large file (560k) is just a bunch of extra debug output that
> makes it easier to debug issues.  It's normally not compiled, so it sounds
> like it's not wanted either.

If the code is not needed, it's obviously better to not submit it :)

> I'll submit without comments in the hardware interface file and take it
> from there.

I don't think removing the comments is a good option.
Please try to split per-feature to make it readable.
You can check how fm10k was introduced as an example:
	http://thread.gmane.org/gmane.comp.networking.dpdk.devel/13447
or mlx5:
	http://thread.gmane.org/gmane.comp.networking.dpdk.devel/26986

Re: New driver (large patch) question.

[Stephen Hurd]

The issue is that the big header in question is the whole hardware/firmware
interface definition.  The comments in it are the only publicly available
documentation on the hardware I'm aware of.

The driver itself doesn't have a lot of optional features in it, it's the
header file that's too big.

On Wed, Mar 2, 2016 at 1:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> 2016-03-02 13:30, Stephen Hurd:
> > The bulk of the patch is the hardware interface header file.  With all
> the
> > comments, it weighs in around 800k.  If I strip the comments, it's around
> > 300k.  If I both strip all the comments and remove all the currently
> unused
> > structures, I can get the entire patch down just below 300k, but that
> makes
> > it much harder for someone to do further development.  I'm willing to do
> > that though if it's what's preferred.
> >
> > The other large file (560k) is just a bunch of extra debug output that
> > makes it easier to debug issues.  It's normally not compiled, so it
> sounds
> > like it's not wanted either.
>
> If the code is not needed, it's obviously better to not submit it :)
>
> > I'll submit without comments in the hardware interface file and take it
> > from there.
>
> I don't think removing the comments is a good option.
> Please try to split per-feature to make it readable.
> You can check how fm10k was introduced as an example:
>         http://thread.gmane.org/gmane.comp.networking.dpdk.devel/13447
> or mlx5:
>         http://thread.gmane.org/gmane.comp.networking.dpdk.devel/26986
>



-- 
Stephen Hurd
Principal Engineer - Software Development
Broadcom Corporation
949-926-8039
stephen.hurd@broadcom.com

Re: New driver (large patch) question.

[Wiles, Keith]

>The issue is that the big header in question is the whole hardware/firmware
>interface definition.  The comments in it are the only publicly available
>documentation on the hardware I'm aware of.
>
>The driver itself doesn't have a lot of optional features in it, it's the
>header file that's too big.

The new files and driver code do these files really need to be in a patch format? Now the changes to current DPDK files must be a patch. If we can figure out how to tar up and then just add those files with a good comment would that be OK?

Regards
Keith
>
>On Wed, Mar 2, 2016 at 1:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
>wrote:
>
>> 2016-03-02 13:30, Stephen Hurd:
>> > The bulk of the patch is the hardware interface header file.  With all
>> the
>> > comments, it weighs in around 800k.  If I strip the comments, it's around
>> > 300k.  If I both strip all the comments and remove all the currently
>> unused
>> > structures, I can get the entire patch down just below 300k, but that
>> makes
>> > it much harder for someone to do further development.  I'm willing to do
>> > that though if it's what's preferred.
>> >
>> > The other large file (560k) is just a bunch of extra debug output that
>> > makes it easier to debug issues.  It's normally not compiled, so it
>> sounds
>> > like it's not wanted either.
>>
>> If the code is not needed, it's obviously better to not submit it :)
>>
>> > I'll submit without comments in the hardware interface file and take it
>> > from there.
>>
>> I don't think removing the comments is a good option.
>> Please try to split per-feature to make it readable.
>> You can check how fm10k was introduced as an example:
>>         http://thread.gmane.org/gmane.comp.networking.dpdk.devel/13447
>> or mlx5:
>>         http://thread.gmane.org/gmane.comp.networking.dpdk.devel/26986
>>
>
>
>
>-- 
>Stephen Hurd
>Principal Engineer - Software Development
>Broadcom Corporation
>949-926-8039
>stephen.hurd@broadcom.com
>


Regards,
Keith

Re: New driver (large patch) question.

[Thomas Monjalon]

2016-03-02 14:06, Stephen Hurd:
> The issue is that the big header in question is the whole hardware/firmware
> interface definition.  The comments in it are the only publicly available
> documentation on the hardware I'm aware of.

So you must keep the comments.

> The driver itself doesn't have a lot of optional features in it, it's the
> header file that's too big.

It is big because there are many different things.
You can split the file in different patches.
Examples:
	- a patch for RSS will bring the hardware structures for RSS
	- a patch for the stats will bring the hardware stats structures
	etc

PS: please answer inline

 
> On Wed, Mar 2, 2016 at 1:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
> > 2016-03-02 13:30, Stephen Hurd:
> > > The bulk of the patch is the hardware interface header file.  With all
> > the
> > > comments, it weighs in around 800k.  If I strip the comments, it's around
> > > 300k.  If I both strip all the comments and remove all the currently
> > unused
> > > structures, I can get the entire patch down just below 300k, but that
> > makes
> > > it much harder for someone to do further development.  I'm willing to do
> > > that though if it's what's preferred.
> > >
> > > The other large file (560k) is just a bunch of extra debug output that
> > > makes it easier to debug issues.  It's normally not compiled, so it
> > sounds
> > > like it's not wanted either.
> >
> > If the code is not needed, it's obviously better to not submit it :)
> >
> > > I'll submit without comments in the hardware interface file and take it
> > > from there.
> >
> > I don't think removing the comments is a good option.
> > Please try to split per-feature to make it readable.
> > You can check how fm10k was introduced as an example:
> >         http://thread.gmane.org/gmane.comp.networking.dpdk.devel/13447
> > or mlx5:
> >         http://thread.gmane.org/gmane.comp.networking.dpdk.devel/26986
> >
> 
> 
> 
> 

Re: New driver (large patch) question.

[Stephen Hurd]

On Wed, Mar 2, 2016 at 2:15 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> > The comments in it are the only publicly available
> > documentation on the hardware I'm aware of.
>
> So you must keep the comments.
>

That's my goal, but the comments are well over the 300k limit.


> > The driver itself doesn't have a lot of optional features in it, it's the
> > header file that's too big.
>
> It is big because there are many different things.
> You can split the file in different patches.
> Examples:
>         - a patch for RSS will bring the hardware structures for RSS
>         - a patch for the stats will bring the hardware stats structures
>         etc
>

Should I split additional definitions/documentation that's not currently
used in the driver as well?  Or should it stay as only enough to document
what the driver already does?

The header file is expected to be publicly released in the future, so I
tried to keep it as close to the original as possible.  I'm not strongly
attached to this approach, but it does make it easier to support future
firmware releases.

It's a fairly work-intensive project to deconstruct the existing driver
into a series of small patches that work at each step, is this a hard
requirement? (if so, I'd better get cracking)

PS: please answer inline
>

Sorry, $work just switched us to GMail and I'm still learning the ropes.

Re: New driver (large patch) question.

[Thomas Monjalon]

2016-03-02 15:10, Stephen Hurd:
> On Wed, Mar 2, 2016 at 2:15 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> > > The driver itself doesn't have a lot of optional features in it, it's the
> > > header file that's too big.
> >
> > It is big because there are many different things.
> > You can split the file in different patches.
> > Examples:
> >         - a patch for RSS will bring the hardware structures for RSS
> >         - a patch for the stats will bring the hardware stats structures
> >         etc
> 
> Should I split additional definitions/documentation that's not currently
> used in the driver as well?  Or should it stay as only enough to document
> what the driver already does?

I don't understand the question.
If something is not used, it should not been there.

> It's a fairly work-intensive project to deconstruct the existing driver
> into a series of small patches that work at each step, is this a hard
> requirement? (if so, I'd better get cracking)

There is no hard requirement. I'm just giving you some advices to get
some reviewers and make them confident when accepting your patches.
By the way, you would get more attention by introducing the device with
some web links and performance numbers in the cover letter.
It is also appreciated to provide a documentation in doc/guides/nics/.
You could also fill the (new) table in overview.rst.

Thanks

Re: New driver (large patch) question.

[Stephen Hurd]

On Wed, Mar 2, 2016 at 4:29 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> > Should I split additional definitions/documentation that's not currently
> > used in the driver as well?  Or should it stay as only enough to document
> > what the driver already does?
>
> I don't understand the question.
> If something is not used, it should not been there.
>

Basically, since the header is the only current public documentation on the
hardware, if someone without access to the whole header was interested in
adding features to the driver, they wouldn't have the documentation to
allow them to do so.

I understand that a header file in DPDK is not the right way for Broadcom
to release documentation on their hardware and firmware, but I was thinking
it would be better than nothing.  Further, as mentioned in another email,
this header file is mechanically derived from one that is planned to be
released separately, so it would be a slightly lower support burden to use
the mechanically derived one rather than manually importing changes.

No problems slimming it down though.


> > It's a fairly work-intensive project to deconstruct the existing driver
> > into a series of small patches that work at each step, is this a hard
> > requirement? (if so, I'd better get cracking)
>
> There is no hard requirement. I'm just giving you some advices to get
> some reviewers and make them confident when accepting your patches.
> By the way, you would get more attention by introducing the device with
> some web links and performance numbers in the cover letter.
> It is also appreciated to provide a documentation in doc/guides/nics/.
> You could also fill the (new) table in overview.rst.
>

Thanks for the pointers, I'll try to get as many of the suggestions done as
possible and talk to marketing to find web resources for the device and its
performance.


-- 
Stephen Hurd
Principal Engineer - Software Development
Broadcom Corporation
949-926-8039
stephen.hurd@broadcom.com

Re: New driver (large patch) question.

[Qiu, Michael]

On 3/3/2016 7:11 AM, Stephen Hurd wrote:
> On Wed, Mar 2, 2016 at 2:15 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
>
>>> The comments in it are the only publicly available
>>> documentation on the hardware I'm aware of.
>> So you must keep the comments.
>>
> That's my goal, but the comments are well over the 300k limit.
>
>
>>> The driver itself doesn't have a lot of optional features in it, it's the
>>> header file that's too big.
>> It is big because there are many different things.
>> You can split the file in different patches.
>> Examples:
>>         - a patch for RSS will bring the hardware structures for RSS
>>         - a patch for the stats will bring the hardware stats structures
>>         etc
>>
> Should I split additional definitions/documentation that's not currently
> used in the driver as well?  Or should it stay as only enough to document
> what the driver already does?
>
> The header file is expected to be publicly released in the future, so I
> tried to keep it as close to the original as possible.  I'm not strongly
> attached to this approach, but it does make it easier to support future
> firmware releases.
>
> It's a fairly work-intensive project to deconstruct the existing driver
> into a series of small patches that work at each step, is this a hard
> requirement? (if so, I'd better get cracking)

Does original header file has it's own commit log(like it in other
project)? If yes, it could make your life simpler.

Thanks,
Michael 
> PS: please answer inline
> Sorry, $work just switched us to GMail and I'm still learning the ropes.
>

Re: New driver (large patch) question.

[Stephen Hurd]

Unfortunately no.

On Wed, Mar 2, 2016 at 9:53 PM, Qiu, Michael <michael.qiu@intel.com> wrote:

> On 3/3/2016 7:11 AM, Stephen Hurd wrote:
> > On Wed, Mar 2, 2016 at 2:15 PM, Thomas Monjalon <
> thomas.monjalon@6wind.com>
> > wrote:
> >
> >>> The comments in it are the only publicly available
> >>> documentation on the hardware I'm aware of.
> >> So you must keep the comments.
> >>
> > That's my goal, but the comments are well over the 300k limit.
> >
> >
> >>> The driver itself doesn't have a lot of optional features in it, it's
> the
> >>> header file that's too big.
> >> It is big because there are many different things.
> >> You can split the file in different patches.
> >> Examples:
> >>         - a patch for RSS will bring the hardware structures for RSS
> >>         - a patch for the stats will bring the hardware stats structures
> >>         etc
> >>
> > Should I split additional definitions/documentation that's not currently
> > used in the driver as well?  Or should it stay as only enough to document
> > what the driver already does?
> >
> > The header file is expected to be publicly released in the future, so I
> > tried to keep it as close to the original as possible.  I'm not strongly
> > attached to this approach, but it does make it easier to support future
> > firmware releases.
> >
> > It's a fairly work-intensive project to deconstruct the existing driver
> > into a series of small patches that work at each step, is this a hard
> > requirement? (if so, I'd better get cracking)
>
> Does original header file has it's own commit log(like it in other
> project)? If yes, it could make your life simpler.
>
> Thanks,
> Michael
> > PS: please answer inline
> > Sorry, $work just switched us to GMail and I'm still learning the ropes.
> >
>
>


-- 
Stephen Hurd
Principal Engineer - Software Development
Broadcom Corporation
949-926-8039
stephen.hurd@broadcom.com

Re: New driver (large patch) question.

[Vincent JARDIN]

Please,

Le 02/03/2016 22:30, Stephen Hurd a écrit :
> Too many of the DPDK drivers are bloated.
>>Recall the venerable paraphrase of Pascal, "I made this so long because I
>>did not have time to make it shorter."
>>https://en.wikipedia.org/wiki/Wikipedia:Too_long;_didn%27t_read

Keep In Simple, Small Is Beautiful, the big drivers with dead codes are 
not easy to be maintained. We have lot of duplication of efforts between 
the kernel and some DPDK PMDs,

Currently, the breakdown of Lines of Codes of the PMDs are:

492 ring
522 null
666 af_packet
829 pcap
1229 szedata2
1300 mpipe
1411 xenvirt
2036 nfp
2260 vmxnet3
3074 virtio
4129 mlx4
4205 bonding
4524 mlx5
4904 enic
7654 cxgbe
7969 fm10k
27862 ixgbe
29209 e1000
31392 i40e
38031 bnx2x

(I did use cloc).

Vincent

Re: New driver (large patch) question.

[Bruce Richardson]

On Thu, Mar 03, 2016 at 12:07:21AM +0100, Vincent JARDIN wrote:
> Please,
> 
> Le 02/03/2016 22:30, Stephen Hurd a écrit :
> >Too many of the DPDK drivers are bloated.
> >>Recall the venerable paraphrase of Pascal, "I made this so long because I
> >>did not have time to make it shorter."
> >>https://en.wikipedia.org/wiki/Wikipedia:Too_long;_didn%27t_read
> 
> Keep In Simple, Small Is Beautiful, the big drivers with dead codes are not
> easy to be maintained. We have lot of duplication of efforts between the
> kernel and some DPDK PMDs,
> 
> Currently, the breakdown of Lines of Codes of the PMDs are:
> 
> 492 ring
> 522 null
> 666 af_packet
> 829 pcap
> 1229 szedata2
> 1300 mpipe
> 1411 xenvirt
> 2036 nfp
> 2260 vmxnet3
> 3074 virtio
> 4129 mlx4
> 4205 bonding
> 4524 mlx5
> 4904 enic
> 7654 cxgbe
> 7969 fm10k
> 27862 ixgbe
> 29209 e1000
> 31392 i40e
> 38031 bnx2x
> 
> (I did use cloc).
> 
> Vincent

Hi Vincent,

I think most of us would wholehearted agree with the "small is beautiful"
sentiment.

However, I disagree with the assumption that these counts reflect the maintenance
burden of these drivers. The Intel drivers, as do some others, use a "base code"
model, where there is a set of code shared between various platforms. This code
should not be included in the maintenance calculation since it is explicitly not
being maintained by the DPDK project itself. A better metric of the overhead of
the various drivers would be the LOC count excluding any base code.

Now, if you are considering the code size for compilation time, or release package
sizing - then it's a fair metric! :-)

Regards,
/Bruce