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

Eelco Chaudron echaudro at redhat.com
Fri May 21 08:41:25 UTC 2021



On 20 May 2021, at 19:09, Vasu Dasari wrote:

> Thank you Eelco for testing the patch.
>
> My responses are inline:


Thanks, looking forward for your next revision. Would be good if you can add a test case for the mac move and flush change.

//Eelco


> *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