Patchwork [01/34] net: add skb_dst_set_noref_force

login
register
mail settings
Submitter Simon Horman
Date March 29, 2013, 4:11 a.m.
Message ID <1364530311-11512-2-git-send-email-horms@verge.net.au>
Download mbox | patch
Permalink /patch/232264/
State Accepted
Headers show

Comments

Simon Horman - March 29, 2013, 4:11 a.m.
From: Julian Anastasov <ja@ssi.bg>

Rename skb_dst_set_noref to __skb_dst_set_noref and
add force flag as suggested by David Miller. The new wrapper
skb_dst_set_noref_force will force dst entries that are not
cached to be attached as skb dst without taking reference
as long as provided dst is reclaimed after RCU grace period.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
Signed-off by: Hans Schillstrom <hans@schillstrom.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 include/linux/skbuff.h |   35 ++++++++++++++++++++++++++++++++++-
 net/core/dst.c         |    9 +++++----
 2 files changed, 39 insertions(+), 5 deletions(-)
Pablo Neira - April 1, 2013, 12:06 p.m.
Hi Simon,

On Fri, Mar 29, 2013 at 01:11:18PM +0900, Simon Horman wrote:
> From: Julian Anastasov <ja@ssi.bg>
> 
> Rename skb_dst_set_noref to __skb_dst_set_noref and
> add force flag as suggested by David Miller. The new wrapper
> skb_dst_set_noref_force will force dst entries that are not
> cached to be attached as skb dst without taking reference
> as long as provided dst is reclaimed after RCU grace period.
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> Signed-off by: Hans Schillstrom <hans@schillstrom.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>

I think was acked by David, right?

> ---
>  include/linux/skbuff.h |   35 ++++++++++++++++++++++++++++++++++-
>  net/core/dst.c         |    9 +++++----
>  2 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 878e0ee..364e244 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -575,7 +575,40 @@ static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst)
>  	skb->_skb_refdst = (unsigned long)dst;
>  }
>  
> -extern void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst);
> +extern void __skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst,
> +				bool force);
> +
> +/**
> + * skb_dst_set_noref - sets skb dst, hopefully, without taking reference
> + * @skb: buffer
> + * @dst: dst entry
> + *
> + * Sets skb dst, assuming a reference was not taken on dst.
> + * If dst entry is cached, we do not take reference and dst_release
> + * will be avoided by refdst_drop. If dst entry is not cached, we take
> + * reference, so that last dst_release can destroy the dst immediately.
> + */
> +static inline void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
> +{
> +	__skb_dst_set_noref(skb, dst, false);
> +}
> +
> +/**
> + * skb_dst_set_noref_force - sets skb dst, without taking reference
> + * @skb: buffer
> + * @dst: dst entry
> + *
> + * Sets skb dst, assuming a reference was not taken on dst.
> + * No reference is taken and no dst_release will be called. While for
> + * cached dsts deferred reclaim is a basic feature, for entries that are
> + * not cached it is caller's job to guarantee that last dst_release for
> + * provided dst happens when nobody uses it, eg. after a RCU grace period.
> + */
> +static inline void skb_dst_set_noref_force(struct sk_buff *skb,
> +					   struct dst_entry *dst)
> +{
> +	__skb_dst_set_noref(skb, dst, true);
> +}
>  
>  /**
>   * skb_dst_is_noref - Test if skb dst isn't refcounted
> diff --git a/net/core/dst.c b/net/core/dst.c
> index 35fd12f..df9cc81 100644
> --- a/net/core/dst.c
> +++ b/net/core/dst.c
> @@ -320,27 +320,28 @@ void __dst_destroy_metrics_generic(struct dst_entry *dst, unsigned long old)
>  EXPORT_SYMBOL(__dst_destroy_metrics_generic);
>  
>  /**
> - * skb_dst_set_noref - sets skb dst, without a reference
> + * __skb_dst_set_noref - sets skb dst, without a reference
>   * @skb: buffer
>   * @dst: dst entry
> + * @force: if force is set, use noref version even for DST_NOCACHE entries
>   *
>   * Sets skb dst, assuming a reference was not taken on dst
>   * skb_dst_drop() should not dst_release() this dst
>   */
> -void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
> +void __skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst, bool force)
>  {
>  	WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
>  	/* If dst not in cache, we must take a reference, because
>  	 * dst_release() will destroy dst as soon as its refcount becomes zero
>  	 */
> -	if (unlikely(dst->flags & DST_NOCACHE)) {
> +	if (unlikely((dst->flags & DST_NOCACHE) && !force)) {
>  		dst_hold(dst);
>  		skb_dst_set(skb, dst);
>  	} else {
>  		skb->_skb_refdst = (unsigned long)dst | SKB_DST_NOREF;
>  	}
>  }
> -EXPORT_SYMBOL(skb_dst_set_noref);
> +EXPORT_SYMBOL(__skb_dst_set_noref);
>  
>  /* Dirty hack. We did it in 2.2 (in __dst_free),
>   * we have _very_ good reasons not to repeat
> -- 
> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 1, 2013, 4:57 p.m.
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 1 Apr 2013 14:06:44 +0200

> Hi Simon,
> 
> On Fri, Mar 29, 2013 at 01:11:18PM +0900, Simon Horman wrote:
>> From: Julian Anastasov <ja@ssi.bg>
>> 
>> Rename skb_dst_set_noref to __skb_dst_set_noref and
>> add force flag as suggested by David Miller. The new wrapper
>> skb_dst_set_noref_force will force dst entries that are not
>> cached to be attached as skb dst without taking reference
>> as long as provided dst is reclaimed after RCU grace period.
>> 
>> Signed-off-by: Julian Anastasov <ja@ssi.bg>
>> Signed-off by: Hans Schillstrom <hans@schillstrom.com>
>> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> I think was acked by David, right?

It was:

Acked-by: David S. Miller <davem@davemloft.net>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - April 1, 2013, 10:42 p.m.
On Mon, Apr 01, 2013 at 12:57:33PM -0400, David Miller wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Mon, 1 Apr 2013 14:06:44 +0200
> 
> > Hi Simon,
> > 
> > On Fri, Mar 29, 2013 at 01:11:18PM +0900, Simon Horman wrote:
> >> From: Julian Anastasov <ja@ssi.bg>
> >> 
> >> Rename skb_dst_set_noref to __skb_dst_set_noref and
> >> add force flag as suggested by David Miller. The new wrapper
> >> skb_dst_set_noref_force will force dst entries that are not
> >> cached to be attached as skb dst without taking reference
> >> as long as provided dst is reclaimed after RCU grace period.
> >> 
> >> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> >> Signed-off by: Hans Schillstrom <hans@schillstrom.com>
> >> Signed-off-by: Simon Horman <horms@verge.net.au>
> > 
> > I think was acked by David, right?
> 
> It was:
> 
> Acked-by: David S. Miller <davem@davemloft.net>

I have added this missing Acked-by to the patch. Simon, you'll have to
rebase your tree, sorry.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman - April 2, 2013, 1:11 a.m.
On Tue, Apr 02, 2013 at 12:42:30AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Apr 01, 2013 at 12:57:33PM -0400, David Miller wrote:
> > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > Date: Mon, 1 Apr 2013 14:06:44 +0200
> > 
> > > Hi Simon,
> > > 
> > > On Fri, Mar 29, 2013 at 01:11:18PM +0900, Simon Horman wrote:
> > >> From: Julian Anastasov <ja@ssi.bg>
> > >> 
> > >> Rename skb_dst_set_noref to __skb_dst_set_noref and
> > >> add force flag as suggested by David Miller. The new wrapper
> > >> skb_dst_set_noref_force will force dst entries that are not
> > >> cached to be attached as skb dst without taking reference
> > >> as long as provided dst is reclaimed after RCU grace period.
> > >> 
> > >> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> > >> Signed-off by: Hans Schillstrom <hans@schillstrom.com>
> > >> Signed-off-by: Simon Horman <horms@verge.net.au>
> > > 
> > > I think was acked by David, right?
> > 
> > It was:
> > 
> > Acked-by: David S. Miller <davem@davemloft.net>
> 
> I have added this missing Acked-by to the patch. Simon, you'll have to
> rebase your tree, sorry.

Thanks. Sorry for the mix up.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 878e0ee..364e244 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -575,7 +575,40 @@  static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst)
 	skb->_skb_refdst = (unsigned long)dst;
 }
 
-extern void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst);
+extern void __skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst,
+				bool force);
+
+/**
+ * skb_dst_set_noref - sets skb dst, hopefully, without taking reference
+ * @skb: buffer
+ * @dst: dst entry
+ *
+ * Sets skb dst, assuming a reference was not taken on dst.
+ * If dst entry is cached, we do not take reference and dst_release
+ * will be avoided by refdst_drop. If dst entry is not cached, we take
+ * reference, so that last dst_release can destroy the dst immediately.
+ */
+static inline void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
+{
+	__skb_dst_set_noref(skb, dst, false);
+}
+
+/**
+ * skb_dst_set_noref_force - sets skb dst, without taking reference
+ * @skb: buffer
+ * @dst: dst entry
+ *
+ * Sets skb dst, assuming a reference was not taken on dst.
+ * No reference is taken and no dst_release will be called. While for
+ * cached dsts deferred reclaim is a basic feature, for entries that are
+ * not cached it is caller's job to guarantee that last dst_release for
+ * provided dst happens when nobody uses it, eg. after a RCU grace period.
+ */
+static inline void skb_dst_set_noref_force(struct sk_buff *skb,
+					   struct dst_entry *dst)
+{
+	__skb_dst_set_noref(skb, dst, true);
+}
 
 /**
  * skb_dst_is_noref - Test if skb dst isn't refcounted
diff --git a/net/core/dst.c b/net/core/dst.c
index 35fd12f..df9cc81 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -320,27 +320,28 @@  void __dst_destroy_metrics_generic(struct dst_entry *dst, unsigned long old)
 EXPORT_SYMBOL(__dst_destroy_metrics_generic);
 
 /**
- * skb_dst_set_noref - sets skb dst, without a reference
+ * __skb_dst_set_noref - sets skb dst, without a reference
  * @skb: buffer
  * @dst: dst entry
+ * @force: if force is set, use noref version even for DST_NOCACHE entries
  *
  * Sets skb dst, assuming a reference was not taken on dst
  * skb_dst_drop() should not dst_release() this dst
  */
-void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
+void __skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst, bool force)
 {
 	WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
 	/* If dst not in cache, we must take a reference, because
 	 * dst_release() will destroy dst as soon as its refcount becomes zero
 	 */
-	if (unlikely(dst->flags & DST_NOCACHE)) {
+	if (unlikely((dst->flags & DST_NOCACHE) && !force)) {
 		dst_hold(dst);
 		skb_dst_set(skb, dst);
 	} else {
 		skb->_skb_refdst = (unsigned long)dst | SKB_DST_NOREF;
 	}
 }
-EXPORT_SYMBOL(skb_dst_set_noref);
+EXPORT_SYMBOL(__skb_dst_set_noref);
 
 /* Dirty hack. We did it in 2.2 (in __dst_free),
  * we have _very_ good reasons not to repeat