[ovs-dev] [PATCH v3] ofproto-dpif: APIs and CLI option to add/delete static fdb entry

Vasu Dasari vdasari at gmail.com
Thu May 20 17:09:07 UTC 2021


Thank you Eelco for testing the patch.

My responses are inline:

*Vasu Dasari*


On Thu, May 20, 2021 at 5:20 AM Eelco Chaudron <echaudro at redhat.com> wrote:

>
>
> On 14 May 2021, at 21:33, Vasu Dasari wrote:
>
> > Currently there is an option to add/flush/show ARP/ND neighbor. This
> covers L3
> > side.  For L2 side, there is only fdb show command. This patch gives an
> option
> > to add/del an fdb entry via ovs-appctl. ovs-appctl command looks like
> this:
> >
> > To add:
> >     ovs-appctl fdb/add <bridge> <port> <vlan> <Mac>
> >     ovs-appctl fdb/add br0 p1 0 50:54:00:00:00:05
> >
> > To del:
> >     ovs-appctl fdb/del <bridge> <port> <vlan> <Mac>
> >     ovs-appctl fdb/del br0 p1 0 50:54:00:00:00:05
> >
> > Static entry should not age. To indicate that entry being programmed is
> a static entry,
> > 'expires' field in 'struct mac_entry' will be set to a
> MAC_ENTRY_AGE_STATIC_ENTRY. A
> > check for this value is made while deleting mac entry as part of regular
> aging process.
> > Another check as part of mac-update process, when a packet with same
> source mac as this
> > entry arrives on the configured port will not modify the expires field
> >
> > Added two new APIs to provide convenient interface to add and delete
> static-macs.
> > void xlate_add_static_mac_entry(const struct ofproto_dpif *, ofp_port_t
> in_port,
> >                                struct eth_addr dl_src, int vlan);
> > void xlate_delete_static_mac_entry(const struct ofproto_dpif *,
> >                                   struct eth_addr dl_src, int vlan);
> >
> > Signed-off-by: Vasu Dasari <vdasari at gmail.com>
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048894.html
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1597752
>
> I did some testing and found the issues below, once fixed, I’ll do a full
> code review.

When you do an FDB flush, it also clears the static FDB entries. I think
> this is wrong, as all hardware vendors I know will retain the static FDB
> entries.
>

I took the liberty of having fdb/flush clear static entries as well. I do
not mind changing the code to delete only dynamic ones. Will take care of
this.


> When you create a static entry for lets say Port A, when a packet with the
> same MAC comes from Port B the entry will be updated with Port B. This
> should not happen for static entries.
>
> I agree. I tested this case too. It fails. Will fix it.

> When you add a static MAC entry, the command just returns "OK". Other
> commands do not return anything on a successful addition. You should either
> follow the same behavior or be more verbose (Static FDB successfully
> added?) on your return.
>
> Sure, will make the command return error string only if it fails,
otherwise will be silent.

> Also, it might be nice to be more verbose when you replace an existing
> static or dynamic FDB entry, i.e. especially if the physical port is
> different (mac move case).
>
>
Sure, will add more detailed output.

> Cheers,
>
>
> Eelco
>
>


More information about the dev mailing list