netdev - Re: [PATCH v4 net-next 14/19] ionic: Add Tx and Rx handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 9 Aug 2019 15:27:56 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Saeed Mahameed <saeedm@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v4 net-next 14/19] ionic: Add Tx and Rx handling

On 7/25/19 4:48 PM, Saeed Mahameed wrote:
> On Mon, 2019-07-22 at 14:40 -0700, Shannon Nelson wrote:
>> Add both the Tx and Rx queue setup and handling.  The related
>> stats display comes later.  Instead of using the generic napi
>> routines used by the slow-path commands, the Tx and Rx paths
>> are simplified and inlined in one file in order to get better
>> compiler optimizations.
>>
>> Signed-off-by: Shannon Nelson<snelson@...sando.io>
>> ---
>>   drivers/net/ethernet/pensando/ionic/Makefile  |   2 +-
>>   .../net/ethernet/pensando/ionic/ionic_lif.c   | 387 ++++++++
>>   .../net/ethernet/pensando/ionic/ionic_lif.h   |  52 ++
>>   .../net/ethernet/pensando/ionic/ionic_txrx.c  | 879
>> ++++++++++++++++++
>>   .../net/ethernet/pensando/ionic/ionic_txrx.h  |  15 +
>>   5 files changed, 1334 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>>   create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_txrx.h
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/Makefile
>> b/drivers/net/ethernet/pensando/ionic/Makefile
>> index 9b19bf57a489..0e2dc53f08d4 100644
>> --- a/drivers/net/ethernet/pensando/ionic/Makefile
>> +++ b/drivers/net/ethernet/pensando/ionic/Makefile
>> @@ -4,4 +4,4 @@
>>   obj-$(CONFIG_IONIC) := ionic.o
>>   
>>   ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o ionic_ethtool.o
>> \
>> -	   ionic_lif.o ionic_rx_filter.o ionic_debugfs.o
>> +	   ionic_lif.o ionic_rx_filter.o ionic_txrx.o ionic_debugfs.o
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> index 2bd8ce61c4a0..40d3b1cb362a 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> @@ -10,6 +10,7 @@
>>   #include "ionic.h"
>>   #include "ionic_bus.h"
>>   #include "ionic_lif.h"
>> +#include "ionic_txrx.h"
>>   #include "ionic_ethtool.h"
>>   #include "ionic_debugfs.h"
>>   
>> @@ -18,6 +19,13 @@ static int ionic_lif_addr_add(struct lif *lif,
>> const u8 *addr);
>>   static int ionic_lif_addr_del(struct lif *lif, const u8 *addr);
>>   static void ionic_link_status_check(struct lif *lif);
>>   
>> +static int ionic_lif_stop(struct lif *lif);
>> +static int ionic_txrx_alloc(struct lif *lif);
>> +static int ionic_txrx_init(struct lif *lif);
>> +static void ionic_qcq_free(struct lif *lif, struct qcq *qcq);
>> +static int ionic_lif_txqs_init(struct lif *lif);
>> +static int ionic_lif_rxqs_init(struct lif *lif);
>> +static void ionic_lif_qcq_deinit(struct lif *lif, struct qcq *qcq);
>>   static int ionic_set_nic_features(struct lif *lif, netdev_features_t
>> features);
>>   static int ionic_notifyq_clean(struct lif *lif, int budget);
>>   
>> @@ -66,12 +74,96 @@ static void ionic_lif_deferred_enqueue(struct
>> ionic_deferred *def,
>>   	schedule_work(&def->work);
>>   }
> Bottom up or top down ? your current design is very mixed and I had to
> to scroll down and up too often just to understand what a function is
> doing, i strongly suggest to pick an use one approach.
>
> [1]
> https://en.wikipedia.org/wiki/Top-down_and_bottom-up_design#Programming

Hi Saeed,

Sorry it has taken me so long to address the comments you had on this 
patch: I've been contemplating them while working some internal issues.  
Part of my hesitance is also because, while correct, these are 
suggesting some significant rework and juggling of stable code. I've 
been aware of these issues since I inherited the code, and have been 
addressing them over time while trying to keep on top of stability, and 
was planning to continue doing so.  It sounds like I need to evolve the 
driver a little further before the next patchset version.  I think I'll 
have time in the next week to do this, so I'm hoping to have a new 
patchset out in another week or so.

Thanks for the inputs,
sln