Re: [net-next v2 04/11] bridge: cfm: Kernel space implementation of CFM.

[Nikolay Aleksandrov]

On Thu, 2020-10-01 at 10:30 +0000, Henrik Bjoernlund wrote:
> This is the first commit of the implementation of the CFM protocol
> according to 802.1Q section 12.14.
> 
> Connectivity Fault Management (CFM) comprises capabilities for
> detecting, verifying, and isolating connectivity failures in
> Virtual Bridged Networks. These capabilities can be used in
> networks operated by multiple independent organizations, each
> with restricted management access to each other<E2><80><99>s equipment.
> 
> CFM functions are partitioned as follows:
>     - Path discovery
>     - Fault detection
>     - Fault verification and isolation
>     - Fault notification
>     - Fault recovery
> 
> Interface consists of these functions:
> br_cfm_mep_create()
> br_cfm_mep_delete()
> br_cfm_mep_config_set()
> br_cfm_cc_config_set()
> br_cfm_cc_peer_mep_add()
> br_cfm_cc_peer_mep_remove()
> 
> A MEP instance is created by br_cfm_mep_create()
>     -It is the Maintenance association End Point
>      described in 802.1Q section 19.2.
>     -It is created on a specific level (1-7) and is assuring
>      that no CFM frames are passing through this MEP on lower levels.
>     -It initiates and validates CFM frames on its level.
>     -It can only exist on a port that is related to a bridge.
>     -Attributes given cannot be changed until the instance is
>      deleted.
> 
> A MEP instance can be deleted by br_cfm_mep_delete().
> 
> A created MEP instance has attributes that can be
> configured by br_cfm_mep_config_set().
> 
> A MEP Continuity Check feature can be configured by
> br_cfm_cc_config_set()
>     The Continuity Check Receiver state machine can be
>     enabled and disabled.
>     According to 802.1Q section 19.2.8
> 
> A MEP can have Peer MEPs added and removed by
> br_cfm_cc_peer_mep_add() and br_cfm_cc_peer_mep_remove()
>     The Continuity Check feature can maintain connectivity
>     status on each added Peer MEP.
> 
> Reviewed-by: Horatiu Vultur  <horatiu.vultur@xxxxxxxxxxxxx>
> Signed-off-by: Henrik Bjoernlund  <henrik.bjoernlund@xxxxxxxxxxxxx>
> ---

Thank you for breaking the big patch into 3 smaller pieces, but could you please
name them appropriately? I'm sure they add different things, so just give them
something more descriptive. Having the same subject for 3 patches looks odd.

>  include/uapi/linux/cfm_bridge.h |  23 +++
>  net/bridge/Makefile             |   2 +
>  net/bridge/br_cfm.c             | 263 ++++++++++++++++++++++++++++++++
>  net/bridge/br_private_cfm.h     |  61 ++++++++
>  4 files changed, 349 insertions(+)
>  create mode 100644 include/uapi/linux/cfm_bridge.h
>  create mode 100644 net/bridge/br_cfm.c
>  create mode 100644 net/bridge/br_private_cfm.h
> 
[snip]
> +
> +	mep = kzalloc(sizeof(*mep), GFP_KERNEL);
> +	if (!mep)
> +		return -ENOMEM;
> +
> +	mep->create = *create;
> +	mep->instance = instance;
> +	rcu_assign_pointer(mep->b_port, p);
> +
> +	INIT_HLIST_HEAD(&mep->peer_mep_list);
> +
> +	hlist_add_tail_rcu(&mep->head, &br->mep_list);
> +
> +	return 0;
> +}
> +
> +static void mep_delete_implementation(struct net_bridge *br,
> +				      struct br_cfm_mep *mep)
> +{
> +	struct br_cfm_peer_mep *peer_mep;
> +
> +	ASSERT_RTNL();
> +
> +	/* Empty and free peer MEP list */
> +	hlist_for_each_entry(peer_mep, &mep->peer_mep_list, head) {

hlist_for_each_entry_safe()

> +		hlist_del_rcu(&peer_mep->head);
> +		kfree_rcu(peer_mep, rcu);
> +	}
> +
> +	RCU_INIT_POINTER(mep->b_port, NULL);
> +	hlist_del_rcu(&mep->head);
> +	kfree_rcu(mep, rcu);
> +}