Subject: Re: [PATCH v3 net-next] net: vlan: Use vlan_prio instead of vlan_qos in mapping

From: Simon Horman <[email-masked]>
Date: Sat, 19 Oct 2024 09:30:40 +0100

+ Ido and Guilliame

On Fri, Oct 18, 2024 at 10:12:33PM +0800, Yajun Deng wrote:
> The vlan_qos member is used to save the vlan qos, but we only save the
> priority. Also, we will get the priority in vlan netlink and proc.
> We can just save the vlan priority using vlan_prio, so we can use vlan_prio
> to get the priority directly.
> 
> For flexibility, we introduced vlan_dev_get_egress_priority() helper
> function. After this patch, we will call vlan_dev_get_egress_priority()
> instead of vlan_dev_get_egress_qos_mask() in irdma.ko and rdma_cm.ko.
> Because we don't need the shift and mask operations anymore.
> 
> There is no functional changes.
> 
> Signed-off-by: Yajun Deng <[email-masked]>

Hi Ido and Guilliame,

I'm wondering if you could take a look over this and provide
any feedback you might have.

Thanks!

> ---
> v3: Remove the restriction that the maximum vlan priority is 7.
> v2: Add more detailed comments and tests.

v2 is here: https://lore.kernel.org/netdev/[email-masked]/

> v1: https://lore.kernel.org/all/[email-masked]/
> ---
>  include/linux/if_vlan.h  | 26 +++++++++++++++++++-------
>  net/8021q/vlan.h         |  4 ++--
>  net/8021q/vlan_dev.c     | 23 ++++++++++++-----------
>  net/8021q/vlan_netlink.c |  4 ++--
>  net/8021q/vlanproc.c     |  4 ++--
>  5 files changed, 37 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index c1645c86eed9..7cc36853c017 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -150,12 +150,12 @@ extern __be16 vlan_dev_vlan_proto(const struct net_device *dev);
>  /**
>   *	struct vlan_priority_tci_mapping - vlan egress priority mappings
>   *	@priority: skb priority
> - *	@vlan_qos: vlan priority: (skb->priority << 13) & 0xE000
> + *	@vlan_prio: vlan priority
>   *	@next: pointer to next struct
>   */
>  struct vlan_priority_tci_mapping {
>  	u32					priority;
> -	u16					vlan_qos;
> +	u8					vlan_prio;
>  	struct vlan_priority_tci_mapping	*next;
>  };
>  
> @@ -204,8 +204,8 @@ static inline struct vlan_dev_priv *vlan_dev_priv(const struct net_device *dev)
>  	return netdev_priv(dev);
>  }
>  
> -static inline u16
> -vlan_dev_get_egress_qos_mask(struct net_device *dev, u32 skprio)
> +static inline u8
> +vlan_dev_get_egress_priority(struct net_device *dev, u32 skprio)
>  {
>  	struct vlan_priority_tci_mapping *mp;
>  
> @@ -214,15 +214,21 @@ vlan_dev_get_egress_qos_mask(struct net_device *dev, u32 skprio)
>  	mp = vlan_dev_priv(dev)->egress_priority_map[(skprio & 0xF)];
>  	while (mp) {
>  		if (mp->priority == skprio) {
> -			return mp->vlan_qos; /* This should already be shifted
> -					      * to mask correctly with the
> -					      * VLAN's TCI */
> +			return mp->vlan_prio;
>  		}
>  		mp = mp->next;
>  	}
>  	return 0;
>  }
>  
> +static inline u16
> +vlan_dev_get_egress_qos_mask(struct net_device *dev, u32 skprio)
> +{
> +	u8 vlan_prio = vlan_dev_get_egress_priority(dev, skprio);
> +
> +	return (vlan_prio << VLAN_PRIO_SHIFT) & VLAN_PRIO_MASK;
> +}
> +
>  extern bool vlan_do_receive(struct sk_buff **skb);
>  
>  extern int vlan_vid_add(struct net_device *dev, __be16 proto, u16 vid);
> @@ -269,6 +275,12 @@ static inline __be16 vlan_dev_vlan_proto(const struct net_device *dev)
>  	return 0;
>  }
>  
> +static inline u8 vlan_dev_get_egress_priority(struct net_device *dev,
> +					      u32 skprio)
> +{
> +	return 0;
> +}
> +
>  static inline u16 vlan_dev_get_egress_qos_mask(struct net_device *dev,
>  					       u32 skprio)
>  {
> diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
> index 5eaf38875554..b28875c4ac86 100644
> --- a/net/8021q/vlan.h
> +++ b/net/8021q/vlan.h
> @@ -126,9 +126,9 @@ void vlan_filter_drop_vids(struct vlan_info *vlan_info, __be16 proto);
>  
>  /* found in vlan_dev.c */
>  void vlan_dev_set_ingress_priority(const struct net_device *dev,
> -				   u32 skb_prio, u16 vlan_prio);
> +				   u32 skb_prio, u8 vlan_prio);
>  int vlan_dev_set_egress_priority(const struct net_device *dev,
> -				 u32 skb_prio, u16 vlan_prio);
> +				 u32 skb_prio, u8 vlan_prio);
>  void vlan_dev_free_egress_priority(const struct net_device *dev);
>  int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask);
>  void vlan_dev_get_realdev_name(const struct net_device *dev, char *result,
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 458040e8a0e0..5d153f1a7963 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -155,35 +155,36 @@ static int vlan_dev_change_mtu(struct net_device *dev, int new_mtu)
>  }
>  
>  void vlan_dev_set_ingress_priority(const struct net_device *dev,
> -				   u32 skb_prio, u16 vlan_prio)
> +				   u32 skb_prio, u8 vlan_prio)
>  {
>  	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
>  
> -	if (vlan->ingress_priority_map[vlan_prio & 0x7] && !skb_prio)
> +	vlan_prio = vlan_prio & 0x7;
> +	if (vlan->ingress_priority_map[vlan_prio] && !skb_prio)
>  		vlan->nr_ingress_mappings--;
> -	else if (!vlan->ingress_priority_map[vlan_prio & 0x7] && skb_prio)
> +	else if (!vlan->ingress_priority_map[vlan_prio] && skb_prio)
>  		vlan->nr_ingress_mappings++;
>  
> -	vlan->ingress_priority_map[vlan_prio & 0x7] = skb_prio;
> +	vlan->ingress_priority_map[vlan_prio] = skb_prio;
>  }
>  
>  int vlan_dev_set_egress_priority(const struct net_device *dev,
> -				 u32 skb_prio, u16 vlan_prio)
> +				 u32 skb_prio, u8 vlan_prio)
>  {
>  	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
>  	struct vlan_priority_tci_mapping *mp = NULL;
>  	struct vlan_priority_tci_mapping *np;
> -	u32 vlan_qos = (vlan_prio << VLAN_PRIO_SHIFT) & VLAN_PRIO_MASK;
>  
> +	vlan_prio = vlan_prio & 0x7;
>  	/* See if a priority mapping exists.. */
>  	mp = vlan->egress_priority_map[skb_prio & 0xF];
>  	while (mp) {
>  		if (mp->priority == skb_prio) {
> -			if (mp->vlan_qos && !vlan_qos)
> +			if (mp->vlan_prio && !vlan_prio)
>  				vlan->nr_egress_mappings--;
> -			else if (!mp->vlan_qos && vlan_qos)
> +			else if (!mp->vlan_prio && vlan_prio)
>  				vlan->nr_egress_mappings++;
> -			mp->vlan_qos = vlan_qos;
> +			mp->vlan_prio = vlan_prio;
>  			return 0;
>  		}
>  		mp = mp->next;
> @@ -197,14 +198,14 @@ int vlan_dev_set_egress_priority(const struct net_device *dev,
>  
>  	np->next = mp;
>  	np->priority = skb_prio;
> -	np->vlan_qos = vlan_qos;
> +	np->vlan_prio = vlan_prio;
>  	/* Before inserting this element in hash table, make sure all its fields
>  	 * are committed to memory.
>  	 * coupled with smp_rmb() in vlan_dev_get_egress_qos_mask()
>  	 */
>  	smp_wmb();
>  	vlan->egress_priority_map[skb_prio & 0xF] = np;
> -	if (vlan_qos)
> +	if (vlan_prio)
>  		vlan->nr_egress_mappings++;
>  	return 0;
>  }
> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
> index cf5219df7903..f62d8320c5b4 100644
> --- a/net/8021q/vlan_netlink.c
> +++ b/net/8021q/vlan_netlink.c
> @@ -261,11 +261,11 @@ static int vlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  		for (i = 0; i < ARRAY_SIZE(vlan->egress_priority_map); i++) {
>  			for (pm = vlan->egress_priority_map[i]; pm;
>  			     pm = pm->next) {
> -				if (!pm->vlan_qos)
> +				if (!pm->vlan_prio)
>  					continue;
>  
>  				m.from = pm->priority;
> -				m.to   = (pm->vlan_qos >> 13) & 0x7;
> +				m.to   = pm->vlan_prio;
>  				if (nla_put(skb, IFLA_VLAN_QOS_MAPPING,
>  					    sizeof(m), &m))
>  					goto nla_put_failure;
> diff --git a/net/8021q/vlanproc.c b/net/8021q/vlanproc.c
> index fa67374bda49..a5a5b8fbb054 100644
> --- a/net/8021q/vlanproc.c
> +++ b/net/8021q/vlanproc.c
> @@ -266,8 +266,8 @@ static int vlandev_seq_show(struct seq_file *seq, void *offset)
>  		const struct vlan_priority_tci_mapping *mp
>  			= vlan->egress_priority_map[i];
>  		while (mp) {
> -			seq_printf(seq, "%u:%d ",
> -				   mp->priority, ((mp->vlan_qos >> 13) & 0x7));
> +			seq_printf(seq, "%u:%u ",
> +				   mp->priority, mp->vlan_prio);
>  			mp = mp->next;
>  		}
>  	}
> -- 
> 2.25.1
> 
>