Re: [PATCH net-next 4/4] net: dsa: bcm_sf2: Add VLAN support

[Vivien Didelot]

Hi Florian,

Florian Fainelli  writes:

> Another possible approach would have been to allocate the vlan structure
> upong port_vlan_prepare() though that would typically result in more
> fragmentation over time once se start using more VLANs.

This is indeed what the switchdev prepare phase is for. If anything goes
wrong before calling the commit phase, switchdev will free the allocated
data.

Thanks,

Vivien

Re: [PATCH net-next 4/4] net: dsa: bcm_sf2: Add VLAN support

[Andrew Lunn]

On Fri, Jun 10, 2016 at 11:47:48AM -0700, Florian Fainelli wrote:
> On 06/10/2016 05:00 AM, Andrew Lunn wrote:
> >> @@ -148,6 +155,9 @@ struct bcm_sf2_priv {
> >>struct device_node  *master_mii_dn;
> >>struct mii_bus  *slave_mii_bus;
> >>struct mii_bus  *master_mii_bus;
> >> +
> >> +  /* Cache of programmed VLANs */
> >> +  struct bcm_sf2_vlan vlans[VLAN_N_VID];
> > 
> > Hi Florian
> > 
> > This is a 16Kbyte array. So i assume the whole priv structure needs 5
> > pages. Have you had any trouble allocating this much memory,
> > particularly once it has been used for a while and fragmented?
> 
> Well, since this is using the old binding, we can't unload the driver,
> it's built into the kernel, so initializes early enough we have got
> plenty of memory.

Don't you want to use the new binding at some point?

> > I just wondered if it might be better to use vmalloc() for the
> > vlans.
> 
> That's a very good point, I can't really see a drawback to doing this,
> will submit a patch moving this to a dynamic allocation.
> 
> Another possible approach would have been to allocate the vlan structure
> upong port_vlan_prepare() though that would typically result in more
> fragmentation over time once se start using more VLANs.

It is a trade off, code complexity vs saved memory. I don't think such
a table is too bad, assuming your not trying to run the whole system
in 32Mbyes of RAM.

   Andrew

Re: [PATCH net-next 4/4] net: dsa: bcm_sf2: Add VLAN support

[Florian Fainelli]

On 06/10/2016 05:00 AM, Andrew Lunn wrote:
>> @@ -148,6 +155,9 @@ struct bcm_sf2_priv {
>>  struct device_node  *master_mii_dn;
>>  struct mii_bus  *slave_mii_bus;
>>  struct mii_bus  *master_mii_bus;
>> +
>> +/* Cache of programmed VLANs */
>> +struct bcm_sf2_vlan vlans[VLAN_N_VID];
> 
> Hi Florian
> 
> This is a 16Kbyte array. So i assume the whole priv structure needs 5
> pages. Have you had any trouble allocating this much memory,
> particularly once it has been used for a while and fragmented?

Well, since this is using the old binding, we can't unload the driver,
it's built into the kernel, so initializes early enough we have got
plenty of memory.

> 
> I just wondered if it might be better to use vmalloc() for the
> vlans.

That's a very good point, I can't really see a drawback to doing this,
will submit a patch moving this to a dynamic allocation.

Another possible approach would have been to allocate the vlan structure
upong port_vlan_prepare() though that would typically result in more
fragmentation over time once se start using more VLANs.
-- 
Florian

Re: [PATCH net-next 4/4] net: dsa: bcm_sf2: Add VLAN support

[Andrew Lunn]

> @@ -148,6 +155,9 @@ struct bcm_sf2_priv {
>   struct device_node  *master_mii_dn;
>   struct mii_bus  *slave_mii_bus;
>   struct mii_bus  *master_mii_bus;
> +
> + /* Cache of programmed VLANs */
> + struct bcm_sf2_vlan vlans[VLAN_N_VID];

Hi Florian

This is a 16Kbyte array. So i assume the whole priv structure needs 5
pages. Have you had any trouble allocating this much memory,
particularly once it has been used for a while and fragmented?

I just wondered if it might be better to use vmalloc() for the
vlans.

Andrew