netdev - RE: [PATCH net-next 1/3] net/dcb: Add IEEE QCN attribute

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] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 5 Mar 2015 09:21:14 +0000
From:	Shachar Raindel <raindel@...lanox.com>
To:	Or Gerlitz <gerlitz.or@...il.com>,
	John Fastabend <john.fastabend@...il.com>
CC:	Or Gerlitz <ogerlitz@...lanox.com>,
	"David S. Miller" <davem@...emloft.net>,
	John Fastabend <john.r.fastabend@...el.com>,
	"Linux Netdev List" <netdev@...r.kernel.org>,
	Amir Vadai <amirv@...lanox.com>,
	"Tal Alon" <talal@...lanox.com>,
	Shani Michaeli <shanim@...lanox.com>
Subject: RE: [PATCH net-next 1/3] net/dcb: Add IEEE QCN attribute



> -----Original Message-----
> From: Or Gerlitz [mailto:gerlitz.or@...il.com]
> Sent: Thursday, March 05, 2015 8:54 AM
> To: John Fastabend
> Cc: Or Gerlitz; David S. Miller; John Fastabend; Linux Netdev List; Amir
> Vadai; Tal Alon; Shani Michaeli; Shachar Raindel
> Subject: Re: [PATCH net-next 1/3] net/dcb: Add IEEE QCN attribute
> 
> On Wed, Mar 4, 2015 at 7:19 PM, John Fastabend
> <john.fastabend@...il.com> wrote:
> > On 03/04/2015 04:51 AM, Or Gerlitz wrote:
> >>
> >> From: Shani Michaeli <shanim@...lanox.com>
> >>
> >> As specified in 802.1Qau spec. Add this optional attribute to the
> >> DCB netlink layer. To allow for application to use the new attribute,
> >> NIC drivers should implement and register the  callbacks ieee_getqcn,
> >> ieee_setqcn and ieee_getqcnstats.
> >>
> >> The QCN attribute holds a set of parameters for management, and
> >> a set of statistics to provide informative data on Congestion-Control
> >> defined by this spec.
> >>
> >> Signed-off-by: Shani Michaeli <shanim@...lanox.com>
> >> Signed-off-by: Shachar Raindel <raindel@...lanox.com>
> >> Signed-off-by: Or Gerlitz <ogerlitz@...lanox.com>
> 
> > Looks good to me. Do you have a QCN enabled switch? I looked at
> > implementing this awhile ago but didn't have any switch support so
> > I never did it.
> 
> I'l let Shachar to address the testing and the MIB questions.

The Mellanox SwitchX-2 IC supports QCN. We were testing our NICs both
with this switch IC and using internal test fixtures where we were
injecting congestion notification messages as raw Ethernet packets by
another host.

> 
> > Also do you have a user space client to configure this? I would like
> > it if someone wanted to add support to lldpad/dcbtool.
> 
> Sure, we have some netlink (python scripts) code to configure/read
> this towards the kernel.
> 
> 
> > [...]
> >> +
> >> +/* This structure contains the IEEE 802.1Qau QCN managed object.
> >> + *
> >> + *@..._enable: enable QCN RP
> >> + *@...p_max_rps: maximum number of RPs allowed for this CNPV on this
> port
> >> + *@..._time_reset: time between rate increases if no CNMs received.
> >> + *                given in u-seconds
> >> + *@..._byte_reset: transmitted data between rate increases if no
> CNMs
> >> received.
> >> + *                given in Bytes
> >> + *@..._threshold: The number of times rpByteStage or rpTimeStage can
> >> count
> >> + *                before RP rate control state machine advances
> states
> >> + *@..._max_rate: the maxinun rate, in Mbits per second,
> >> + *              at which an RP can transmit
> >> + *@..._ai_rate: The rate, in Mbits per second,
> >> + *             used to increase rpTargetRate in the
> RPR_ACTIVE_INCREASE
> >> + *@..._hai_rate: The rate, in Mbits per second,
> >> + *              used to increase rpTargetRate in the
> RPR_HYPER_INCREASE
> >> state
> >> + *@..._gd: Upon CNM receive, flow rate is limited to
> (Fb/Gd)*CurrentRate.
> >> + *        rpgGd is given as log2(Gd), where Gd may only be powers of
> 2
> >> + *@..._min_dec_fac: The minimum factor by which the current transmit
> rate
> >> + *                 can be changed by reception of a CNM.
> >> + *                 value is given as percentage (1-100)
> >> + *@..._min_rate: The minimum value, in bits per second, for rate to
> limit
> >> + *@...d_state_machine: The state of the congestion notification
> domain
> >> + *                    defense state machine, as defined by IEEE
> 802.3Qau
> >> + *                    section 32.1.1. In the interior ready state,
> >> + *                    the QCN capable hardware may add CN-TAG TLV to
> the
> >> + *                    outgoing traffic, to specifically identify
> outgoing
> >> + *                    flows.
> >> + */
> >
> >
> > I'm assuming this structure maps to an IEEE MIB?
> 
> yep, I guess so

The structures map to the IEEE MIB for QCN (part of IEEE 802.1Qau).
The fields rppp_max_rps and cndd_state_machine are in different sections
than the rest of the fields. However, it seems bit redundant to define a
whole struct just for one field. The cndd_state_machine is not explicitly
defined in the MIB, as the LLDP negotiation, which the standard assumes,
is implemented by lldpad.

> 
> > Its a rather large structure for a single netlink type but this seems
> > to be how we built
> > the dcbnl interface and if it does seem logical that the structure is
> > one logical block, meaning you need to supply all fields.
> 

This is the set of parameters defining how you will reduce or increase you
TX rate upon receiving CNM from the network. I agree that it is a long list,
but this is how the standard was written...

> > [...]
> >> +       if (ops->ieee_getqcn) {
> >> +               struct ieee_qcn qcn;
> 
> > you might consider adding a newline here it  is the best practice for
> > new code although dcbnl has plenty of examples where it doesn't use
> > this convention.
> 
> OK, will add this here and in the other places where used that practice.