[net-next,v2,4/6] gtp: consolidate pdp context destruction into helper

Message ID 20170130163713.17524-5-aschultz@tpip.net
State New
Headers show

Commit Message

Andreas Schultz Jan. 30, 2017, 4:37 p.m.
Consolidate duplicate code into helper.

Signed-off-by: Andreas Schultz <aschultz@tpip.net>
---
 drivers/net/gtp.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

Comments

Harald Welte Feb. 6, 2017, 1:58 p.m. | #1
Hi Andreas,

On Mon, Jan 30, 2017 at 05:37:11PM +0100, Andreas Schultz wrote:
> Consolidate duplicate code into helper.

Makes complete sense.

However:

> +static void pdp_context_delete(struct pdp_ctx *pctx);
> +

why introduce the forward-declaration and then define the function
further down in the file?  I think in general, it is preferred to put
helper functions on top, before their first use, so forward declarations
can be avoided?  Particularly if the helper function doesn't call any
other functions that are not yet declared at this point.

Please note the above might just be my personal taste, not sure if
that's a general habit in the kernel networking code these days.

So with or without the re-ordering:

Acked-by: Harald Welte <laforge@gnumonks.org>
Andreas Schultz Feb. 13, 2017, 2:14 p.m. | #2
----- On Feb 6, 2017, at 2:58 PM, laforge laforge@gnumonks.org wrote:

> Hi Andreas,
> 
> On Mon, Jan 30, 2017 at 05:37:11PM +0100, Andreas Schultz wrote:
>> Consolidate duplicate code into helper.
> 
> Makes complete sense.
> 
> However:
> 
>> +static void pdp_context_delete(struct pdp_ctx *pctx);
>> +
> 
> why introduce the forward-declaration and then define the function
> further down in the file?  I think in general, it is preferred to put
> helper functions on top, before their first use, so forward declarations
> can be avoided?  Particularly if the helper function doesn't call any
> other functions that are not yet declared at this point.

I wanted to keep functions that work on the data structure close
together. I this case the function the initialized the pdp context
and the delete function for it.

Andreas

> 
> Please note the above might just be my personal taste, not sure if
> that's a general habit in the kernel networking code these days.
> 
> So with or without the re-ordering:
> 
> Acked-by: Harald Welte <laforge@gnumonks.org>
> 
> --
> - Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
> ============================================================================
> "Privacy in residential applications is a desirable marketing option."
>                                                   (ETSI EN 300 175-7 Ch. A6)

Patch

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 6b7a3c2..68c6c9b 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -84,6 +84,8 @@  struct gtp_net {
 
 static u32 gtp_h_initval;
 
+static void pdp_context_delete(struct pdp_ctx *pctx);
+
 static inline u32 gtp0_hashfn(u64 tid)
 {
 	u32 *tid32 = (u32 *) &tid;
@@ -774,13 +776,10 @@  static void gtp_hashtable_free(struct gtp_dev *gtp)
 	struct pdp_ctx *pctx;
 	int i;
 
-	for (i = 0; i < gtp->hash_size; i++) {
-		hlist_for_each_entry_rcu(pctx, &gtp->tid_hash[i], hlist_tid) {
-			hlist_del_rcu(&pctx->hlist_tid);
-			hlist_del_rcu(&pctx->hlist_addr);
-			kfree_rcu(pctx, rcu_head);
-		}
-	}
+	for (i = 0; i < gtp->hash_size; i++)
+		hlist_for_each_entry_rcu(pctx, &gtp->tid_hash[i], hlist_tid)
+			pdp_context_delete(pctx);
+
 	synchronize_rcu();
 	kfree(gtp->addr_hash);
 	kfree(gtp->tid_hash);
@@ -985,6 +984,13 @@  static int ipv4_pdp_add(struct gtp_dev *gtp, struct genl_info *info)
 	return 0;
 }
 
+static void pdp_context_delete(struct pdp_ctx *pctx)
+{
+	hlist_del_rcu(&pctx->hlist_tid);
+	hlist_del_rcu(&pctx->hlist_addr);
+	kfree(pctx);
+}
+
 static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)
 {
 	struct gtp_dev *gtp;
@@ -1093,9 +1099,7 @@  static int gtp_genl_del_pdp(struct sk_buff *skb, struct genl_info *info)
 		pr_debug("GTPv1-U: deleting tunnel id = %x/%x (pdp %p)\n",
 			 pctx->u.v1.i_tei, pctx->u.v1.o_tei, pctx);
 
-	hlist_del_rcu(&pctx->hlist_tid);
-	hlist_del_rcu(&pctx->hlist_addr);
-	kfree_rcu(pctx, rcu_head);
+	pdp_context_delete(pctx);
 
 out_unlock:
 	rcu_read_unlock();