[PATCH 0/2] Combine standard and phy timestamping logic

[PATCH 0/2] Combine standard and phy timestamping logic

[Alexander Duyck]

This change makes it so that the core path for the phy timestamping logic
is shared between skb_tx_tstamp and skb_complete_tx_timestamp.  In addition
it provides a means of using the same skb clone type path in non phy
timestamping drivers.

The main motivation for this is to enable non-phy drivers to be able to
manipulate tx timestamp skbs for such things as putting them in lists or
setting aside buffer in the context block.

---

Alexander Duyck (2):
      net-timestamp: Merge shared code between phy and regular timestamping
      net-timestamp: Make the clone operation stand-alone from phy timestamping


 include/linux/skbuff.h  |    2 +
 net/core/skbuff.c       |   86 ++++++++++++++++++++++++++++++++++++-----------
 net/core/timestamping.c |   43 ++----------------------
 3 files changed, 70 insertions(+), 61 deletions(-)

-- 

[PATCH 1/2] net-timestamp: Merge shared code between phy and regular timestamping

[Alexander Duyck]

This change merges the shared bits that exist between skb_tx_tstamp and
skb_complete_tx_timestamp.  By doing this we can avoid the two diverging as
there were already changes pushed into skb_tx_tstamp that hadn't made it
into the other function.

In addition this resolves issues with the fact that
skb_complete_tx_timestamp was included in linux/skbuff.h even though it was
only compiled in if phy timestamping was enabled.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 net/core/skbuff.c       |   70 ++++++++++++++++++++++++++++++++---------------
 net/core/timestamping.c |   29 -------------------
 2 files changed, 47 insertions(+), 52 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 53ce536..488d902 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3511,33 +3511,13 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
 }
 EXPORT_SYMBOL(sock_dequeue_err_skb);
 
-void __skb_tstamp_tx(struct sk_buff *orig_skb,
-		     struct skb_shared_hwtstamps *hwtstamps,
-		     struct sock *sk, int tstype)
+static void __skb_complete_tx_timestamp(struct sk_buff *skb,
+					struct sock *sk,
+					int tstype)
 {
 	struct sock_exterr_skb *serr;
-	struct sk_buff *skb;
 	int err;
 
-	if (!sk)
-		return;
-
-	if (hwtstamps) {
-		*skb_hwtstamps(orig_skb) =
-			*hwtstamps;
-	} else {
-		/*
-		 * no hardware time stamps available,
-		 * so keep the shared tx_flags and only
-		 * store software time stamp
-		 */
-		orig_skb->tstamp = ktime_get_real();
-	}
-
-	skb = skb_clone(orig_skb, GFP_ATOMIC);
-	if (!skb)
-		return;
-
 	serr = SKB_EXT_ERR(skb);
 	memset(serr, 0, sizeof(*serr));
 	serr->ee.ee_errno = ENOMSG;
@@ -3554,6 +3534,50 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 	if (err)
 		kfree_skb(skb);
 }
+
+void skb_complete_tx_timestamp(struct sk_buff *skb,
+			       struct skb_shared_hwtstamps *hwtstamps)
+{
+	struct sock *sk = skb->sk;
+
+	skb->sk = NULL;
+
+	if (hwtstamps) {
+		*skb_hwtstamps(skb) = *hwtstamps;
+		__skb_complete_tx_timestamp(skb, sk, SCM_TSTAMP_SND);
+	} else {
+		kfree_skb(skb);
+	}
+
+	sock_put(sk);
+}
+EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
+
+void __skb_tstamp_tx(struct sk_buff *orig_skb,
+		     struct skb_shared_hwtstamps *hwtstamps,
+		     struct sock *sk, int tstype)
+{
+	struct sk_buff *skb;
+
+	if (!sk)
+		return;
+
+	/*
+	 * no hardware time stamps available,
+	 * so keep the shared tx_flags and only
+	 * store software time stamp
+	 */
+	if (!hwtstamps)
+		orig_skb->tstamp = ktime_get_real();
+	else
+		*skb_hwtstamps(orig_skb) = *hwtstamps;
+
+	skb = skb_clone(orig_skb, GFP_ATOMIC);
+	if (!skb)
+		return;
+
+	__skb_complete_tx_timestamp(skb, sk, tstype);
+}
 EXPORT_SYMBOL_GPL(__skb_tstamp_tx);
 
 void skb_tstamp_tx(struct sk_buff *orig_skb,
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index a877039..f48a59f 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -63,35 +63,6 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
 }
 EXPORT_SYMBOL_GPL(skb_clone_tx_timestamp);
 
-void skb_complete_tx_timestamp(struct sk_buff *skb,
-			       struct skb_shared_hwtstamps *hwtstamps)
-{
-	struct sock *sk = skb->sk;
-	struct sock_exterr_skb *serr;
-	int err;
-
-	if (!hwtstamps) {
-		sock_put(sk);
-		kfree_skb(skb);
-		return;
-	}
-
-	*skb_hwtstamps(skb) = *hwtstamps;
-
-	serr = SKB_EXT_ERR(skb);
-	memset(serr, 0, sizeof(*serr));
-	serr->ee.ee_errno = ENOMSG;
-	serr->ee.ee_origin = SO_EE_ORIGIN_TIMESTAMPING;
-	skb->sk = NULL;
-
-	err = sock_queue_err_skb(sk, skb);
-
-	sock_put(sk);
-	if (err)
-		kfree_skb(skb);
-}
-EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
-
 bool skb_defer_rx_timestamp(struct sk_buff *skb)
 {
 	struct phy_device *phydev;

[PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping

[Alexander Duyck]

The phy timestamping takes a different path than the regular timestamping
does in that it will create a clone first so that the packets needing to be
timestamped can be placed in a queue, or the context block could be used.

In order to support these use cases I am pulling the core of the code out
so it can be used in other drivers beyond just phy devices.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 include/linux/skbuff.h  |    2 ++
 net/core/skbuff.c       |   20 ++++++++++++++++++++
 net/core/timestamping.c |   14 +++-----------
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 02529fc..19eb787 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2690,6 +2690,8 @@ static inline ktime_t net_invalid_timestamp(void)
 	return ktime_set(0, 0);
 }
 
+struct sk_buff *__skb_clone_tx_timestamp(struct sk_buff *skb);
+
 #ifdef CONFIG_NETWORK_PHY_TIMESTAMPING
 
 void skb_clone_tx_timestamp(struct sk_buff *skb);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 488d902..3f5cf29 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3511,6 +3511,26 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
 }
 EXPORT_SYMBOL(sock_dequeue_err_skb);
 
+struct sk_buff *__skb_clone_tx_timestamp(struct sk_buff *skb)
+{
+	struct sock *sk = skb->sk;
+	struct sk_buff *clone;
+
+	if (!sk || !atomic_inc_not_zero(&sk->sk_refcnt))
+		return NULL;
+
+	clone = skb_clone(skb, GFP_ATOMIC);
+	if (!clone) {
+		sock_put(sk);
+		return NULL;
+	}
+
+	clone->sk = sk;
+
+	return clone;
+}
+EXPORT_SYMBOL(__skb_clone_tx_timestamp);
+
 static void __skb_complete_tx_timestamp(struct sk_buff *skb,
 					struct sock *sk,
 					int tstype)
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index f48a59f..458f5cb 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -36,10 +36,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
 {
 	struct phy_device *phydev;
 	struct sk_buff *clone;
-	struct sock *sk = skb->sk;
 	unsigned int type;
 
-	if (!sk)
+	if (!skb->sk)
 		return;
 
 	type = classify(skb);
@@ -48,16 +47,9 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
 
 	phydev = skb->dev->phydev;
 	if (likely(phydev->drv->txtstamp)) {
-		if (!atomic_inc_not_zero(&sk->sk_refcnt))
+		clone = __skb_clone_tx_timestamp(skb);
+		if (!clone)
 			return;
-
-		clone = skb_clone(skb, GFP_ATOMIC);
-		if (!clone) {
-			sock_put(sk);
-			return;
-		}
-
-		clone->sk = sk;
 		phydev->drv->txtstamp(phydev, clone, type);
 	}
 }

Re: [PATCH 1/2] net-timestamp: Merge shared code between phy and regular timestamping

[Willem de Bruijn]

> +}
> +EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
> +
> +void __skb_tstamp_tx(struct sk_buff *orig_skb,
> +                    struct skb_shared_hwtstamps *hwtstamps,
> +                    struct sock *sk, int tstype)
> +{
> +       struct sk_buff *skb;
> +
> +       if (!sk)
> +               return;
> +
> +       /*
> +        * no hardware time stamps available,
> +        * so keep the shared tx_flags and only
> +        * store software time stamp
> +        */

This comment only makes sense within the else {} clause. As a matter
of fact, I'd just drop it. The patch also inverts the branch. I would
keep the original order, as that is also the order in
skb_complete_tx_timestamp.

> +       if (!hwtstamps)
> +               orig_skb->tstamp = ktime_get_real();
> +       else
> +               *skb_hwtstamps(orig_skb) = *hwtstamps;
> +

Otherwise looks great to me. The code deduplicates shared code between
__skb_tstamp_tx and skb_complete_timestamp without introduces logical
changes to either codepath.

Re: [PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping

[Willem de Bruijn]

On Wed, Sep 3, 2014 at 11:53 AM, Alexander Duyck
<alexander.h.duyck@intel.com> wrote:
> The phy timestamping takes a different path than the regular timestamping
> does in that it will create a clone first so that the packets needing to be
> timestamped can be placed in a queue, or the context block could be used.
>
> In order to support these use cases I am pulling the core of the code out
> so it can be used in other drivers beyond just phy devices.

Do you already have additional such use cases?

> +struct sk_buff *__skb_clone_tx_timestamp(struct sk_buff *skb)
> +{
> +       struct sock *sk = skb->sk;
> +       struct sk_buff *clone;
> +
> +       if (!sk || !atomic_inc_not_zero(&sk->sk_refcnt))
> +               return NULL;
> +
> +       clone = skb_clone(skb, GFP_ATOMIC);
> +       if (!clone) {
> +               sock_put(sk);
> +               return NULL;
> +       }
> +
> +       clone->sk = sk;
> +
> +       return clone;
> +}
> +EXPORT_SYMBOL(__skb_clone_tx_timestamp);
> +

Code looks great. Again, this can be verified to be a functional noop.
One minor comment is that this really is not a timestamping function,
but an skb_clone variant. skb_clone_sk?

Re: [PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping

[Eric Dumazet]

On Wed, 2014-09-03 at 11:53 -0400, Alexander Duyck wrote:
> The phy timestamping takes a different path than the regular timestamping
> does in that it will create a clone first so that the packets needing to be
> timestamped can be placed in a queue, or the context block could be used.
> 
> In order to support these use cases I am pulling the core of the code out
> so it can be used in other drivers beyond just phy devices.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>  include/linux/skbuff.h  |    2 ++
>  net/core/skbuff.c       |   20 ++++++++++++++++++++
>  net/core/timestamping.c |   14 +++-----------
>  3 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 02529fc..19eb787 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2690,6 +2690,8 @@ static inline ktime_t net_invalid_timestamp(void)
>  	return ktime_set(0, 0);
>  }
>  
> +struct sk_buff *__skb_clone_tx_timestamp(struct sk_buff *skb);
> +
>  #ifdef CONFIG_NETWORK_PHY_TIMESTAMPING
>  
>  void skb_clone_tx_timestamp(struct sk_buff *skb);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 488d902..3f5cf29 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3511,6 +3511,26 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
>  }
>  EXPORT_SYMBOL(sock_dequeue_err_skb);
>  
> +struct sk_buff *__skb_clone_tx_timestamp(struct sk_buff *skb)
> +{
> +	struct sock *sk = skb->sk;
> +	struct sk_buff *clone;
> +
> +	if (!sk || !atomic_inc_not_zero(&sk->sk_refcnt))


> +		return NULL;
> +
> +	clone = skb_clone(skb, GFP_ATOMIC);
> +	if (!clone) {
> +		sock_put(sk);
> +		return NULL;
> +	}
> +
> +	clone->sk = sk;
> +
> +	return clone;
> +}
> +EXPORT_SYMBOL(__skb_clone_tx_timestamp);


Normally, if one skb holds a reference to a socket, it should have
skb->destructor set to a cleanup function.

Otherwise, we rely on callers following a convention, to release sk
reference.

If you believe its needed, it should be dully documented.

Thanks

Re: [PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping

[Alexander Duyck]

On 09/03/2014 11:54 AM, Willem de Bruijn wrote:
> On Wed, Sep 3, 2014 at 11:53 AM, Alexander Duyck
> <alexander.h.duyck@intel.com> wrote:
>> The phy timestamping takes a different path than the regular timestamping
>> does in that it will create a clone first so that the packets needing to be
>> timestamped can be placed in a queue, or the context block could be used.
>>
>> In order to support these use cases I am pulling the core of the code out
>> so it can be used in other drivers beyond just phy devices.
> 
> Do you already have additional such use cases?

I have a driver that I am working on that I will probably push in a
couple of weeks that will make use of this interface.  I basically need
to maintain a list of SKBs as I can multiple timestamps out for
completion at the same time.

>> +struct sk_buff *__skb_clone_tx_timestamp(struct sk_buff *skb)
>> +{
>> +       struct sock *sk = skb->sk;
>> +       struct sk_buff *clone;
>> +
>> +       if (!sk || !atomic_inc_not_zero(&sk->sk_refcnt))
>> +               return NULL;
>> +
>> +       clone = skb_clone(skb, GFP_ATOMIC);
>> +       if (!clone) {
>> +               sock_put(sk);
>> +               return NULL;
>> +       }
>> +
>> +       clone->sk = sk;
>> +
>> +       return clone;
>> +}
>> +EXPORT_SYMBOL(__skb_clone_tx_timestamp);
>> +
> 
> Code looks great. Again, this can be verified to be a functional noop.
> One minor comment is that this really is not a timestamping function,
> but an skb_clone variant. skb_clone_sk?

Let me think about this one.  Between the comment Eric had about
skb->destructor and the fact that this is essentially just forking the
skb so we can hold it for the reply I might be able to come up with a
better solution.

I'm thinking it might be worthwhile to create a simple destructor as
then I could probably tear out several spots in the phy timestamping
code where it is having to use skb_complete_tx_timestamp to free the
frames that are allocated using the approach in this function.

Thanks,

Alex

Re: [PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping

[Eric Dumazet]

On Wed, 2014-09-03 at 14:07 -0700, Eric Dumazet wrote:

> 
> Normally, if one skb holds a reference to a socket, it should have
> skb->destructor set to a cleanup function.
> 
> Otherwise, we rely on callers following a convention, to release sk
> reference.
> 
> If you believe its needed, it should be dully documented.


BTW, skb_orphan() would BUG() on this case (skb->destrutor == NULL and
skb->sk != NULL)

Re: [PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping

[Alexander Duyck]

On 09/03/2014 03:05 PM, Eric Dumazet wrote:
> On Wed, 2014-09-03 at 14:07 -0700, Eric Dumazet wrote:
> 
>>
>> Normally, if one skb holds a reference to a socket, it should have
>> skb->destructor set to a cleanup function.
>>
>> Otherwise, we rely on callers following a convention, to release sk
>> reference.
>>
>> If you believe its needed, it should be dully documented.
> 
> 
> BTW, skb_orphan() would BUG() on this case (skb->destrutor == NULL and
> skb->sk != NULL)

That was why we were setting skb->sk to null before passing it to
sock_queue_err_skb.  I think I found the reason why things were done the
way they were.  It looks like skb_orphan is called in
sock_queue_err_skb.  So any destructor added would be fired there before
before being replaced.

The only part I am not sure about is if that would actually be any sort
of issue.  Do we really need to hold the extra reference to the socket
all the way through the sock_queue_err_skb call or can we just let it
get dropped with the call to skb_orphan and let the fact that the
message is being queued be good enough?

I'm not sure due to the comment about "before exiting rcu section, make
sure dst is refcounted".  It kind of implies I should be able to hand
off the reference counts without worrying about the socket being freed
out from under me.

Thanks,

Alex

Re: [PATCH 2/2] net-timestamp: Make the clone operation stand-alone from phy timestamping

[Eric Dumazet]

On Wed, 2014-09-03 at 16:02 -0700, Alexander Duyck wrote:
> On 09/03/2014 03:05 PM, Eric Dumazet wrote:
> > On Wed, 2014-09-03 at 14:07 -0700, Eric Dumazet wrote:
> > 
> >>
> >> Normally, if one skb holds a reference to a socket, it should have
> >> skb->destructor set to a cleanup function.
> >>
> >> Otherwise, we rely on callers following a convention, to release sk
> >> reference.
> >>
> >> If you believe its needed, it should be dully documented.
> > 
> > 
> > BTW, skb_orphan() would BUG() on this case (skb->destrutor == NULL and
> > skb->sk != NULL)
> 
> That was why we were setting skb->sk to null before passing it to
> sock_queue_err_skb.

Hmpff... This is scary. skb_clone_tx_timestamp() seems the source of
confusion.

>   I think I found the reason why things were done the
> way they were.  It looks like skb_orphan is called in
> sock_queue_err_skb.

Thats standard practice before setting skb->destructor

>   So any destructor added would be fired there before
> before being replaced.

You can use a pair of sock_hold()/sock_put() to guard against early
dismantle. Look at commit da92b194cc36b5dc1fbd 

>  
> 
> The only part I am not sure about is if that would actually be any sort
> of issue.  Do we really need to hold the extra reference to the socket
> all the way through the sock_queue_err_skb call or can we just let it
> get dropped with the call to skb_orphan and let the fact that the
> message is being queued be good enough?


> I'm not sure due to the comment about "before exiting rcu section, make
> sure dst is refcounted".  It kind of implies I should be able to hand
> off the reference counts without worrying about the socket being freed
> out from under me.

Thats a different concern : dst in input path are rcu protected.

If we queue one skb with an attached dst, we must take a refcount on the
dst, or risk the dst being freed under us, by the time we dequeue
packet.

That will not prevent sk from being destroyed.