diff mbox series

[net-next] rocker: Explicitly mark learned FDB entries as offloaded

Message ID 20221031075922.1717163-1-idosch@nvidia.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] rocker: Explicitly mark learned FDB entries as offloaded | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ido Schimmel Oct. 31, 2022, 7:59 a.m. UTC 
Currently, FDB entries that are notified to the bridge driver via
'SWITCHDEV_FDB_ADD_TO_BRIDGE' are always marked as offloaded by the
bridge. With MAB enabled, this will no longer be universally true.
Device drivers will report locked FDB entries to the bridge to let it
know that the corresponding hosts required authorization, but it does
not mean that these entries are necessarily programmed in the underlying
hardware.

We would like to solve it by having the bridge driver determine the
offload indication based of the 'offloaded' bit in the FDB notification
[1].

Prepare for that change by having rocker explicitly mark learned FDB
entries as offloaded. This is consistent with all the other switchdev
drivers.

[1] https://lore.kernel.org/netdev/20221025100024.1287157-4-idosch@nvidia.com/

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/ethernet/rocker/rocker_ofdpa.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Ido Schimmel Oct. 31, 2022, 8:32 a.m. UTC | #1 
On Mon, Oct 31, 2022 at 09:59:22AM +0200, Ido Schimmel wrote:
> diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> index 58cf7cc54f40..f5880d0053da 100644
> --- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
> +++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> @@ -1828,12 +1828,14 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work)
>  	info.vid = lw->vid;
>  
>  	rtnl_lock();
> -	if (learned && removing)
> +	if (learned && removing) {
>  		call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
>  					 lw->ofdpa_port->dev, &info.info, NULL);
> -	else if (learned && !removing)
> +	} else if (learned && !removing) {
> +		info.offloaded = true;
>  		call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
>  					 lw->ofdpa_port->dev, &info.info, NULL);
> +	}
>  	rtnl_unlock();
>  
>  	kfree(work);

Looking at it again, this is better:

diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
index 58cf7cc54f40..4d17ae18ea61 100644
--- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
+++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
@@ -1826,6 +1826,7 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work)
 
        info.addr = lw->addr;
        info.vid = lw->vid;
+       info.offloaded = learned && !removing;
 
        rtnl_lock();
        if (learned && removing)

Will send another version tomorrow assuming no other comments.
Vladimir Oltean Oct. 31, 2022, 9:08 a.m. UTC | #2 
On Mon, Oct 31, 2022 at 10:32:04AM +0200, Ido Schimmel wrote:
> On Mon, Oct 31, 2022 at 09:59:22AM +0200, Ido Schimmel wrote:
> > diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > index 58cf7cc54f40..f5880d0053da 100644
> > --- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > +++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > @@ -1828,12 +1828,14 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work)
> >  	info.vid = lw->vid;
> >  
> >  	rtnl_lock();
> > -	if (learned && removing)
> > +	if (learned && removing) {
> >  		call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
> >  					 lw->ofdpa_port->dev, &info.info, NULL);
> > -	else if (learned && !removing)
> > +	} else if (learned && !removing) {
> > +		info.offloaded = true;
> >  		call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
> >  					 lw->ofdpa_port->dev, &info.info, NULL);
> > +	}
> >  	rtnl_unlock();
> >  
> >  	kfree(work);
> 
> Looking at it again, this is better:
> 
> diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> index 58cf7cc54f40..4d17ae18ea61 100644
> --- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
> +++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> @@ -1826,6 +1826,7 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work)
>  
>         info.addr = lw->addr;
>         info.vid = lw->vid;
> +       info.offloaded = learned && !removing;
>  
>         rtnl_lock();
>         if (learned && removing)
> 
> Will send another version tomorrow assuming no other comments.

It may also be an opportunity to not take rtnl_lock() if (!learned), and
this will in turn simplify the condition to just "info.offloaded = !removing"?

Actually this elimination of useless work should be done at the level of
ofdpa_port_fdb_learn(), if "flags" does not contain OFDPA_OP_FLAG_LEARNED.
Ido Schimmel Oct. 31, 2022, 11:53 a.m. UTC | #3 
On Mon, Oct 31, 2022 at 09:08:23AM +0000, Vladimir Oltean wrote:
> On Mon, Oct 31, 2022 at 10:32:04AM +0200, Ido Schimmel wrote:
> > On Mon, Oct 31, 2022 at 09:59:22AM +0200, Ido Schimmel wrote:
> > > diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > > index 58cf7cc54f40..f5880d0053da 100644
> > > --- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > > +++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > > @@ -1828,12 +1828,14 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work)
> > >  	info.vid = lw->vid;
> > >  
> > >  	rtnl_lock();
> > > -	if (learned && removing)
> > > +	if (learned && removing) {
> > >  		call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
> > >  					 lw->ofdpa_port->dev, &info.info, NULL);
> > > -	else if (learned && !removing)
> > > +	} else if (learned && !removing) {
> > > +		info.offloaded = true;
> > >  		call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
> > >  					 lw->ofdpa_port->dev, &info.info, NULL);
> > > +	}
> > >  	rtnl_unlock();
> > >  
> > >  	kfree(work);
> > 
> > Looking at it again, this is better:
> > 
> > diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > index 58cf7cc54f40..4d17ae18ea61 100644
> > --- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > +++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> > @@ -1826,6 +1826,7 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work)
> >  
> >         info.addr = lw->addr;
> >         info.vid = lw->vid;
> > +       info.offloaded = learned && !removing;
> >  
> >         rtnl_lock();
> >         if (learned && removing)
> > 
> > Will send another version tomorrow assuming no other comments.
> 
> It may also be an opportunity to not take rtnl_lock() if (!learned), and
> this will in turn simplify the condition to just "info.offloaded = !removing"?
> 
> Actually this elimination of useless work should be done at the level of
> ofdpa_port_fdb_learn(), if "flags" does not contain OFDPA_OP_FLAG_LEARNED.

OK, I have this as the first patch:

diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
index 58cf7cc54f40..77ad09ad8304 100644
--- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
+++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
@@ -1821,19 +1821,16 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work)
        const struct ofdpa_fdb_learn_work *lw =
                container_of(work, struct ofdpa_fdb_learn_work, work);
        bool removing = (lw->flags & OFDPA_OP_FLAG_REMOVE);
-       bool learned = (lw->flags & OFDPA_OP_FLAG_LEARNED);
        struct switchdev_notifier_fdb_info info = {};
+       enum switchdev_notifier_type event;
 
        info.addr = lw->addr;
        info.vid = lw->vid;
+       event = removing ? SWITCHDEV_FDB_DEL_TO_BRIDGE :
+                          SWITCHDEV_FDB_ADD_TO_BRIDGE;
 
        rtnl_lock();
-       if (learned && removing)
-               call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
-                                        lw->ofdpa_port->dev, &info.info, NULL);
-       else if (learned && !removing)
-               call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
-                                        lw->ofdpa_port->dev, &info.info, NULL);
+       call_switchdev_notifiers(event, lw->ofdpa_port->dev, &info.info, NULL);
        rtnl_unlock();
 
        kfree(work);
@@ -1865,6 +1862,9 @@ static int ofdpa_port_fdb_learn(struct ofdpa_port *ofdpa_port,
        if (!ofdpa_port_is_bridged(ofdpa_port))
                return 0;
 
+       if (!(flags & OFDPA_OP_FLAG_LEARNED))
+               return 0;
+
        lw = kzalloc(sizeof(*lw), GFP_ATOMIC);
        if (!lw)
                return -ENOMEM;
diff mbox series

Patch

diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
index 58cf7cc54f40..f5880d0053da 100644
--- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
+++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
@@ -1828,12 +1828,14 @@  static void ofdpa_port_fdb_learn_work(struct work_struct *work)
 	info.vid = lw->vid;
 
 	rtnl_lock();
-	if (learned && removing)
+	if (learned && removing) {
 		call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
 					 lw->ofdpa_port->dev, &info.info, NULL);
-	else if (learned && !removing)
+	} else if (learned && !removing) {
+		info.offloaded = true;
 		call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
 					 lw->ofdpa_port->dev, &info.info, NULL);
+	}
 	rtnl_unlock();
 
 	kfree(work);