[RFC] net : add tx timestamp to packet mmap.

[RFC] net : add tx timestamp to packet mmap.

[Paul Chavent]

This patch allow to generate tx timestamps of packets sent by the packet mmap interface.

Actually, you can't get tx timestamps with the sample code below.

I wonder if my current implementation is good. And if not, how should i get the timestamps ?

Wouldn't be a good idea to put timestamps in the ring buffer frame before give it back to the user ?

Thanks for your comments.

/* BEGIN OF SAMPLE CODE */
struct timespec ts = {0,0};
struct sockaddr from_addr;
static uint8_t tmp_data[256];
struct iovec msg_iov = {tmp_data, sizeof(tmp_data)};
static uint8_t cmsg_buff[256];
struct msghdr msghdr = {&from_addr, sizeof(from_addr),
                        &msg_iov, 1,
                        cmsg_buff, sizeof(cmsg_buff),
                        0};

ssize_t err = recvmsg(itf->sock_fd, &msghdr, MSG_ERRQUEUE);
if(err < 0)
  {
    perror("recvmsg failed");
    return -1;
  }

struct cmsghdr *cmsg;
for(cmsg = CMSG_FIRSTHDR(&msghdr); cmsg != NULL; cmsg = CMSG_NXTHDR(&msghdr, cmsg))
{
  if(cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_TIMESTAMPING)
    {
      ts = *(struct timespec *)CMSG_DATA(cmsg);
      fprintf(stderr, "SCM_TIMESTAMPING available\n");
    }
  else if (cmsg->cmsg_level == SOL_PACKET && cmsg->cmsg_type == PACKET_TX_TIMESTAMP)
      {
        ts = *(struct timespec *)CMSG_DATA(cmsg);
        fprintf(stderr, "PACKET_TX_TIMESTAMP available\n");
      }
} 
/* END OF SAMPLE CODE */

Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
---
 net/packet/af_packet.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index e639645..948748b 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1857,6 +1857,10 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 	void *data;
 	int err;
 
+	err = sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
+	if (err < 0)
+		return err;
+
 	ph.raw = frame;
 
 	skb->protocol = proto;
-- 
1.7.12.1

Re: [RFC] net : add tx timestamp to packet mmap.

[David Miller]

You're changing the code that handles sendmsg() and then wondering why
a recvmsg() call doesn't provide a timestamp.

Re: [RFC] net : add tx timestamp to packet mmap.

[Paul Chavent]

After a sendmsg, we have to call recvmsg on the ERRQUEUE to get 
timestamp. I find that unfortunate indeed...

So this patch fix the tx timestamping (that take place in sendmsg), in 
order to be able to get timestamp (via recvmsg).

This seems suboptimal to me, that why i also ask if it wouldn't be 
possible to put the timestamp in the ring buffer frame before give it 
back to user.

Thanks for your reading.


On 12/12/2012 08:23 PM, David Miller wrote:
>
> You're changing the code that handles sendmsg() and then wondering why
> a recvmsg() call doesn't provide a timestamp.
>

Re: [RFC] net : add tx timestamp to packet mmap.

[Richard Cochran]

On Wed, Dec 12, 2012 at 04:29:25PM +0100, Paul Chavent wrote:
> This patch allow to generate tx timestamps of packets sent by the packet mmap interface.
> 
> Actually, you can't get tx timestamps with the sample code below.
> 
> I wonder if my current implementation is good. And if not, how should i get the timestamps ?

In order for time stamps to appear, somebody has to call
skb_tx_timestamp() ...

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index e639645..948748b 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1857,6 +1857,10 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
>  	void *data;
>  	int err;
>  
> +	err = sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);

and this call is only setting some flags.

HTH,
Richard

Re: [RFC] net : add tx timestamp to packet mmap.

[Paul Chavent]

Hello.

On 12/13/2012 02:29 PM, Richard Cochran wrote:
> On Wed, Dec 12, 2012 at 04:29:25PM +0100, Paul Chavent wrote:
>> This patch allow to generate tx timestamps of packets sent by the packet mmap interface.
>>
>> Actually, you can't get tx timestamps with the sample code below.
>>
>> I wonder if my current implementation is good. And if not, how should i get the timestamps ?
>
> In order for time stamps to appear, somebody has to call
> skb_tx_timestamp() ...
Yes. "Somebody" means "the hardware driver" after completing xmit. 
That's true ?

>
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index e639645..948748b 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -1857,6 +1857,10 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
>>   	void *data;
>>   	int err;
>>
>> +	err = sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
 >
 > and this call is only setting some flags.

Yes, it only sets some flags. I thought that those flags was required by 
the skb_tx_timestamp() in order to make the appropriate timestamping 
(hardware, software, etc).

So in order to have tx timestamp that work, both calls are needed ?

Why sock_tx_timestamp is called in packet_fill_skb and 
packet_sendmsg_spkt and not in tpacket_fill_skb ?
Why i can retrieve timestamps when i add this call ?

>
> HTH,
> Richard
>
Thank for your help.

Paul.

Re: [RFC] net : add tx timestamp to packet mmap.

[Richard Cochran]

On Thu, Dec 13, 2012 at 05:13:56PM +0100, Paul Chavent wrote:
> >
> >In order for time stamps to appear, somebody has to call
> >skb_tx_timestamp() ...
> Yes. "Somebody" means "the hardware driver" after completing xmit.
> That's true ?

Yes, the MAC driver must call this helper function, but not many
drivers do this yet. You didn't say which MAC driver you are using and
whether it supports Tx SO_TIMESTAMPING or not.

> Yes, it only sets some flags. I thought that those flags was
> required by the skb_tx_timestamp() in order to make the appropriate
> timestamping (hardware, software, etc).
> 
> So in order to have tx timestamp that work, both calls are needed ?

Yes.
 
> Why sock_tx_timestamp is called in packet_fill_skb and
> packet_sendmsg_spkt and not in tpacket_fill_skb ?
> Why i can retrieve timestamps when i add this call ?

Sorry, I don't know much about packet mmap. Last time I tried it, some
years ago, it wasn't really working.

Richard

Re: [RFC] net : add tx timestamp to packet mmap.

[Paul Chavent]

On 12/13/2012 07:17 PM, Richard Cochran wrote:
> On Thu, Dec 13, 2012 at 05:13:56PM +0100, Paul Chavent wrote:
>>>
>>> In order for time stamps to appear, somebody has to call
>>> skb_tx_timestamp() ...
>> Yes. "Somebody" means "the hardware driver" after completing xmit.
>> That's true ?
>
> Yes, the MAC driver must call this helper function, but not many
> drivers do this yet. You didn't say which MAC driver you are using and
> whether it supports Tx SO_TIMESTAMPING or not.
I'm using the uml net device (which recently gains tx timestamping), 
e1000e (wich seems to support it according to my tests), and arm macb 
(wich seems to support it too).


>
>> Yes, it only sets some flags. I thought that those flags was
>> required by the skb_tx_timestamp() in order to make the appropriate
>> timestamping (hardware, software, etc).
>>
>> So in order to have tx timestamp that work, both calls are needed ?
>
> Yes.
>
>> Why sock_tx_timestamp is called in packet_fill_skb and
>> packet_sendmsg_spkt and not in tpacket_fill_skb ?
>> Why i can retrieve timestamps when i add this call ?
>
> Sorry, I don't know much about packet mmap. Last time I tried it, some
> years ago, it wasn't really working.

I haven't measured the performance, but it works for me (however, not on 
my arm platfrom yet).

>
> Richard
>

The af_packet implementation contains 3 "paths" for packets. Perhaps i'm 
a bit confused by its complexity.

Paul.

Re: [RFC] net : add tx timestamp to packet mmap.

[Paul Chavent]

On 12/13/2012 07:17 PM, Richard Cochran wrote:
> On Thu, Dec 13, 2012 at 05:13:56PM +0100, Paul Chavent wrote:
>>>
>>> In order for time stamps to appear, somebody has to call
>>> skb_tx_timestamp() ...
>> Yes. "Somebody" means "the hardware driver" after completing xmit.
>> That's true ?
>
> Yes, the MAC driver must call this helper function, but not many
> drivers do this yet. You didn't say which MAC driver you are using and
> whether it supports Tx SO_TIMESTAMPING or not.
>
>> Yes, it only sets some flags. I thought that those flags was
>> required by the skb_tx_timestamp() in order to make the appropriate
>> timestamping (hardware, software, etc).
>>
>> So in order to have tx timestamp that work, both calls are needed ?
>
> Yes.
>
>> Why sock_tx_timestamp is called in packet_fill_skb and
>> packet_sendmsg_spkt and not in tpacket_fill_skb ?
>> Why i can retrieve timestamps when i add this call ?
>
> Sorry, I don't know much about packet mmap. Last time I tried it, some
> years ago, it wasn't really working.
>
> Richard
>
Hi.

Would it be possible that the packet mmap maintainers give their opinion 
on this thread please ?

Regards.

Paul.

Re: [RFC] net : add tx timestamp to packet mmap.

[Richard Cochran]

On Tue, Apr 09, 2013 at 12:42:57PM +0200, Paul Chavent wrote:
> 
> Would it be possible that the packet mmap maintainers give their
> opinion on this thread please ?

I was digging around, trying to understand whether libpcap can get HW
time stamps via the packet_mmap interface, and I found this.

 commit 614f60fa9d73a9e8fdff3df83381907fea7c5649
 Author: Scott McMillan <scott.a.mcmillan@intel.com>
 Date:   Wed Jun 2 05:53:56 2010 -0700

    packet_mmap: expose hw packet timestamps to network packet capture utilities

Maybe you could ask Scott for help?

[ It looks to me like that patch is kinda useless, since the user has
  no way to tell whether the time stamps are from HW or SW. ]

HTH,
Richard

Re: [RFC] net : add tx timestamp to packet mmap.

[Willem de Bruijn]

On Thu, Dec 13, 2012 at 11:13 AM, Paul Chavent <Paul.Chavent@onera.fr> wrote:
> Hello.
>
>
> On 12/13/2012 02:29 PM, Richard Cochran wrote:
>>
>> On Wed, Dec 12, 2012 at 04:29:25PM +0100, Paul Chavent wrote:
>>>
>>> This patch allow to generate tx timestamps of packets sent by the packet
>>> mmap interface.
>>>
>>> Actually, you can't get tx timestamps with the sample code below.
>>>
>>> I wonder if my current implementation is good. And if not, how should i
>>> get the timestamps ?
>>
>>
>> In order for time stamps to appear, somebody has to call
>> skb_tx_timestamp() ...
>
> Yes. "Somebody" means "the hardware driver" after completing xmit. That's
> true ?
>
>
>>
>>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>>> index e639645..948748b 100644
>>> --- a/net/packet/af_packet.c
>>> +++ b/net/packet/af_packet.c
>>> @@ -1857,6 +1857,10 @@ static int tpacket_fill_skb(struct packet_sock
>>> *po, struct sk_buff *skb,
>>>         void *data;
>>>         int err;
>>>
>>> +       err = sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
>
>>
>> and this call is only setting some flags.
>
> Yes, it only sets some flags. I thought that those flags was required by the
> skb_tx_timestamp() in order to make the appropriate timestamping (hardware,
> software, etc).
>
> So in order to have tx timestamp that work, both calls are needed ?

Yes.

> Why sock_tx_timestamp is called in packet_fill_skb and packet_sendmsg_spkt
> and not in tpacket_fill_skb ?

Good point. From what I can tell, if you add it, the existing error
queue mechanism should work fine. At a glance, your code looks fine,
too, although I haven't tested it.

> Why i can retrieve timestamps when i add this call ?

I actually wondered it if would be possible to return the timestamps
in the ring itself. It is, but requires two additional changes:
writing the timestamp to orig_skb->tstamp in skb_tstamp_tx and then
writing it into the ring in tpacket_destruct_skb. The first change is
a bit odd, as the orig_skb is usually freed shortly after
skb_tstamp_tx is called. Still, I verified that it works and the patch
should also make your errorqueue-based code work, as it inserts the
sock_tx_timestamp. Will send it for review following this.

>>
>> HTH,
>> Richard
>>
> Thank for your help.
>
> Paul.
>
> --
> 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

[PATCH] net-packet: tx timestamping on tpacket ring

[Willem de Bruijn]

When transmit timestamping is enabled at the socket level, have
writes to a PACKET_TX_RING record a timestamp for the generated
skbuffs. Tx timestamps are always looped to the application over
the socket error queue.

The patch also loops software timestamps back into the ring.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/core/skbuff.c      |  2 +-
 net/packet/af_packet.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ba64614..44b9c2a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3311,7 +3311,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
 		 * so keep the shared tx_flags and only
 		 * store software time stamp
 		 */
-		skb->tstamp = ktime_get_real();
+		orig_skb->tstamp = skb->tstamp = ktime_get_real();
 	}
 
 	serr = SKB_EXT_ERR(skb);
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 8e4644f..dc5d224 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -341,6 +341,45 @@ static int __packet_get_status(struct packet_sock *po, void *frame)
 	}
 }
 
+static void __packet_set_timestamp(struct packet_sock *po, void *frame,
+				   ktime_t tstamp)
+{
+	struct tpacket_hdr *h1;
+	struct tpacket2_hdr *h2;
+	struct timespec ts;
+
+	if (!tstamp.tv64 || !sock_flag(&po->sk, SOCK_TIMESTAMPING_SOFTWARE))
+		return;
+
+	ts = ktime_to_timespec(tstamp);
+
+	switch (po->tp_version) {
+	case TPACKET_V1:
+		h1 = frame;
+		h1->tp_sec = ts.tv_sec;
+		h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
+
+		flush_dcache_page(pgv_to_page(&h1->tp_sec));
+		flush_dcache_page(pgv_to_page(&h1->tp_usec));
+		break;
+	case TPACKET_V2:
+		h2 = frame;
+		h2->tp_sec = ts.tv_sec;
+		h2->tp_nsec = ts.tv_nsec;
+
+		flush_dcache_page(pgv_to_page(&h2->tp_sec));
+		flush_dcache_page(pgv_to_page(&h2->tp_nsec));
+		break;
+	case TPACKET_V3:
+	default:
+		WARN(1, "TPACKET version not supported.\n");
+		BUG();
+	}
+
+
+	smp_wmb();
+}
+
 static void *packet_lookup_frame(struct packet_sock *po,
 		struct packet_ring_buffer *rb,
 		unsigned int position,
@@ -1900,6 +1939,7 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
 		ph = skb_shinfo(skb)->destructor_arg;
 		BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
 		atomic_dec(&po->tx_ring.pending);
+		__packet_set_timestamp(po, ph, skb->tstamp);
 		__packet_set_status(po, ph, TP_STATUS_AVAILABLE);
 	}
 
@@ -2119,6 +2159,8 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 			}
 		}
 
+		sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
+
 		skb->destructor = tpacket_destruct_skb;
 		__packet_set_status(po, ph, TP_STATUS_SENDING);
 		atomic_inc(&po->tx_ring.pending);
-- 
1.8.1.3

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Daniel Borkmann]

On 04/13/2013 08:56 PM, Willem de Bruijn wrote:
> When transmit timestamping is enabled at the socket level, have
> writes to a PACKET_TX_RING record a timestamp for the generated
> skbuffs. Tx timestamps are always looped to the application over
> the socket error queue.

Nitpick: if so, then this should go to net-next (subject line).

> The patch also loops software timestamps back into the ring.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>   net/core/skbuff.c      |  2 +-
>   net/packet/af_packet.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 43 insertions(+), 1 deletion(-)
[...]
> +static void __packet_set_timestamp(struct packet_sock *po, void *frame,
> +				   ktime_t tstamp)
> +{
> +	struct tpacket_hdr *h1;
> +	struct tpacket2_hdr *h2;
> +	struct timespec ts;
> +
> +	if (!tstamp.tv64 || !sock_flag(&po->sk, SOCK_TIMESTAMPING_SOFTWARE))
> +		return;
> +
> +	ts = ktime_to_timespec(tstamp);
> +
> +	switch (po->tp_version) {
> +	case TPACKET_V1:
> +		h1 = frame;
> +		h1->tp_sec = ts.tv_sec;
> +		h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
> +
> +		flush_dcache_page(pgv_to_page(&h1->tp_sec));
> +		flush_dcache_page(pgv_to_page(&h1->tp_usec));

Hmm, not sure, but could we also flush the dcache only once?

> +		break;
> +	case TPACKET_V2:
> +		h2 = frame;
> +		h2->tp_sec = ts.tv_sec;
> +		h2->tp_nsec = ts.tv_nsec;
> +
> +		flush_dcache_page(pgv_to_page(&h2->tp_sec));
> +		flush_dcache_page(pgv_to_page(&h2->tp_nsec));
> +		break;
> +	case TPACKET_V3:
> +	default:
> +		WARN(1, "TPACKET version not supported.\n");
> +		BUG();
> +	}
> +
> +

Nitpick: one space too much.

> +	smp_wmb();
> +}
> +
>   static void *packet_lookup_frame(struct packet_sock *po,
>   		struct packet_ring_buffer *rb,
>   		unsigned int position,
> @@ -1900,6 +1939,7 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
>   		ph = skb_shinfo(skb)->destructor_arg;
>   		BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
>   		atomic_dec(&po->tx_ring.pending);
> +		__packet_set_timestamp(po, ph, skb->tstamp);
>   		__packet_set_status(po, ph, TP_STATUS_AVAILABLE);
>   	}
>
> @@ -2119,6 +2159,8 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>   			}
>   		}
>
> +		sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
> +

Hmm, so in case nobody wants to use timestamping on TX (which might be the majority
of people), we have to go through those 3 additional if statements in sock_tx_timestamp()
each time? Shouldn't we rather make the TX_RING faster? ;-)

>   		skb->destructor = tpacket_destruct_skb;
>   		__packet_set_status(po, ph, TP_STATUS_SENDING);
>   		atomic_inc(&po->tx_ring.pending);
>

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Sun, 14 Apr 2013 00:18:48 +0200

>> +		flush_dcache_page(pgv_to_page(&h1->tp_sec));
>> +		flush_dcache_page(pgv_to_page(&h1->tp_usec));
> 
> Hmm, not sure, but could we also flush the dcache only once?

Indeed, I truly hope that headers never straddle pages.

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Sat, Apr 13, 2013 at 6:47 PM, David Miller <davem@davemloft.net> wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Sun, 14 Apr 2013 00:18:48 +0200
>
>>> +            flush_dcache_page(pgv_to_page(&h1->tp_sec));
>>> +            flush_dcache_page(pgv_to_page(&h1->tp_usec));
>>
>> Hmm, not sure, but could we also flush the dcache only once?
>
> Indeed, I truly hope that headers never straddle pages.

I should have checked the alignment restrictions on frames. Frames
must be a multiple of 16 B as well as larger than the header (obviously),
so this can indeed never happen.

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Sat, Apr 13, 2013 at 8:04 PM, Willem de Bruijn <willemb@google.com> wrote:
> On Sat, Apr 13, 2013 at 6:47 PM, David Miller <davem@davemloft.net> wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>> Date: Sun, 14 Apr 2013 00:18:48 +0200
>>
>>>> +            flush_dcache_page(pgv_to_page(&h1->tp_sec));
>>>> +            flush_dcache_page(pgv_to_page(&h1->tp_usec));
>>>
>>> Hmm, not sure, but could we also flush the dcache only once?
>>
>> Indeed, I truly hope that headers never straddle pages.
>
> I should have checked the alignment restrictions on frames. Frames
> must be a multiple of 16 B as well as larger than the header (obviously),
> so this can indeed never happen.

Actually, 48 B is a multiple of 16, so should be accepted, and 85
frames on a page leaves half a frame for the next. I'll check whether
this is right. Even if so, it would still not matter for these time
offsets, as they start at 16 or 20 B offset from the start of the
frame.

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Sat, Apr 13, 2013 at 8:16 PM, Willem de Bruijn <willemb@google.com> wrote:
> On Sat, Apr 13, 2013 at 8:04 PM, Willem de Bruijn <willemb@google.com> wrote:
>> On Sat, Apr 13, 2013 at 6:47 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Daniel Borkmann <dborkman@redhat.com>
>>> Date: Sun, 14 Apr 2013 00:18:48 +0200
>>>
>>>>> +            flush_dcache_page(pgv_to_page(&h1->tp_sec));
>>>>> +            flush_dcache_page(pgv_to_page(&h1->tp_usec));
>>>>
>>>> Hmm, not sure, but could we also flush the dcache only once?
>>>
>>> Indeed, I truly hope that headers never straddle pages.
>>
>> I should have checked the alignment restrictions on frames. Frames
>> must be a multiple of 16 B as well as larger than the header (obviously),
>> so this can indeed never happen.
>
> Actually, 48 B is a multiple of 16, so should be accepted, and 85
> frames on a page leaves half a frame for the next. I'll check whether
> this is right. Even if so, it would still not matter for these time
> offsets, as they start at 16 or 20 B offset from the start of the
> frame.

48 B is too small (Because less than TPACKET_HDRLEN), but it can
be triggered with 80 B frames. Daniel, thanks for submitting the
selftest: both the timestamp and this alignment question were now very
easy to test by just changing a few lines in your code.

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Daniel Borkmann]

On 04/14/2013 02:49 AM, Willem de Bruijn wrote:
> On Sat, Apr 13, 2013 at 8:16 PM, Willem de Bruijn <willemb@google.com> wrote:
>> On Sat, Apr 13, 2013 at 8:04 PM, Willem de Bruijn <willemb@google.com> wrote:
>>> On Sat, Apr 13, 2013 at 6:47 PM, David Miller <davem@davemloft.net> wrote:
>>>> From: Daniel Borkmann <dborkman@redhat.com>
>>>> Date: Sun, 14 Apr 2013 00:18:48 +0200
>>>>
>>>>>> +            flush_dcache_page(pgv_to_page(&h1->tp_sec));
>>>>>> +            flush_dcache_page(pgv_to_page(&h1->tp_usec));
>>>>>
>>>>> Hmm, not sure, but could we also flush the dcache only once?
>>>>
>>>> Indeed, I truly hope that headers never straddle pages.
>>>
>>> I should have checked the alignment restrictions on frames. Frames
>>> must be a multiple of 16 B as well as larger than the header (obviously),
>>> so this can indeed never happen.
>>
>> Actually, 48 B is a multiple of 16, so should be accepted, and 85
>> frames on a page leaves half a frame for the next. I'll check whether
>> this is right. Even if so, it would still not matter for these time
>> offsets, as they start at 16 or 20 B offset from the start of the
>> frame.
>
> 48 B is too small (Because less than TPACKET_HDRLEN), but it can
> be triggered with 80 B frames. Daniel, thanks for submitting the
> selftest: both the timestamp and this alignment question were now very
> easy to test by just changing a few lines in your code.

Feel free to further extend it. :-)

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Sat, Apr 13, 2013 at 6:18 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 04/13/2013 08:56 PM, Willem de Bruijn wrote:
>>
>> When transmit timestamping is enabled at the socket level, have
>> writes to a PACKET_TX_RING record a timestamp for the generated
>> skbuffs. Tx timestamps are always looped to the application over
>> the socket error queue.
>
>
> Nitpick: if so, then this should go to net-next (subject line).

Thanks.
>
>> The patch also loops software timestamps back into the ring.
>>
>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>> ---
>>   net/core/skbuff.c      |  2 +-
>>   net/packet/af_packet.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 43 insertions(+), 1 deletion(-)
>
> [...]
>
>> +static void __packet_set_timestamp(struct packet_sock *po, void *frame,
>> +                                  ktime_t tstamp)
>> +{
>> +       struct tpacket_hdr *h1;
>> +       struct tpacket2_hdr *h2;
>> +       struct timespec ts;
>> +
>> +       if (!tstamp.tv64 || !sock_flag(&po->sk,
>> SOCK_TIMESTAMPING_SOFTWARE))
>> +               return;
>> +
>> +       ts = ktime_to_timespec(tstamp);
>> +
>> +       switch (po->tp_version) {
>> +       case TPACKET_V1:
>> +               h1 = frame;
>> +               h1->tp_sec = ts.tv_sec;
>> +               h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
>> +
>> +               flush_dcache_page(pgv_to_page(&h1->tp_sec));
>> +               flush_dcache_page(pgv_to_page(&h1->tp_usec));
>
>
> Hmm, not sure, but could we also flush the dcache only once?
>
>
>> +               break;
>> +       case TPACKET_V2:
>> +               h2 = frame;
>> +               h2->tp_sec = ts.tv_sec;
>> +               h2->tp_nsec = ts.tv_nsec;
>> +
>> +               flush_dcache_page(pgv_to_page(&h2->tp_sec));
>> +               flush_dcache_page(pgv_to_page(&h2->tp_nsec));
>> +               break;
>> +       case TPACKET_V3:
>> +       default:
>> +               WARN(1, "TPACKET version not supported.\n");
>> +               BUG();
>> +       }
>> +
>> +
>
>
> Nitpick: one space too much.
>
>
>> +       smp_wmb();
>> +}
>> +
>>   static void *packet_lookup_frame(struct packet_sock *po,
>>                 struct packet_ring_buffer *rb,
>>                 unsigned int position,
>> @@ -1900,6 +1939,7 @@ static void tpacket_destruct_skb(struct sk_buff
>> *skb)
>>                 ph = skb_shinfo(skb)->destructor_arg;
>>                 BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
>>                 atomic_dec(&po->tx_ring.pending);
>> +               __packet_set_timestamp(po, ph, skb->tstamp);
>>                 __packet_set_status(po, ph, TP_STATUS_AVAILABLE);
>>         }
>>
>> @@ -2119,6 +2159,8 @@ static int tpacket_snd(struct packet_sock *po,
>> struct msghdr *msg)
>>                         }
>>                 }
>>
>> +               sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
>> +
>
>
> Hmm, so in case nobody wants to use timestamping on TX (which might be the
> majority
> of people), we have to go through those 3 additional if statements in
> sock_tx_timestamp()
> each time? Shouldn't we rather make the TX_RING faster? ;-)

A static key, similar to netstamp_needed, might make it cheaper, but
may be more complexity than warranted for a corner case. Simpler is
testing only the software timestamp, and in tpacket_fill_skb, where
the sk struct is already accessed. I don't feel strongly about merging
given the performance trade-off: just wanted to see if it worked.
Happy to resubmit either revision (or a better idea).

>
>>                 skb->destructor = tpacket_destruct_skb;
>>                 __packet_set_status(po, ph, TP_STATUS_SENDING);
>>                 atomic_inc(&po->tx_ring.pending);
>>
>

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Daniel Borkmann]

On 04/14/2013 02:00 AM, Willem de Bruijn wrote:
> On Sat, Apr 13, 2013 at 6:18 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
>> On 04/13/2013 08:56 PM, Willem de Bruijn wrote:
>>>
>>> When transmit timestamping is enabled at the socket level, have
>>> writes to a PACKET_TX_RING record a timestamp for the generated
>>> skbuffs. Tx timestamps are always looped to the application over
>>> the socket error queue.
>>
>>
>> Nitpick: if so, then this should go to net-next (subject line).
>
> Thanks.
>>
>>> The patch also loops software timestamps back into the ring.

Also v2 should probably contain a ``Reported-by''.

>>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>>> ---
>>>    net/core/skbuff.c      |  2 +-
>>>    net/packet/af_packet.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 43 insertions(+), 1 deletion(-)
>>
>> [...]
>>
>>> +static void __packet_set_timestamp(struct packet_sock *po, void *frame,
>>> +                                  ktime_t tstamp)
>>> +{
>>> +       struct tpacket_hdr *h1;
>>> +       struct tpacket2_hdr *h2;
>>> +       struct timespec ts;
>>> +
>>> +       if (!tstamp.tv64 || !sock_flag(&po->sk,
>>> SOCK_TIMESTAMPING_SOFTWARE))
>>> +               return;

While going a bit more through the code, I'm wondering .. if we want to support
TX timestamps, could we also support SW _and_ HW timestamps e.g. similar as in
sock_recv_timestamp()? I'm asking, because we already allow setting the flags
for it via sock_tx_timestamp(). This might be good, if possible.

Paul, since you've reported / requested this, your use case would be to fill
the ring, trigger a sendto() and then loop through all the frames to check the
tx timestamps for your custom protocol mockup, then fill the returned frames
again, etc.?

>>> +       ts = ktime_to_timespec(tstamp);
>>> +
>>> +       switch (po->tp_version) {
>>> +       case TPACKET_V1:
>>> +               h1 = frame;
>>> +               h1->tp_sec = ts.tv_sec;
>>> +               h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
>>> +
>>> +               flush_dcache_page(pgv_to_page(&h1->tp_sec));
>>> +               flush_dcache_page(pgv_to_page(&h1->tp_usec));
>>
>>
>> Hmm, not sure, but could we also flush the dcache only once?

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Richard Cochran]

On Sun, Apr 14, 2013 at 12:52:16PM +0200, Daniel Borkmann wrote:
> 
> While going a bit more through the code, I'm wondering .. if we want to support
> TX timestamps, could we also support SW _and_ HW timestamps e.g. similar as in
> sock_recv_timestamp()? I'm asking, because we already allow setting the flags
> for it via sock_tx_timestamp(). This might be good, if possible.

And while you are at it, you could also fix the receive code.

As it stand now, it is fairly useless, since there is no way for user
space to tell which kind of time stamp has been reported. In fact, the
code will silently intermingle hardware and software time stamps. That
is surely a mean trick to play on the users.

Thanks,
Richard

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Paul Chavent]

On 04/14/2013 03:07 PM, Richard Cochran wrote:
> On Sun, Apr 14, 2013 at 12:52:16PM +0200, Daniel Borkmann wrote:
>>
>> While going a bit more through the code, I'm wondering .. if we want to support
>> TX timestamps, could we also support SW _and_ HW timestamps e.g. similar as in
>> sock_recv_timestamp()? I'm asking, because we already allow setting the flags
>> for it via sock_tx_timestamp(). This might be good, if possible.
>
> And while you are at it, you could also fix the receive code.
>
> As it stand now, it is fairly useless, since there is no way for user
> space to tell which kind of time stamp has been reported. In fact, the
> code will silently intermingle hardware and software time stamps. That
> is surely a mean trick to play on the users.

Isn't it the one that the user ask with setsockopt(fd, SOL_PACKET, 
PACKET_TIMESTAMP, &timestamping, sizeof(timestamping)) ?

However, i wonder why you added an other sockopt that do the same thing 
as SOL_SOCKET/SO_TIMESTAMPING sockopt ?

Paul.

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Richard Cochran]

On Mon, Apr 15, 2013 at 09:37:30AM +0200, Paul Chavent wrote:
> On 04/14/2013 03:07 PM, Richard Cochran wrote:
> >As it stand now, it is fairly useless, since there is no way for user
> >space to tell which kind of time stamp has been reported. In fact, the
> >code will silently intermingle hardware and software time stamps. That
> >is surely a mean trick to play on the users.
> 
> Isn't it the one that the user ask with setsockopt(fd, SOL_PACKET,
> PACKET_TIMESTAMP, &timestamping, sizeof(timestamping)) ?

No, not necessarily. Look at the code in net/packet/af_packet.c.

		if ((po->tp_tstamp & SOF_TIMESTAMPING_SYS_HARDWARE)
				&& shhwtstamps->syststamp.tv64)
			ts = ktime_to_timespec(shhwtstamps->syststamp);
		else if ((po->tp_tstamp & SOF_TIMESTAMPING_RAW_HARDWARE)
				&& shhwtstamps->hwtstamp.tv64)
			ts = ktime_to_timespec(shhwtstamps->hwtstamp);
		else if (skb->tstamp.tv64)
			ts = ktime_to_timespec(skb->tstamp);
		else
			getnstimeofday(&ts);

What happens if RAW is requested, but no HW time stamp is available?

 
> However, i wonder why you added an other sockopt that do the same
> thing as SOL_SOCKET/SO_TIMESTAMPING sockopt ?

Not sure what you mean. I did not add the SOL_PACKET socket option.

Thanks,
Richard

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Mon, Apr 15, 2013 at 3:37 AM, Paul Chavent <Paul.Chavent@onera.fr> wrote:
>
>
> On 04/14/2013 03:07 PM, Richard Cochran wrote:
>>
>> On Sun, Apr 14, 2013 at 12:52:16PM +0200, Daniel Borkmann wrote:
>>>
>>>
>>> While going a bit more through the code, I'm wondering .. if we want to
>>> support
>>> TX timestamps, could we also support SW _and_ HW timestamps e.g. similar
>>> as in
>>> sock_recv_timestamp()? I'm asking, because we already allow setting the
>>> flags
>>> for it via sock_tx_timestamp(). This might be good, if possible.
>>
>>
>> And while you are at it, you could also fix the receive code.
>>
>> As it stand now, it is fairly useless, since there is no way for user
>> space to tell which kind of time stamp has been reported. In fact, the
>> code will silently intermingle hardware and software time stamps. That
>> is surely a mean trick to play on the users.

Interesting issue, but I'll leave that for a separate fix.
>
> Isn't it the one that the user ask with setsockopt(fd, SOL_PACKET,
> PACKET_TIMESTAMP, &timestamping, sizeof(timestamping)) ?

This option toggles whether, if an skbuff has a timestamp, it should
be written to the rx ring metadata. skbuffs can have multiple
timestamps, so it also determines which one is selected.

If I understand correctly, the problem that Richard refers to is that
when one socket requests rx timestamping, all rx skbuffs have to be
timestamped as it is not known early in the datapath to which socket
they will be enqueued. As a result, the timestamps in skbuffs depend
on the preferences of other active sockets. Correct me if I'm wrong.

If so, it may even vary during the lifetime of a packet socket, so a
getsockopt wouldn't help, either. Perhaps nothing short of a new field
in the frame header structure will. In which case it makes more sense
to support sending each type of timestamp independently, just like the
socker error queue mechanism. Adding yet another header format,
though?

> However, i wonder why you added an other sockopt that do the same thing as
> SOL_SOCKET/SO_TIMESTAMPING sockopt ?

Back to the patch. I'll revise with

- a small patch for the errqueue mechanism
  - add hw timestamp pass-through in skb_tstamp_tx, if correct
  - move sock_tx_timestamp where cache is warm
  - update it to use only a single conditional in the common case: no
flags enabled

- a follow-up for the ring
  - single flush_dcache_page, which is safe for current header layouts
on 32bit+ platforms
    (it is also a noop on many of these)

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Richard Cochran]

On Mon, Apr 15, 2013 at 12:59:26PM -0400, Willem de Bruijn wrote:
> 
> If I understand correctly, the problem that Richard refers to is that
> when one socket requests rx timestamping, all rx skbuffs have to be
> timestamped as it is not known early in the datapath to which socket
> they will be enqueued. As a result, the timestamps in skbuffs depend
> on the preferences of other active sockets. Correct me if I'm wrong.

No, that is not it.

Hardware time stamping needs to be globally enabled at the driver
level using the SIOCSHWTSTAMP [1] ioctl. The hardware has various
different abilities, usually one of:

- no time stamping
- some subset of PTP packets
- all packets

So depending on the mode, not all packets will receive a time
stamp. In addition, some of the hardware can only time stamp one
packet at a time. Furthermore, time stamps can get dropped in high
traffic situations.

The code in af_packet.c unconditionally delivers some sort of time
stamp, with an inexplicable fall back to gettimeofday. Thus, the user
space has no way of knowing what is being reported in the time stamp.

Thanks,
Richard

1. See Documentation/networking/timestamping.txt

[PATCH net-next v2] packet: tx timestamping on tpacket ring

[Willem de Bruijn]

v1->v2:
- move sock_tx_timestamp near other sk reads (warm cacheline)
- remove duplicate flush_dcache_page
- enable hardware timestamps reporting using the error queue (not ring)
- use new ktime_to_timespec_cond API

When transmit timestamping is enabled at the socket level, record a
timestamp on packets written to a PACKET_TX_RING. Tx timestamps are
always looped to the application over the socket error queue. Software
timestamps are also written back into the packet frame header in the
packet ring.

Reported-by: Paul Chavent <paul.chavent@onera.fr>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/core/skbuff.c      | 12 ++++++------
 net/packet/af_packet.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 898cf5c..af9185d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3327,12 +3327,8 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
 	if (!sk)
 		return;
 
-	skb = skb_clone(orig_skb, GFP_ATOMIC);
-	if (!skb)
-		return;
-
 	if (hwtstamps) {
-		*skb_hwtstamps(skb) =
+		*skb_hwtstamps(orig_skb) =
 			*hwtstamps;
 	} else {
 		/*
@@ -3340,9 +3336,13 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
 		 * so keep the shared tx_flags and only
 		 * store software time stamp
 		 */
-		skb->tstamp = ktime_get_real();
+		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;
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 7e387ff..ec8ea27 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -339,6 +339,37 @@ static int __packet_get_status(struct packet_sock *po, void *frame)
 	}
 }
 
+static void __packet_set_timestamp(struct packet_sock *po, void *frame,
+				   ktime_t tstamp)
+{
+	union tpacket_uhdr h;
+	struct timespec ts;
+
+	if (!ktime_to_timespec_cond(tstamp, &ts) ||
+	    !sock_flag(&po->sk, SOCK_TIMESTAMPING_SOFTWARE))
+		return;
+
+	h.raw = frame;
+	switch (po->tp_version) {
+	case TPACKET_V1:
+		h.h1->tp_sec = ts.tv_sec;
+		h.h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
+		break;
+	case TPACKET_V2:
+		h.h2->tp_sec = ts.tv_sec;
+		h.h2->tp_nsec = ts.tv_nsec;
+		break;
+	case TPACKET_V3:
+	default:
+		WARN(1, "TPACKET version not supported.\n");
+		BUG();
+	}
+
+	/* one flush is safe, as both fields always lie on the same cacheline */
+	flush_dcache_page(pgv_to_page(&h.h1->tp_sec));
+	smp_wmb();
+}
+
 static void *packet_lookup_frame(struct packet_sock *po,
 		struct packet_ring_buffer *rb,
 		unsigned int position,
@@ -1877,6 +1908,7 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
 		ph = skb_shinfo(skb)->destructor_arg;
 		BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
 		atomic_dec(&po->tx_ring.pending);
+		__packet_set_timestamp(po, ph, skb->tstamp);
 		__packet_set_status(po, ph, TP_STATUS_AVAILABLE);
 	}
 
@@ -1900,6 +1932,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 	skb->dev = dev;
 	skb->priority = po->sk.sk_priority;
 	skb->mark = po->sk.sk_mark;
+	sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
 	skb_shinfo(skb)->destructor_arg = ph.raw;
 
 	switch (po->tp_version) {
-- 
1.8.2.1

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Daniel Borkmann]

On 04/19/2013 11:51 PM, Willem de Bruijn wrote:
> v1->v2:
> - move sock_tx_timestamp near other sk reads (warm cacheline)
> - remove duplicate flush_dcache_page
> - enable hardware timestamps reporting using the error queue (not ring)
> - use new ktime_to_timespec_cond API

Nitpick: probably this should rather go below the "---" line, so that this
will not show up in the commit message.

> When transmit timestamping is enabled at the socket level, record a
> timestamp on packets written to a PACKET_TX_RING. Tx timestamps are
> always looped to the application over the socket error queue. Software
> timestamps are also written back into the packet frame header in the
> packet ring.
>
> Reported-by: Paul Chavent <paul.chavent@onera.fr>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>   net/core/skbuff.c      | 12 ++++++------
>   net/packet/af_packet.c | 33 +++++++++++++++++++++++++++++++++
>   2 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 898cf5c..af9185d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3327,12 +3327,8 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>   	if (!sk)
>   		return;
>
> -	skb = skb_clone(orig_skb, GFP_ATOMIC);
> -	if (!skb)
> -		return;
> -
>   	if (hwtstamps) {
> -		*skb_hwtstamps(skb) =
> +		*skb_hwtstamps(orig_skb) =
>   			*hwtstamps;
>   	} else {
>   		/*
> @@ -3340,9 +3336,13 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>   		 * so keep the shared tx_flags and only
>   		 * store software time stamp
>   		 */
> -		skb->tstamp = ktime_get_real();
> +		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;
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7e387ff..ec8ea27 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -339,6 +339,37 @@ static int __packet_get_status(struct packet_sock *po, void *frame)
>   	}
>   }
>
> +static void __packet_set_timestamp(struct packet_sock *po, void *frame,
> +				   ktime_t tstamp)
> +{
> +	union tpacket_uhdr h;
> +	struct timespec ts;
> +
> +	if (!ktime_to_timespec_cond(tstamp, &ts) ||
> +	    !sock_flag(&po->sk, SOCK_TIMESTAMPING_SOFTWARE))
> +		return;

If we currently have the po->tp_tstamp member that was introduced for the RX_RING
part only, using it if possible for the TX_RING part as well would be good, at
least cleaner. Also the documentation [1] should receive a status update on this
feature, otherwise only Paul will use this feature and nobody else.

Not an expert in timestamping, but why can't we also allow hw timestamps but have
to use sw only? Would it be possible to reuse the tpacket_get_timestamp() function
that got recently introduced for this (while keeping sock_tx_timestamp() below on
TX path)?

Thanks.

  [1] Documentation/networking/packet_mmap.txt

> +	h.raw = frame;
> +	switch (po->tp_version) {
> +	case TPACKET_V1:
> +		h.h1->tp_sec = ts.tv_sec;
> +		h.h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
> +		break;
> +	case TPACKET_V2:
> +		h.h2->tp_sec = ts.tv_sec;
> +		h.h2->tp_nsec = ts.tv_nsec;
> +		break;
> +	case TPACKET_V3:
> +	default:
> +		WARN(1, "TPACKET version not supported.\n");
> +		BUG();
> +	}
> +
> +	/* one flush is safe, as both fields always lie on the same cacheline */
> +	flush_dcache_page(pgv_to_page(&h.h1->tp_sec));
> +	smp_wmb();
> +}
> +
>   static void *packet_lookup_frame(struct packet_sock *po,
>   		struct packet_ring_buffer *rb,
>   		unsigned int position,
> @@ -1877,6 +1908,7 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
>   		ph = skb_shinfo(skb)->destructor_arg;
>   		BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
>   		atomic_dec(&po->tx_ring.pending);
> +		__packet_set_timestamp(po, ph, skb->tstamp);
>   		__packet_set_status(po, ph, TP_STATUS_AVAILABLE);
>   	}
>
> @@ -1900,6 +1932,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
>   	skb->dev = dev;
>   	skb->priority = po->sk.sk_priority;
>   	skb->mark = po->sk.sk_mark;
> +	sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
>   	skb_shinfo(skb)->destructor_arg = ph.raw;
>
>   	switch (po->tp_version) {
>

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Sat, Apr 20, 2013 at 8:33 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 04/19/2013 11:51 PM, Willem de Bruijn wrote:
>>
>> v1->v2:
>> - move sock_tx_timestamp near other sk reads (warm cacheline)
>> - remove duplicate flush_dcache_page
>> - enable hardware timestamps reporting using the error queue (not ring)
>> - use new ktime_to_timespec_cond API
>
>
> Nitpick: probably this should rather go below the "---" line, so that this
> will not show up in the commit message.
>
>
>> When transmit timestamping is enabled at the socket level, record a
>> timestamp on packets written to a PACKET_TX_RING. Tx timestamps are
>> always looped to the application over the socket error queue. Software
>> timestamps are also written back into the packet frame header in the
>> packet ring.
>>
>> Reported-by: Paul Chavent <paul.chavent@onera.fr>
>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>> ---
>>   net/core/skbuff.c      | 12 ++++++------
>>   net/packet/af_packet.c | 33 +++++++++++++++++++++++++++++++++
>>   2 files changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 898cf5c..af9185d 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3327,12 +3327,8 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>>         if (!sk)
>>                 return;
>>
>> -       skb = skb_clone(orig_skb, GFP_ATOMIC);
>> -       if (!skb)
>> -               return;
>> -
>>         if (hwtstamps) {
>> -               *skb_hwtstamps(skb) =
>> +               *skb_hwtstamps(orig_skb) =
>>                         *hwtstamps;
>>         } else {
>>                 /*
>> @@ -3340,9 +3336,13 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>>                  * so keep the shared tx_flags and only
>>                  * store software time stamp
>>                  */
>> -               skb->tstamp = ktime_get_real();
>> +               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;
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index 7e387ff..ec8ea27 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -339,6 +339,37 @@ static int __packet_get_status(struct packet_sock
>> *po, void *frame)
>>         }
>>   }
>>
>> +static void __packet_set_timestamp(struct packet_sock *po, void *frame,
>> +                                  ktime_t tstamp)
>> +{
>> +       union tpacket_uhdr h;
>> +       struct timespec ts;
>> +
>> +       if (!ktime_to_timespec_cond(tstamp, &ts) ||
>> +           !sock_flag(&po->sk, SOCK_TIMESTAMPING_SOFTWARE))
>> +               return;
>
>
> If we currently have the po->tp_tstamp member that was introduced for the
> RX_RING
> part only, using it if possible for the TX_RING part as well would be good,
> at
> least cleaner.

Then the caller has to set both the SOL_SOCKET and SOL_PACKET
options. I'm not convinced that that is an improvement over using generic
socket tx timestamping option as is. How would you use it specifically?
To determine whether or not to write the values into the ring, or more
extensively?

> Also the documentation [1] should receive a status update on
> this
> feature, otherwise only Paul will use this feature and nobody else.

Okay. I'll update the documentation. Speaking of which, it would be
great if someone could proofread the packet socket manpage update
patch (http://patchwork.ozlabs.org/patch/228709/)

> Not an expert in timestamping, but why can't we also allow hw timestamps but
> have
> to use sw only?

To avoid getting into the same situation on tx that Richard pointed out
about rx: the ring can only communicate one timestamp and cannot
communicate which one it is. Better to be consistent in that case, I
think. I'm open to discussing this, though.

> Would it be possible to reuse the tpacket_get_timestamp()
> function
> that got recently introduced for this (while keeping sock_tx_timestamp()
> below on
> TX path)?
>
> Thanks.
>
>  [1] Documentation/networking/packet_mmap.txt
>
>
>> +       h.raw = frame;
>> +       switch (po->tp_version) {
>> +       case TPACKET_V1:
>> +               h.h1->tp_sec = ts.tv_sec;
>> +               h.h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
>> +               break;
>> +       case TPACKET_V2:
>> +               h.h2->tp_sec = ts.tv_sec;
>> +               h.h2->tp_nsec = ts.tv_nsec;
>> +               break;
>> +       case TPACKET_V3:
>> +       default:
>> +               WARN(1, "TPACKET version not supported.\n");
>> +               BUG();
>> +       }
>> +
>> +       /* one flush is safe, as both fields always lie on the same
>> cacheline */
>> +       flush_dcache_page(pgv_to_page(&h.h1->tp_sec));
>> +       smp_wmb();
>> +}
>> +
>>   static void *packet_lookup_frame(struct packet_sock *po,
>>                 struct packet_ring_buffer *rb,
>>                 unsigned int position,
>> @@ -1877,6 +1908,7 @@ static void tpacket_destruct_skb(struct sk_buff
>> *skb)
>>                 ph = skb_shinfo(skb)->destructor_arg;
>>                 BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
>>                 atomic_dec(&po->tx_ring.pending);
>> +               __packet_set_timestamp(po, ph, skb->tstamp);
>>                 __packet_set_status(po, ph, TP_STATUS_AVAILABLE);
>>         }
>>
>> @@ -1900,6 +1932,7 @@ static int tpacket_fill_skb(struct packet_sock *po,
>> struct sk_buff *skb,
>>         skb->dev = dev;
>>         skb->priority = po->sk.sk_priority;
>>         skb->mark = po->sk.sk_mark;
>> +       sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
>>         skb_shinfo(skb)->destructor_arg = ph.raw;
>>
>>         switch (po->tp_version) {
>>
>

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Daniel Borkmann]

On 04/21/2013 04:30 AM, Willem de Bruijn wrote:
> On Sat, Apr 20, 2013 at 8:33 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
[..]
>> On 04/19/2013 11:51 PM, Willem de Bruijn wrote:
>> If we currently have the po->tp_tstamp member that was introduced for the
>> RX_RING
>> part only, using it if possible for the TX_RING part as well would be good,
>> at
>> least cleaner.
>
> Then the caller has to set both the SOL_SOCKET and SOL_PACKET
> options. I'm not convinced that that is an improvement over using generic
> socket tx timestamping option as is. How would you use it specifically?
> To determine whether or not to write the values into the ring, or more
> extensively?

I would use it in combination with tpacket_get_timestamp(). There, we do not
check for SOCK_TIMESTAMPING_SOFTWARE, but just fall back for skb->tstamp if
not null. The tpacket_get_timestamp() would need a small change though, in
that sense that i) getnstimeofday() is moved out of the function and ii) the
function will return a bool, if the ts variable was filled. Thus, in the RX
path if the function returns false, you do getnstimeofday() as fallback, and
in the TX path nothing needs to be done in __packet_set_timestamp() if the skb
was not timestamped.

That said, for the software timestamps, the caller would not need to do more
as with your current patch, for the hardware part, it's then the same afaik
as in the RX_RING.

Just to give some pseudo code (instead of the bool, we could also use an enum
that tells which timestamp was found, but more on that further below):

static bool tpacket_get_timestamp(struct sk_buff *skb, struct timespec *ts,
				  unsigned int flags)
{
	struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);

	if (shhwtstamps) {
		if ((flags & SOF_TIMESTAMPING_SYS_HARDWARE) &&
		    ktime_to_timespec_cond(shhwtstamps->syststamp, ts))
			return true;
		if ((flags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
		    ktime_to_timespec_cond(shhwtstamps->hwtstamp, ts))
			return true;
	}

	if (ktime_to_timespec_cond(skb->tstamp, ts))
		return true;

	return false;
}

In tpacket_rcv():
...
	if (!tpacket_get_timestamp(skb, &ts, po->tp_tstamp))
		getnstimeofday(ts);
...

In __packet_set_timestamp():
...
	if (!tpacket_get_timestamp(skb, &ts, po->tp_tstamp))
		return;
...

>> Also the documentation [1] should receive a status update on
>> this
>> feature, otherwise only Paul will use this feature and nobody else.
>
> Okay. I'll update the documentation. Speaking of which, it would be
> great if someone could proofread the packet socket manpage update
> patch (http://patchwork.ozlabs.org/patch/228709/)

Ok, I will have a look.

>> Not an expert in timestamping, but why can't we also allow hw timestamps but
>> have
>> to use sw only?
>
> To avoid getting into the same situation on tx that Richard pointed out
> about rx: the ring can only communicate one timestamp and cannot
> communicate which one it is. Better to be consistent in that case, I
> think. I'm open to discussing this, though.

Ok, I agree that it would be very useful to also state what timestamp is
being reported. Ideally, without introducing yet another tpacket version.

The only thing I can currently think of could be to use the tp_status bit
for that, then used by both RX_RING and TX_RING, e.g. ...

	TP_STATUS_TS_SYS_HARDWARE	(1 << 30)
	TP_STATUS_TS_RAW_HARDWARE	(1 << 31)

... although it would be a bit of waste to use 2 bits for that. If none of
them is set, it indicates that the timestamp is in software-only. This we
could do, if you don't have a better idea.

``Better to be consistent in that case'' would not be the current situation
with the patch, right? I mean, for RX_RING, we report one of those 3, for
TX_RING only the sw timestamp. So on one hand we might be consistent, but on
the other hand we're not. ;-) With the above proposal, we could tackle this
and stay consistent then. If you want, I could do that after your revised
patch gets accepted (with hw ts support). Let me know what you think of that.

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Sun, Apr 21, 2013 at 6:10 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 04/21/2013 04:30 AM, Willem de Bruijn wrote:
>>
>> On Sat, Apr 20, 2013 at 8:33 AM, Daniel Borkmann <dborkman@redhat.com>
>> wrote:
>
> [..]
>>>
>>> On 04/19/2013 11:51 PM, Willem de Bruijn wrote:
>>> If we currently have the po->tp_tstamp member that was introduced for the
>>> RX_RING
>>> part only, using it if possible for the TX_RING part as well would be
>>> good,
>>> at
>>> least cleaner.
>>
>>
>> Then the caller has to set both the SOL_SOCKET and SOL_PACKET
>> options. I'm not convinced that that is an improvement over using generic
>> socket tx timestamping option as is. How would you use it specifically?
>> To determine whether or not to write the values into the ring, or more
>> extensively?
>
>
> I would use it in combination with tpacket_get_timestamp(). There, we do not
> check for SOCK_TIMESTAMPING_SOFTWARE, but just fall back for skb->tstamp if
> not null. The tpacket_get_timestamp() would need a small change though, in
> that sense that i) getnstimeofday() is moved out of the function and ii) the
> function will return a bool, if the ts variable was filled. Thus, in the RX
> path if the function returns false, you do getnstimeofday() as fallback, and
> in the TX path nothing needs to be done in __packet_set_timestamp() if the
> skb
> was not timestamped.
>
> That said, for the software timestamps, the caller would not need to do more
> as with your current patch, for the hardware part, it's then the same afaik
> as in the RX_RING.
>
> Just to give some pseudo code (instead of the bool, we could also use an
> enum
> that tells which timestamp was found, but more on that further below):
>
> static bool tpacket_get_timestamp(struct sk_buff *skb, struct timespec *ts,
>                                   unsigned int flags)
> {
>         struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
>
>         if (shhwtstamps) {
>                 if ((flags & SOF_TIMESTAMPING_SYS_HARDWARE) &&
>                     ktime_to_timespec_cond(shhwtstamps->syststamp, ts))
>                         return true;
>                 if ((flags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>                     ktime_to_timespec_cond(shhwtstamps->hwtstamp, ts))
>                         return true;
>         }
>
>         if (ktime_to_timespec_cond(skb->tstamp, ts))
>                 return true;
>
>         return false;
> }
>
> In tpacket_rcv():
> ...
>         if (!tpacket_get_timestamp(skb, &ts, po->tp_tstamp))
>                 getnstimeofday(ts);
> ...
>
> In __packet_set_timestamp():
> ...
>         if (!tpacket_get_timestamp(skb, &ts, po->tp_tstamp))
>                 return;
> ...
>
>
>>> Also the documentation [1] should receive a status update on
>>> this
>>> feature, otherwise only Paul will use this feature and nobody else.
>>
>>
>> Okay. I'll update the documentation. Speaking of which, it would be
>> great if someone could proofread the packet socket manpage update
>> patch (http://patchwork.ozlabs.org/patch/228709/)
>
>
> Ok, I will have a look.

Saw your ack. Thanks, Daniel!
>
>>> Not an expert in timestamping, but why can't we also allow hw timestamps
>>> but
>>> have
>>> to use sw only?
>>
>>
>> To avoid getting into the same situation on tx that Richard pointed out
>> about rx: the ring can only communicate one timestamp and cannot
>> communicate which one it is. Better to be consistent in that case, I
>> think. I'm open to discussing this, though.
>
>
> Ok, I agree that it would be very useful to also state what timestamp is
> being reported. Ideally, without introducing yet another tpacket version.
>
> The only thing I can currently think of could be to use the tp_status bit
> for that, then used by both RX_RING and TX_RING, e.g. ...
>
>         TP_STATUS_TS_SYS_HARDWARE       (1 << 30)
>         TP_STATUS_TS_RAW_HARDWARE       (1 << 31)
>
> ... although it would be a bit of waste to use 2 bits for that. If none of
> them is set, it indicates that the timestamp is in software-only. This we
> could do, if you don't have a better idea.

I like that a lot. Visibility would certainly improve the state on
rx-ring side. One issue with interleaving various kinds of timestamps,
even when clearly labeled in tp_status, is that applications likely
cannot use timestamp series from different sources, as these sequences
are incomparable. Tx has the advantage that time sources can be chosen
per socket independent of all other sockets. The only time when
multiple sources come into play is when a socket has hw timestamps
selected, but the hardware failed to create a tstamp for some
device-specific reason. In that case, generating the sw timestamp may
or may not be helpful to the application. When at least it is clearly
labeled in tp_status, worst case the application will simply ignore
it. If the application only selects software timestamps, the mechanism
works just like what I proposed: always communicate the software
timestamp and do not modify tp_status. So, I'd say this is a clear
win. Since it is a strict superset, how about I send the current patch
as is (with extra documentation) and you submit the hw tstamp support
+ labels separately?

> ``Better to be consistent in that case'' would not be the current situation
> with the patch, right? I mean, for RX_RING, we report one of those 3, for
> TX_RING only the sw timestamp. So on one hand we might be consistent, but on
> the other hand we're not. ;-) With the above proposal, we could tackle this
> and stay consistent then. If you want, I could do that after your revised
> patch gets accepted (with hw ts support). Let me know what you think of
> that.

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Daniel Borkmann]

On 04/21/2013 06:42 PM, Willem de Bruijn wrote:
> win. Since it is a strict superset, how about I send the current patch
> as is (with extra documentation) and you submit the hw tstamp support
> + labels separately?

Ok, we could do that.

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Paul Chavent]

On 04/21/2013 06:42 PM, Willem de Bruijn wrote:
> Tx has the advantage that time sources can be chosen
> per socket independent of all other sockets

Sorry if my question is trivial.
I understand that when we require rx timestamping, we need to ask to the 
device to timestamp all incoming packets since we don't know the path of 
the packet in advance.
For tx timestamping, you seems to say that the request to timestamp the 
packet is contained in the skbuff and is done on a per packet basis ?

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Daniel Borkmann]

On 04/22/2013 10:19 AM, Paul Chavent wrote:
> On 04/21/2013 06:42 PM, Willem de Bruijn wrote:
>> Tx has the advantage that time sources can be chosen
>> per socket independent of all other sockets
>
> Sorry if my question is trivial.
> I understand that when we require rx timestamping, we need to ask to the device to timestamp all incoming packets since we don't know the path of the packet in advance.
> For tx timestamping, you seems to say that the request to timestamp the packet is contained in the skbuff and is done on a per packet basis ?

It's all in: Documentation/networking/timestamping.txt

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Mon, Apr 22, 2013 at 4:19 AM, Paul Chavent <Paul.Chavent@onera.fr> wrote:
>
>
> On 04/21/2013 06:42 PM, Willem de Bruijn wrote:
>>
>> Tx has the advantage that time sources can be chosen
>> per socket independent of all other sockets
>
>
> Sorry if my question is trivial.
> I understand that when we require rx timestamping, we need to ask to the
> device to timestamp all incoming packets since we don't know the path of the
> packet in advance.
> For tx timestamping, you seems to say that the request to timestamp the
> packet is contained in the skbuff and is done on a per packet basis ?

Yes. More precisely, this is set using a socket option and applies to
all packets on that socket sent since the last update of the socket
option.

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Richard Cochran]

On Fri, Apr 19, 2013 at 05:51:57PM -0400, Willem de Bruijn wrote:

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 898cf5c..af9185d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3327,12 +3327,8 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>  	if (!sk)
>  		return;
>  
> -	skb = skb_clone(orig_skb, GFP_ATOMIC);
> -	if (!skb)
> -		return;
> -
>  	if (hwtstamps) {
> -		*skb_hwtstamps(skb) =
> +		*skb_hwtstamps(orig_skb) =
>  			*hwtstamps;

And how does *hwtstamps get into the clone?

>  	} else {
>  		/*
> @@ -3340,9 +3336,13 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>  		 * so keep the shared tx_flags and only
>  		 * store software time stamp
>  		 */
> -		skb->tstamp = ktime_get_real();
> +		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;

Thanks,
Richard

Re: [PATCH net-next v2] packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Sat, Apr 20, 2013 at 12:43 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
> On Fri, Apr 19, 2013 at 05:51:57PM -0400, Willem de Bruijn wrote:
>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 898cf5c..af9185d 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3327,12 +3327,8 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>>       if (!sk)
>>               return;
>>
>> -     skb = skb_clone(orig_skb, GFP_ATOMIC);
>> -     if (!skb)
>> -             return;
>> -
>>       if (hwtstamps) {
>> -             *skb_hwtstamps(skb) =
>> +             *skb_hwtstamps(orig_skb) =
>>                       *hwtstamps;
>
> And how does *hwtstamps get into the clone?

The struct is part of skb_shared_info, which is stored immediately
after skb->end in the region shared by all clones.

>
>>       } else {
>>               /*
>> @@ -3340,9 +3336,13 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>>                * so keep the shared tx_flags and only
>>                * store software time stamp
>>                */
>> -             skb->tstamp = ktime_get_real();
>> +             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;
>
> Thanks,
> Richard

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Paul Chavent]

On 04/14/2013 12:52 PM, Daniel Borkmann wrote:
> Paul, since you've reported / requested this, your use case would be to
> fill
> the ring, trigger a sendto() and then loop through all the frames to
> check the
> tx timestamps for your custom protocol mockup, then fill the returned
> frames
> again, etc.?

I've run a test that create (and setup) a tx ring buffer with 8 frames, 
then fill the frames payload, then call sendto.
I've checked the timestamp by two means :
  - check the timestamp in the tx ring after the status became 
TP_STATUS_AVAILABLE again,
  - by issuing 8 recvmsg on the ERRQUEUE.
Both timestamp are coherents and increment themselves.

The tests has been done with a UML kernel, and with software timestamps.

 From my point of view, it seems usable.

However, I've found one strange behavior. I must call recvmsg as many 
times as i have submitted a frame, or not at all. If i only pop the 
ERRQUEUE for one or two message for instance, the next call to sentdo 
fails with "No message of desired type" (errno 42).


Paul.

RE: [PATCH] net-packet: tx timestamping on tpacket ring

[David Laight]

> > +	case TPACKET_V1:
> > +		h1 = frame;
> > +		h1->tp_sec = ts.tv_sec;
> > +		h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
> > +
> > +		flush_dcache_page(pgv_to_page(&h1->tp_sec));
> > +		flush_dcache_page(pgv_to_page(&h1->tp_usec));
> 
> Hmm, not sure, but could we also flush the dcache only once?

If it isn't a silly question, why is the dcache being flushed
here at all?

	David

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Mon, Apr 15, 2013 at 5:45 AM, David Laight <David.Laight@aculab.com> wrote:
>> > +   case TPACKET_V1:
>> > +           h1 = frame;
>> > +           h1->tp_sec = ts.tv_sec;
>> > +           h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
>> > +
>> > +           flush_dcache_page(pgv_to_page(&h1->tp_sec));
>> > +           flush_dcache_page(pgv_to_page(&h1->tp_usec));
>>
>> Hmm, not sure, but could we also flush the dcache only once?
>
> If it isn't a silly question, why is the dcache being flushed
> here at all?

I'm not an expert on this, but have a look at
Documentation/cachetlb.txt, specifically the bits on this function and
the discussion of aliasing: on virtually indexed cache architectures,
the same physical address may be cached in multiple cachelines at the
same time. If I understand correctly, updating the kernel logical
address does not necessarily invalidate the user virtual cacheline for
the same physical memory.

>         David
>
>
>

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[David Miller]

From: "David Laight" <David.Laight@ACULAB.COM>
Date: Mon, 15 Apr 2013 10:45:55 +0100

>> > +	case TPACKET_V1:
>> > +		h1 = frame;
>> > +		h1->tp_sec = ts.tv_sec;
>> > +		h1->tp_usec = ts.tv_nsec / NSEC_PER_USEC;
>> > +
>> > +		flush_dcache_page(pgv_to_page(&h1->tp_sec));
>> > +		flush_dcache_page(pgv_to_page(&h1->tp_usec));
>> 
>> Hmm, not sure, but could we also flush the dcache only once?
> 
> If it isn't a silly question, why is the dcache being flushed
> here at all?

Anything that gets written into userspace mapped pages via the
kernel linear mapping of the pages must be D-cache flushed into
order to achieve coherency on cpus that have virtually indexed
caches.

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Paul Chavent]

Hi.

On 04/13/2013 08:56 PM, Willem de Bruijn wrote:
> [...]
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3311,7 +3311,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>   		 * so keep the shared tx_flags and only
>   		 * store software time stamp
>   		 */
> -		skb->tstamp = ktime_get_real();
> +		orig_skb->tstamp = skb->tstamp = ktime_get_real();
>   	}

You said that "the orig_skb is usually freed shortly after
skb_tstamp_tx is called".

So i suppose that if you had coded it, that's because this orig_skb is 
the one that is given to the skb destructor. So when you call 
__packet_set_timestamp, in tpacket_destruct_skb, you get this timestamp 
? Am i right ?


Why we couldn't call
*skb_hwtstamps(orig_skb) = *skb_hwtstamps(skb) = *hwtstamps;
in order to get the hardware timestamping too ?

Thank for your help.

Paul.

Re: [PATCH] net-packet: tx timestamping on tpacket ring

[Willem de Bruijn]

On Mon, Apr 15, 2013 at 3:31 AM, Paul Chavent <Paul.Chavent@onera.fr> wrote:
> Hi.
>
>
> On 04/13/2013 08:56 PM, Willem de Bruijn wrote:
>>
>> [...]
>>
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3311,7 +3311,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>>                  * so keep the shared tx_flags and only
>>                  * store software time stamp
>>                  */
>> -               skb->tstamp = ktime_get_real();
>> +               orig_skb->tstamp = skb->tstamp = ktime_get_real();
>>         }
>
>
> You said that "the orig_skb is usually freed shortly after
> skb_tstamp_tx is called".
>
> So i suppose that if you had coded it, that's because this orig_skb is the
> one that is given to the skb destructor. So when you call
> __packet_set_timestamp, in tpacket_destruct_skb, you get this timestamp ? Am
> i right ?
>
>
> Why we couldn't call
> *skb_hwtstamps(orig_skb) = *skb_hwtstamps(skb) = *hwtstamps;
> in order to get the hardware timestamping too ?

Probably. That would also answer Daniel's question to the same
functionality. I'm a bit less familiar with the hardware path, so will
read up before just submitting it.

> Thank for your help.
>
> Paul.