| Message ID | 20140813091605.GA2947@himangi-Dell |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2014-08-13 at 14:46 +0530, Himangi Saraogi wrote: > Continue is not needed at the bottom of a loop. > > The Coccinelle semantic patch implementing this change is: > > @@ > @@ > > for (...;...;...) { > ... > if (...) { > ... > - continue; > } > } > > Signed-off-by: Himangi Saraogi <himangi774@gmail.com> > Acked-by: Julia Lawall <julia.lawall@lip6.fr> > --- > net/ipv4/udp_offload.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > index 59035bc..62d5b9b 100644 > --- a/net/ipv4/udp_offload.c > +++ b/net/ipv4/udp_offload.c > @@ -269,10 +269,8 @@ unflush: > continue; > > uh2 = (struct udphdr *)(p->data + off); > - if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) { > + if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) > NAPI_GRO_CB(p)->same_flow = 0; > - continue; > - } > } > > skb_gro_pull(skb, sizeof(struct udphdr)); /* pull encapsulating udp header */ Please do not do that. If we add another check later, we'll miss that the 'continue;' needs to be put back. GRO stack is quite difficult to maintain, I prefer we keep this as is for consistency and code readability. Every time we clear same_flow, we use the "continue;" construct. if (...) { NAPI_GRO_CB(p)->same_flow = 0; continue; } <here we can eventually add some other check> Compiler generates the same code anyway. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 13, 2014 at 07:48:11AM -0700, Eric Dumazet wrote: > On Wed, 2014-08-13 at 14:46 +0530, Himangi Saraogi wrote: > > Continue is not needed at the bottom of a loop. > > > > The Coccinelle semantic patch implementing this change is: > > > > @@ > > @@ > > > > for (...;...;...) { > > ... > > if (...) { > > ... > > - continue; > > } > > } > > > > Signed-off-by: Himangi Saraogi <himangi774@gmail.com> > > Acked-by: Julia Lawall <julia.lawall@lip6.fr> > > --- > > net/ipv4/udp_offload.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > index 59035bc..62d5b9b 100644 > > --- a/net/ipv4/udp_offload.c > > +++ b/net/ipv4/udp_offload.c > > @@ -269,10 +269,8 @@ unflush: > > continue; > > > > uh2 = (struct udphdr *)(p->data + off); > > - if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) { > > + if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) > > NAPI_GRO_CB(p)->same_flow = 0; > > - continue; > > - } > > } > > > > skb_gro_pull(skb, sizeof(struct udphdr)); /* pull encapsulating udp header */ > > > Please do not do that. > > If we add another check later, we'll miss that the 'continue;' needs to > be put back. > > GRO stack is quite difficult to maintain, I prefer we keep this as is > for consistency and code readability. +1 this code as-is is definitely more readable -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index 59035bc..62d5b9b 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -269,10 +269,8 @@ unflush: continue; uh2 = (struct udphdr *)(p->data + off); - if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) { + if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) NAPI_GRO_CB(p)->same_flow = 0; - continue; - } } skb_gro_pull(skb, sizeof(struct udphdr)); /* pull encapsulating udp header */
Continue is not needed at the bottom of a loop. The Coccinelle semantic patch implementing this change is: @@ @@ for (...;...;...) { ... if (...) { ... - continue; } } Signed-off-by: Himangi Saraogi <himangi774@gmail.com> Acked-by: Julia Lawall <julia.lawall@lip6.fr> --- net/ipv4/udp_offload.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)