Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Stephen Hemminger]

On Thu,  8 Apr 2021 15:58:40 -0700
Dexuan Cui <decui@microsoft.com> wrote:

> Add a VF driver for Microsoft Azure Network Adapter (MANA) that will be
> available in the future.
> 
> Co-developed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  MAINTAINERS                                   |    4 +-
>  drivers/net/ethernet/Kconfig                  |    1 +
>  drivers/net/ethernet/Makefile                 |    1 +
>  drivers/net/ethernet/microsoft/Kconfig        |   29 +
>  drivers/net/ethernet/microsoft/Makefile       |    5 +
>  drivers/net/ethernet/microsoft/mana/Makefile  |    6 +
>  drivers/net/ethernet/microsoft/mana/gdma.h    |  728 +++++++
>  .../net/ethernet/microsoft/mana/gdma_main.c   | 1515 ++++++++++++++
>  .../net/ethernet/microsoft/mana/hw_channel.c  |  859 ++++++++
>  .../net/ethernet/microsoft/mana/hw_channel.h  |  186 ++
>  drivers/net/ethernet/microsoft/mana/mana.h    |  531 +++++
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 1827 +++++++++++++++++
>  .../ethernet/microsoft/mana/mana_ethtool.c    |  278 +++
>  .../net/ethernet/microsoft/mana/shm_channel.c |  292 +++
>  .../net/ethernet/microsoft/mana/shm_channel.h |   21 +
>  15 files changed, 6282 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/microsoft/Kconfig
>  create mode 100644 drivers/net/ethernet/microsoft/Makefile
>  create mode 100644 drivers/net/ethernet/microsoft/mana/Makefile
>  create mode 100644 drivers/net/ethernet/microsoft/mana/gdma.h
>  create mode 100644 drivers/net/ethernet/microsoft/mana/gdma_main.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/hw_channel.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/hw_channel.h
>  create mode 100644 drivers/net/ethernet/microsoft/mana/mana.h
>  create mode 100644 drivers/net/ethernet/microsoft/mana/mana_en.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/mana_ethtool.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/shm_channel.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/shm_channel.h

Linux kernel doesn't do namespaces in the code, so every new driver needs
to worry about global symbols clashing

Please make sure there are not name conflicts with other drivers around variable or
functions name "gdma_". Noticed there is one driver in staging using similar
names (drivers/staging/ralink-gdma/ralink-gdma.c). Granted that is in the staging
doghouse so probably not important but might show up as a name conflict
with something like the randconfig testing.

Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[David Miller]

From: Dexuan Cui <decui@microsoft.com>
Date: Thu,  8 Apr 2021 15:58:40 -0700

> +struct gdma_msg_hdr {
> +	u32 hdr_type;
> +	u32 msg_type;
> +	u16 msg_version;
> +	u16 hwc_msg_id;
> +	u32 msg_size;
> +} __packed;
> +
> +struct gdma_dev_id {
> +	union {
> +		struct {
> +			u16 type;
> +			u16 instance;
> +		};
> +
> +		u32 as_uint32;
> +	};
> +} __packed;

Please don't  use __packed unless absolutely necessary.  It generates suboptimal code (byte at a time
accesses etc.) and for many of these you don't even need it.

Thank you.

Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Randy Dunlap]

On 4/8/21 3:58 PM, Dexuan Cui wrote:
> Changes in v3:
>     Changed the Kconfig file:
>         Microsoft Azure Network Device ==> Microsoft Network Devices
>         Drop the default m
>         validated ==> supported

Hi,
I am satisfied with the Kconfig changes.

thanks.
-- 
~Randy

RE: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui]

> From: David Miller <davem@davemloft.net>
> Sent: Thursday, April 8, 2021 4:46 PM
> ...
> > +struct gdma_msg_hdr {
> > +	u32 hdr_type;
> > +	u32 msg_type;
> > +	u16 msg_version;
> > +	u16 hwc_msg_id;
> > +	u32 msg_size;
> > +} __packed;
> > +
> > +struct gdma_dev_id {
> > +	union {
> > +		struct {
> > +			u16 type;
> > +			u16 instance;
> > +		};
> > +
> > +		u32 as_uint32;
> > +	};
> > +} __packed;
> 
> Please don't  use __packed unless absolutely necessary.  It generates
> suboptimal code (byte at a time
> accesses etc.) and for many of these you don't even need it.

In the driver code, all the structs/unions marked by __packed are used to
talk with the hardware, so I think __packed is necessary here?

Do you think if it's better if we remove all the __packed, and add
static_assert(sizeof(struct XXX) == YYY) instead? e.g.

@@ -105,7 +105,8 @@ struct gdma_msg_hdr {
        u16 msg_version;
        u16 hwc_msg_id;
        u32 msg_size;
-} __packed;
+};
+static_assert(sizeof(struct gdma_msg_hdr) == 16);

 struct gdma_dev_id {
        union {

Now I'm trying to figure out how other NIC drivers define structs/unions.

Thanks,
Dexuan

Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Andrew Lunn]

> Linux kernel doesn't do namespaces in the code, so every new driver needs
> to worry about global symbols clashing

This driver is called mana, yet the code uses ana. It would be good to
resolve this inconsistency as well. Ideally, you want to prefix
everything with ana_ or mana_, depending on what you choose, so we
have a clean namespace.

	   Andrew

Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[David Miller]

From: Dexuan Cui <decui@microsoft.com>
Date: Fri, 9 Apr 2021 00:24:51 +0000

>> From: David Miller <davem@davemloft.net>
>> Sent: Thursday, April 8, 2021 4:46 PM
>> ...
>> > +struct gdma_msg_hdr {
>> > +	u32 hdr_type;
>> > +	u32 msg_type;
>> > +	u16 msg_version;
>> > +	u16 hwc_msg_id;
>> > +	u32 msg_size;
>> > +} __packed;
>> > +
>> > +struct gdma_dev_id {
>> > +	union {
>> > +		struct {
>> > +			u16 type;
>> > +			u16 instance;
>> > +		};
>> > +
>> > +		u32 as_uint32;
>> > +	};
>> > +} __packed;
>> 
>> Please don't  use __packed unless absolutely necessary.  It generates
>> suboptimal code (byte at a time
>> accesses etc.) and for many of these you don't even need it.
> 
> In the driver code, all the structs/unions marked by __packed are used to
> talk with the hardware, so I think __packed is necessary here?

It actually isan't in many cases, check with and without the __packed directive
and see if anything chasnges.

> Do you think if it's better if we remove all the __packed, and add
> static_assert(sizeof(struct XXX) == YYY) instead? e.g.
> 
> @@ -105,7 +105,8 @@ struct gdma_msg_hdr {
>         u16 msg_version;
>         u16 hwc_msg_id;
>         u32 msg_size;
> -} __packed;
> +};
> +static_assert(sizeof(struct gdma_msg_hdr) == 16);

This won't make sure the structure member offsets are what you expect.

I think you'll have to go through the structures one-by-one by hand to
figure out which ones really require the __packed attribute and which do not.

Thank you.

RE: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui]

> From: David Miller <davem@davemloft.net>
> Sent: Thursday, April 8, 2021 5:41 PM
> > ...
> > In the driver code, all the structs/unions marked by __packed are used to
> > talk with the hardware, so I think __packed is necessary here?
> 
> It actually isan't in many cases, check with and without the __packed directive
> and see if anything chasnges.

Will do.

> > Do you think if it's better if we remove all the __packed, and add
> > static_assert(sizeof(struct XXX) == YYY) instead? e.g.
> >
> > @@ -105,7 +105,8 @@ struct gdma_msg_hdr {
> >         u16 msg_version;
> >         u16 hwc_msg_id;
> >         u32 msg_size;
> > -} __packed;
> > +};
> > +static_assert(sizeof(struct gdma_msg_hdr) == 16);
> 
> This won't make sure the structure member offsets are what you expect.
> 
> I think you'll have to go through the structures one-by-one by hand to
> figure out which ones really require the __packed attribute and which do not.

Got it. Let me see if I can remove all the __packed.

RE: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui]

> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, April 8, 2021 5:30 PM
> To: Stephen Hemminger <stephen@networkplumber.org>
> ...
> > Linux kernel doesn't do namespaces in the code, so every new driver needs
> > to worry about global symbols clashing
> 
> This driver is called mana, yet the code uses ana. It would be good to
> resolve this inconsistency as well. Ideally, you want to prefix
> everything with ana_ or mana_, depending on what you choose, so we
> have a clean namespace.
> 
> 	   Andrew

Thanks for the suggestion! Let me think about this and work out a solution.

RE: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Haiyang Zhang]

> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Thursday, April 8, 2021 8:41 PM
> To: Dexuan Cui <decui@microsoft.com>
> Cc: kuba@kernel.org; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; wei.liu@kernel.org; Wei Liu
> <liuwe@microsoft.com>; netdev@vger.kernel.org; leon@kernel.org;
> andrew@lunn.ch; bernd@petrovitsch.priv.at; rdunlap@infradead.org; linux-
> kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Subject: Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure
> Network Adapter (MANA)
> 
> From: Dexuan Cui <decui@microsoft.com>
> Date: Fri, 9 Apr 2021 00:24:51 +0000
> 
> >> From: David Miller <davem@davemloft.net>
> >> Sent: Thursday, April 8, 2021 4:46 PM
> >> ...
> >> > +struct gdma_msg_hdr {
> >> > +	u32 hdr_type;
> >> > +	u32 msg_type;
> >> > +	u16 msg_version;
> >> > +	u16 hwc_msg_id;
> >> > +	u32 msg_size;
> >> > +} __packed;
> >> > +
> >> > +struct gdma_dev_id {
> >> > +	union {
> >> > +		struct {
> >> > +			u16 type;
> >> > +			u16 instance;
> >> > +		};
> >> > +
> >> > +		u32 as_uint32;
> >> > +	};
> >> > +} __packed;
> >>
> >> Please don't  use __packed unless absolutely necessary.  It generates
> >> suboptimal code (byte at a time
> >> accesses etc.) and for many of these you don't even need it.
> >
> > In the driver code, all the structs/unions marked by __packed are used to
> > talk with the hardware, so I think __packed is necessary here?
> 
> It actually isan't in many cases, check with and without the __packed
> directive
> and see if anything chasnges.
> 
> > Do you think if it's better if we remove all the __packed, and add
> > static_assert(sizeof(struct XXX) == YYY) instead? e.g.
> >
> > @@ -105,7 +105,8 @@ struct gdma_msg_hdr {
> >         u16 msg_version;
> >         u16 hwc_msg_id;
> >         u32 msg_size;
> > -} __packed;
> > +};
> > +static_assert(sizeof(struct gdma_msg_hdr) == 16);
> 
> This won't make sure the structure member offsets are what you expect.
> 
> I think you'll have to go through the structures one-by-one by hand to
> figure out which ones really require the __packed attribute and which do not.

For the structs containing variables with the same sizes, or already size aligned 
variables, we knew the __packed has no effect. And for these structs, it doesn't 
cause performance impact either, correct? 

But in the future, if different sized variables are added, the __packed may 
become necessary again. To prevent anyone accidently forget to add __packed 
when adding new variables to these structs, can we keep the __packed for all 
messages going through the "wire"?

Thanks,
- Haiyang

Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Andrew Lunn]

 For the structs containing variables with the same sizes, or already size aligned 
> variables, we knew the __packed has no effect. And for these structs, it doesn't 
> cause performance impact either, correct? 
> 
> But in the future, if different sized variables are added, the __packed may 
> become necessary again. To prevent anyone accidently forget to add __packed 
> when adding new variables to these structs, can we keep the __packed for all 
> messages going through the "wire"?

It should not be a problem because anybody adding new variables should
know packed is not liked in the kernel and will take care.

If you want to be paranoid add a BUILD_BUG_ON(size(struct foo) != 42);

   Andrew