Linux Ethernet Bridging - Dec 2019 - [Bridge] [PATCH net-next] net: bridge: add STP xstats

Hi Nikolay,

On Tue, 10 Dec 2019 09:49:59 +0200, Nikolay Aleksandrov <nikolay at
cumulusnetworks.com> wrote:
> Why did you send the bridge patch again ? Does it have any changes ?
The second iproute2 patch does not include the include guards update, but
I kept the bridge_stp_stats structure and the BRIDGE_XSTATS_STP definition
otherwise iproute2 wouldn't compile.
> 
> Why do you need percpu ? All of these seem to be incremented with the
> bridge lock held. A few more comments below.
All other xstats are incremented percpu, I simply followed the pattern.
> >  	struct net_bridge_port *p
> >  		= container_of(kobj, struct net_bridge_port, kobj);
> > +	free_percpu(p->stp_stats);
> 
> Please leave a new line between local var declaration and the code. I know
> it was missing, but you can add it now. :)
OK.
> > +	if (p) {
> > +		struct bridge_stp_xstats xstats;
> 
> Please rename the local var here, using just xstats is misleading.
> Maybe stp_xstats ?
This isn't misleading to me since its scope is limited to the current block
and not the entire function. The block above dumping the VLAN xstats is
using a local "struct br_vlan_stats stats" variable for example.
> 
> > +
> > +		br_stp_get_xstats(p, &xstats);
> > +
> > +		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
> > +			goto nla_put_failure;
> 
> Could you please follow how mcast xstats are dumped and do something
similar ?
> It'd be nice to have similar code to audit.
Sure. I would also love to have easily auditable code in net/bridge. For
the bridge STP xstats I followed the VLAN xstats code above, which does:

    if (nla_put(skb, BRIDGE_XSTATS_VLAN, sizeof(vxi), &vxi))
        goto nla_put_failure;

But I can change the STP xstats code to the following:

    if (p) {
        nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
                                sizeof(struct bridge_stp_xstats),
                                BRIDGE_XSTATS_PAD);
        if (!nla)
            goto nla_put_failure;

        br_stp_get_xstats(p, nla_data(nla));
    }

Would that be preferred?


Thanks,

	Vivien

On 10/12/2019 21:39, Vivien Didelot wrote:
> Hi Nikolay,
> 
> On Tue, 10 Dec 2019 09:49:59 +0200, Nikolay Aleksandrov <nikolay at
cumulusnetworks.com> wrote:
> 
>> Why did you send the bridge patch again ? Does it have any changes ?
> 
> The second iproute2 patch does not include the include guards update, but
> I kept the bridge_stp_stats structure and the BRIDGE_XSTATS_STP definition
> otherwise iproute2 wouldn't compile.
> >>
>> Why do you need percpu ? All of these seem to be incremented with the
>> bridge lock held. A few more comments below.
> 
> All other xstats are incremented percpu, I simply followed the pattern.
> 
We have already a lock, we can use it and avoid the whole per-cpu memory
handling.
It seems to be acquired in all cases where these counters need to be changed.
>>>  	struct net_bridge_port *p
>>>  		= container_of(kobj, struct net_bridge_port, kobj);
>>> +	free_percpu(p->stp_stats);
>>
>> Please leave a new line between local var declaration and the code. I
know
>> it was missing, but you can add it now. :)
> 
> OK.
> 
>>> +	if (p) {
>>> +		struct bridge_stp_xstats xstats;
>>
>> Please rename the local var here, using just xstats is misleading.
>> Maybe stp_xstats ?
> 
> This isn't misleading to me since its scope is limited to the current
block
> and not the entire function. The block above dumping the VLAN xstats is
> using a local "struct br_vlan_stats stats" variable for example.
> 
Yep, as I answered to myself earlier, with the below change this goes away.
>>
>>> +
>>> +		br_stp_get_xstats(p, &xstats);
>>> +
>>> +		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats),
&xstats))
>>> +			goto nla_put_failure;
>>
>> Could you please follow how mcast xstats are dumped and do something
similar ?
>> It'd be nice to have similar code to audit.
> 
> Sure. I would also love to have easily auditable code in net/bridge. For
> the bridge STP xstats I followed the VLAN xstats code above, which does:
> 
>     if (nla_put(skb, BRIDGE_XSTATS_VLAN, sizeof(vxi), &vxi))
>         goto nla_put_failure;
> 
Yeah, we can move that to a vlan-specific helper too, but that is an unrelated
change.
> But I can change the STP xstats code to the following:
> 
>     if (p) {
>         nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
>                                 sizeof(struct bridge_stp_xstats),
>                                 BRIDGE_XSTATS_PAD);
>         if (!nla)
>             goto nla_put_failure;
> 
>         br_stp_get_xstats(p, nla_data(nla));
>     }
> 
> Would that be preferred?
> 
> 
Perfect, thanks!
> Thanks,
> 
> 	Vivien
>