diff mbox series

[net-next,v1,04/11] vxlan: vxlan_rcv(): Extract vxlan_hdr(skb) to a named variable

Message ID d28c09cf04d210255882d7f370862f60e8f7fdf3.1733235367.git.petrm@nvidia.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series vxlan: Support user-defined reserved bits | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
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: 305 this patch: 305
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 37 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-12-04--03-00 (tests: 760)

Commit Message

Petr Machata Dec. 3, 2024, 2:30 p.m. UTC 
Having a named reference to the VXLAN header is more handy than having to
conjure it anew through vxlan_hdr() on every use. Add a new variable and
convert several open-coded sites.

Additionally, convert one "unparsed" use to the new variable as well. Thus
the only "unparsed" uses that remain are the flag-clearing and the header
validity check at the end.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---

Notes:
CC: Andrew Lunn <andrew+netdev@lunn.ch>
CC: Menglong Dong <menglong8.dong@gmail.com>
CC: Guillaume Nault <gnault@redhat.com>
CC: Alexander Lobakin <aleksander.lobakin@intel.com>
CC: Breno Leitao <leitao@debian.org>

 drivers/net/vxlan/vxlan_core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Mateusz Polchlopek Dec. 4, 2024, 12:59 p.m. UTC | #1 
On 12/3/2024 3:30 PM, Petr Machata wrote:
> Having a named reference to the VXLAN header is more handy than having to
> conjure it anew through vxlan_hdr() on every use. Add a new variable and
> convert several open-coded sites.
> 
> Additionally, convert one "unparsed" use to the new variable as well. Thus
> the only "unparsed" uses that remain are the flag-clearing and the header
> validity check at the end.
> 
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> ---
> 
> Notes:
> CC: Andrew Lunn <andrew+netdev@lunn.ch>
> CC: Menglong Dong <menglong8.dong@gmail.com>
> CC: Guillaume Nault <gnault@redhat.com>
> CC: Alexander Lobakin <aleksander.lobakin@intel.com>
> CC: Breno Leitao <leitao@debian.org>
> 
>   drivers/net/vxlan/vxlan_core.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 4905ed1c5e20..257411d1ccca 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1667,6 +1667,7 @@ static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,
>   static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>   {
>   	struct vxlan_vni_node *vninode = NULL;
> +	const struct vxlanhdr *vh;
>   	struct vxlan_dev *vxlan;
>   	struct vxlan_sock *vs;
>   	struct vxlanhdr unparsed;
> @@ -1685,11 +1686,11 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>   		goto drop;
>   
>   	unparsed = *vxlan_hdr(skb);
> +	vh = vxlan_hdr(skb);
>   	/* VNI flag always required to be set */
> -	if (!(unparsed.vx_flags & VXLAN_HF_VNI)) {
> +	if (!(vh->vx_flags & VXLAN_HF_VNI)) {
>   		netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
> -			   ntohl(vxlan_hdr(skb)->vx_flags),
> -			   ntohl(vxlan_hdr(skb)->vx_vni));
> +			   ntohl(vh->vx_flags), ntohl(vh->vx_vni));
>   		reason = SKB_DROP_REASON_VXLAN_INVALID_HDR;
>   		/* Return non vxlan pkt */
>   		goto drop;
> @@ -1701,7 +1702,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>   	if (!vs)
>   		goto drop;
>   
> -	vni = vxlan_vni(vxlan_hdr(skb)->vx_vni);
> +	vni = vxlan_vni(vh->vx_vni);
>   
>   	vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, &vninode);
>   	if (!vxlan) {
> @@ -1713,7 +1714,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>   	 * used by VXLAN extensions if explicitly requested.
>   	 */
>   	if (vxlan->cfg.flags & VXLAN_F_GPE) {
> -		if (!vxlan_parse_gpe_proto(vxlan_hdr(skb), &protocol))
> +		if (!vxlan_parse_gpe_proto(vh, &protocol))
>   			goto drop;
>   		unparsed.vx_flags &= ~VXLAN_GPE_USED_BITS;
>   		raw_proto = true;

Overall that's cool refactor but I wonder - couldn't it be somehow
merged with patch03? You touch vxlan_rcv function and the same
pieces of code in both patches, so maybe you can do that there?
Squash those two patches into one? It seems that in this patch you
change something you already changed in prev patch - maybe
it should be done in patch03? Or do I miss something?
Petr Machata Dec. 5, 2024, 10:44 a.m. UTC | #2 
Mateusz Polchlopek <mateusz.polchlopek@intel.com> writes:

> On 12/3/2024 3:30 PM, Petr Machata wrote:
>
>> @@ -1713,7 +1714,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>>   	 * used by VXLAN extensions if explicitly requested.
>>   	 */
>>   	if (vxlan->cfg.flags & VXLAN_F_GPE) {
>> -		if (!vxlan_parse_gpe_proto(vxlan_hdr(skb), &protocol))
>> +		if (!vxlan_parse_gpe_proto(vh, &protocol))
>>   			goto drop;
>>   		unparsed.vx_flags &= ~VXLAN_GPE_USED_BITS;
>>   		raw_proto = true;
>
> Overall that's cool refactor but I wonder - couldn't it be somehow
> merged with patch03? You touch vxlan_rcv function and the same
> pieces of code in both patches, so maybe you can do that there?
> Squash those two patches into one? It seems that in this patch you
> change something you already changed in prev patch - maybe
> it should be done in patch03? Or do I miss something?

Look, I'm juggling various bits back and forth and honestly it's all
much of a muchness. There's nothing obviously better whichever way you
package it. First changing to open-coded vxlan_hdr in 03 makes sense,
because it's already open-coded like that several times. Then we have a
clean 04 that replaces all the existing open-coded sites, including the
new one, thus everything is done in one go.

I'd just leave it as is, largely because I don't want to touch something
that works for frankly cosmetic reasons when the end result is the same.
Mateusz Polchlopek Dec. 5, 2024, 11:22 a.m. UTC | #3 
On 12/5/2024 11:44 AM, Petr Machata wrote:
> 
> Mateusz Polchlopek <mateusz.polchlopek@intel.com> writes:
> 
>> On 12/3/2024 3:30 PM, Petr Machata wrote:
>>
>>> @@ -1713,7 +1714,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>>>    	 * used by VXLAN extensions if explicitly requested.
>>>    	 */
>>>    	if (vxlan->cfg.flags & VXLAN_F_GPE) {
>>> -		if (!vxlan_parse_gpe_proto(vxlan_hdr(skb), &protocol))
>>> +		if (!vxlan_parse_gpe_proto(vh, &protocol))
>>>    			goto drop;
>>>    		unparsed.vx_flags &= ~VXLAN_GPE_USED_BITS;
>>>    		raw_proto = true;
>>
>> Overall that's cool refactor but I wonder - couldn't it be somehow
>> merged with patch03? You touch vxlan_rcv function and the same
>> pieces of code in both patches, so maybe you can do that there?
>> Squash those two patches into one? It seems that in this patch you
>> change something you already changed in prev patch - maybe
>> it should be done in patch03? Or do I miss something?
> 
> Look, I'm juggling various bits back and forth and honestly it's all
> much of a muchness. There's nothing obviously better whichever way you
> package it. First changing to open-coded vxlan_hdr in 03 makes sense,
> because it's already open-coded like that several times. Then we have a
> clean 04 that replaces all the existing open-coded sites, including the
> new one, thus everything is done in one go.
> 
> I'd just leave it as is, largely because I don't want to touch something
> that works for frankly cosmetic reasons when the end result is the same.

Okay, you convinced me, so for this patch:

Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
diff mbox series

Patch

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 4905ed1c5e20..257411d1ccca 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1667,6 +1667,7 @@  static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,
 static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
 {
 	struct vxlan_vni_node *vninode = NULL;
+	const struct vxlanhdr *vh;
 	struct vxlan_dev *vxlan;
 	struct vxlan_sock *vs;
 	struct vxlanhdr unparsed;
@@ -1685,11 +1686,11 @@  static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
 		goto drop;
 
 	unparsed = *vxlan_hdr(skb);
+	vh = vxlan_hdr(skb);
 	/* VNI flag always required to be set */
-	if (!(unparsed.vx_flags & VXLAN_HF_VNI)) {
+	if (!(vh->vx_flags & VXLAN_HF_VNI)) {
 		netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
-			   ntohl(vxlan_hdr(skb)->vx_flags),
-			   ntohl(vxlan_hdr(skb)->vx_vni));
+			   ntohl(vh->vx_flags), ntohl(vh->vx_vni));
 		reason = SKB_DROP_REASON_VXLAN_INVALID_HDR;
 		/* Return non vxlan pkt */
 		goto drop;
@@ -1701,7 +1702,7 @@  static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
 	if (!vs)
 		goto drop;
 
-	vni = vxlan_vni(vxlan_hdr(skb)->vx_vni);
+	vni = vxlan_vni(vh->vx_vni);
 
 	vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, &vninode);
 	if (!vxlan) {
@@ -1713,7 +1714,7 @@  static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
 	 * used by VXLAN extensions if explicitly requested.
 	 */
 	if (vxlan->cfg.flags & VXLAN_F_GPE) {
-		if (!vxlan_parse_gpe_proto(vxlan_hdr(skb), &protocol))
+		if (!vxlan_parse_gpe_proto(vh, &protocol))
 			goto drop;
 		unparsed.vx_flags &= ~VXLAN_GPE_USED_BITS;
 		raw_proto = true;