| Message ID | 20191125142452.21819-1-radoslawx.tyl@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
| Series | [v2] ixgbevf: Remove limit of 10 entries for unicast filter list | expand |
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@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
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
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@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.
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@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.
> -----Original Message----- > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On > Behalf Of Radoslaw Tyl > Sent: Monday, November 25, 2019 6:25 AM > To: intel-wired-lan@lists.osuosl.org > Cc: pmenzel@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.
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
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;
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(-)