[Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for unicast filter list

[Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for unicast filter list

[Paul Menzel]

Dear Radoslaw,


On 2019-11-25 15:24, Radoslaw Tyl wrote:
> Currently, though the FDB entry is added to VF, it does not appear in
> RAR filters. VF driver only allows to add 10 entries. Attempting to add
> another causes an error. This patch removes limitation and allows use of
> all free RAR entries for the FDB if needed.

I still wonder, why the limit was introduced in the first place.
gregory.v.rose at intel.com bounces, so we can?t ask.

> Fixes: 46ec20ff7d ("ixgbevf: Add macvlan support in the set rx mode op")
> 
> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> index 076f2da36f27..64ec0e7c64b4 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> @@ -2081,11 +2081,6 @@ static int ixgbevf_write_uc_addr_list(struct net_device *netdev)
>  	struct ixgbe_hw *hw = &adapter->hw;
>  	int count = 0;
>  
> -	if ((netdev_uc_count(netdev)) > 10) {
> -		pr_err("Too many unicast filters - No Space\n");
> -		return -ENOSPC;
> -	}
> -
>  	if (!netdev_uc_empty(netdev)) {
>  		struct netdev_hw_addr *ha;

Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5174 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20191125/27e30d36/attachment.p7s>

[Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for unicast filter list

[Paul Menzel]

Dear Radoslaw,


Just a note, that the date of your patch message is from the future.

> Date: Mon, 25 Nov 2019 15:24:52 +0100

I guess that?s by design so it always shows up at the top of the
inbox for a certain amount of time. ;-)


Kind regards,

Paul

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5174 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20191125/b88c725c/attachment.p7s>

[Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for unicast filter list

[Radoslaw Tyl]

Currently, though the FDB entry is added to VF, it does not appear in
RAR filters. VF driver only allows to add 10 entries. Attempting to add
another causes an error. This patch removes limitation and allows use of
all free RAR entries for the FDB if needed.

Fixes: 46ec20ff7d ("ixgbevf: Add macvlan support in the set rx mode op")

Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 076f2da36f27..64ec0e7c64b4 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -2081,11 +2081,6 @@ static int ixgbevf_write_uc_addr_list(struct net_device *netdev)
 	struct ixgbe_hw *hw = &adapter->hw;
 	int count = 0;
 
-	if ((netdev_uc_count(netdev)) > 10) {
-		pr_err("Too many unicast filters - No Space\n");
-		return -ENOSPC;
-	}
-
 	if (!netdev_uc_empty(netdev)) {
 		struct netdev_hw_addr *ha;
 
-- 
2.21.0

[Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for unicast filter list

[Alexander Duyck]

On Mon, Nov 25, 2019 at 5:32 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Radoslaw,
>
>
> On 2019-11-25 15:24, Radoslaw Tyl wrote:
> > Currently, though the FDB entry is added to VF, it does not appear in
> > RAR filters. VF driver only allows to add 10 entries. Attempting to add
> > another causes an error. This patch removes limitation and allows use of
> > all free RAR entries for the FDB if needed.
>
> I still wonder, why the limit was introduced in the first place.
> gregory.v.rose at intel.com bounces, so we can?t ask.

I've added Greg's current email address in case he has some memory for
why the limit of 10 was added.

It probably has to do with wanting to prevent starvation of resources.
The hardware itself only supports 128 total RAR entries. So if we have
63 VFs and 1 PF, and 15 of PF macvlans, then we would only have 49
entries to spare that are then shared. So at best this is only pushing
things out to 49 since at that point we are out of RAR entries anyway.

> > Fixes: 46ec20ff7d ("ixgbevf: Add macvlan support in the set rx mode op")

I wouldn't bother with the fixes tag since it isn't "fixing" things.
It is changing behavior. I would say it was by design that it was
limited to 10 entries. All this change does is push the work onto the
PF for returning an error instead of doing so earlier.

For a normal NIC the failure case here would be to enable promiscuous
mode. However since this is a VF you cannot do that. Instead it might
make more sense to dump a message when you hit the limit.

> > Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > index 076f2da36f27..64ec0e7c64b4 100644
> > --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > @@ -2081,11 +2081,6 @@ static int ixgbevf_write_uc_addr_list(struct net_device *netdev)
> >       struct ixgbe_hw *hw = &adapter->hw;
> >       int count = 0;
> >
> > -     if ((netdev_uc_count(netdev)) > 10) {
> > -             pr_err("Too many unicast filters - No Space\n");
> > -             return -ENOSPC;
> > -     }
> > -
> >       if (!netdev_uc_empty(netdev)) {
> >               struct netdev_hw_addr *ha;

I would say NAK. The problem here is this doesn't solve the original
problem. It just masks it by pushing the failure out to the
set_uc_addr call which doesn't have the return value checked so
instead you will get a silent failure.

[Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for unicast filter list

[Gregory Rose]

On 11/25/2019 10:23 AM, Alexander Duyck wrote:
> On Mon, Nov 25, 2019 at 5:32 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>> Dear Radoslaw,
>>
>>
>> On 2019-11-25 15:24, Radoslaw Tyl wrote:
>>> Currently, though the FDB entry is added to VF, it does not appear in
>>> RAR filters. VF driver only allows to add 10 entries. Attempting to add
>>> another causes an error. This patch removes limitation and allows use of
>>> all free RAR entries for the FDB if needed.
>> I still wonder, why the limit was introduced in the first place.
>> gregory.v.rose at intel.com bounces, so we can?t ask.
> I've added Greg's current email address in case he has some memory for
> why the limit of 10 was added.
>
> It probably has to do with wanting to prevent starvation of resources.
> The hardware itself only supports 128 total RAR entries. So if we have
> 63 VFs and 1 PF, and 15 of PF macvlans, then we would only have 49
> entries to spare that are then shared. So at best this is only pushing
> things out to 49 since at that point we are out of RAR entries anyway.

Hi Alex,

It's tough to recall exactly what my thinking was - 8 years is a long 
time.? However, I think you're
right that this is about resource sharing and not allowing any single VF 
to consume all the remaining
RAR entries.? Ten entries seems arbitrary but I do recall at the time a 
common test setup was with
4 VFs.? Also, we needed to reserve RAR entries for the PF too IIRC.

Maybe Sibai can recall, I don't know if she's still at Intel but maybe 
ask her as well.

Sorry I couldn't be more help.

Regards,

- Greg

>
>>> Fixes: 46ec20ff7d ("ixgbevf: Add macvlan support in the set rx mode op")
> I wouldn't bother with the fixes tag since it isn't "fixing" things.
> It is changing behavior. I would say it was by design that it was
> limited to 10 entries. All this change does is push the work onto the
> PF for returning an error instead of doing so earlier.
>
> For a normal NIC the failure case here would be to enable promiscuous
> mode. However since this is a VF you cannot do that. Instead it might
> make more sense to dump a message when you hit the limit.
>
>>> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 -----
>>>   1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> index 076f2da36f27..64ec0e7c64b4 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> @@ -2081,11 +2081,6 @@ static int ixgbevf_write_uc_addr_list(struct net_device *netdev)
>>>        struct ixgbe_hw *hw = &adapter->hw;
>>>        int count = 0;
>>>
>>> -     if ((netdev_uc_count(netdev)) > 10) {
>>> -             pr_err("Too many unicast filters - No Space\n");
>>> -             return -ENOSPC;
>>> -     }
>>> -
>>>        if (!netdev_uc_empty(netdev)) {
>>>                struct netdev_hw_addr *ha;
> I would say NAK. The problem here is this doesn't solve the original
> problem. It just masks it by pushing the failure out to the
> set_uc_addr call which doesn't have the return value checked so
> instead you will get a silent failure.

[Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for unicast filter list

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Radoslaw Tyl
> Sent: Monday, November 25, 2019 6:25 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: pmenzel at molgen.mpg.de; Gangadhar, Mukesh
> <mukesh.gangadhar@intel.com>
> Subject: [Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for
> unicast filter list
> 
> Currently, though the FDB entry is added to VF, it does not appear in RAR
> filters. VF driver only allows to add 10 entries. Attempting to add another
> causes an error. This patch removes limitation and allows use of all free RAR
> entries for the FDB if needed.
> 
> Fixes: 46ec20ff7d ("ixgbevf: Add macvlan support in the set rx mode op")
> 
> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 -----
>  1 file changed, 5 deletions(-)

Exceeding the filter limits causes a call trace on reboot and locks the system up until you do a hard power cycle.

[Intel-wired-lan] [PATCH v2] ixgbevf: Remove limit of 10 entries for unicast filter list

[Jacob Keller]

On 11/25/2019 11:14 AM, Gregory Rose wrote:
> 
> It's tough to recall exactly what my thinking was - 8 years is a long 
> time.? However, I think you're
> right that this is about resource sharing and not allowing any single VF 
> to consume all the remaining
> RAR entries.? Ten entries seems arbitrary but I do recall at the time a 
> common test setup was with
> 4 VFs.? Also, we needed to reserve RAR entries for the PF too IIRC.
> 
> Maybe Sibai can recall, I don't know if she's still at Intel but maybe 
> ask her as well.
> 
> Sorry I couldn't be more help.
> 
> Regards,
> 
> - Greg

Right. This is what I would have thought as well. By not limiting, we
potentially allow one VF to hog all of the resources.

It's plausible that this limit ought to be configurable instead of
static. This way, a system administrator could change the limit on the
PF and enable more entries than the static limit of 10.

That obviously requires more plumbing in place to represent the limit
and find an adequate way of informing the PF system administrator...

Thanks,
Jake