[PATCH net-next] net: Print real skb addresses for all net events

[PATCH net-next] net: Print real skb addresses for all net events

[Subash Abhinov Kasiviswanathan]

Commit 65875073eddd ("net: use %px to print skb address in trace_netif_receive_skb")
added support for printing the real addresses for the events using
net_dev_template.

However, tracing the packet traversal shows a mix of hashes and real
addresses. Pasting a sample trace for reference-

ping-14249   [002] .....  3424.046612: netif_rx_entry: dev=lo napi_id=0x3 queue_mapping=0
skbaddr=00000000dcbed83e vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800
ip_summed=0 hash=0x00000000 l4_hash=0 len=84 data_len=0 truesize=768 mac_header_valid=1
mac_header=-14 nr_frags=0 gso_size=0 gso_type=0x0
ping-14249   [002] .....  3424.046615: netif_rx: dev=lo skbaddr=ffffff888e5d1000 len=84

Switch the trace print formats to %px for all the events to have a
consistent format of printing the real addresses in all cases.

Signed-off-by: Sean Tranchetti <quic_stranche@quicinc.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
---
 include/trace/events/net.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/trace/events/net.h b/include/trace/events/net.h
index 032b431..2651350 100644
--- a/include/trace/events/net.h
+++ b/include/trace/events/net.h
@@ -58,7 +58,7 @@ TRACE_EVENT(net_dev_start_xmit,
 		__entry->gso_type = skb_shinfo(skb)->gso_type;
 	),
 
-	TP_printk("dev=%s queue_mapping=%u skbaddr=%p vlan_tagged=%d vlan_proto=0x%04x vlan_tci=0x%04x protocol=0x%04x ip_summed=%d len=%u data_len=%u network_offset=%d transport_offset_valid=%d transport_offset=%d tx_flags=%d gso_size=%d gso_segs=%d gso_type=%#x",
+	TP_printk("dev=%s queue_mapping=%u skbaddr=%px vlan_tagged=%d vlan_proto=0x%04x vlan_tci=0x%04x protocol=0x%04x ip_summed=%d len=%u data_len=%u network_offset=%d transport_offset_valid=%d transport_offset=%d tx_flags=%d gso_size=%d gso_segs=%d gso_type=%#x",
 		  __get_str(name), __entry->queue_mapping, __entry->skbaddr,
 		  __entry->vlan_tagged, __entry->vlan_proto, __entry->vlan_tci,
 		  __entry->protocol, __entry->ip_summed, __entry->len,
@@ -91,7 +91,7 @@ TRACE_EVENT(net_dev_xmit,
 		__assign_str(name, dev->name);
 	),
 
-	TP_printk("dev=%s skbaddr=%p len=%u rc=%d",
+	TP_printk("dev=%s skbaddr=%px len=%u rc=%d",
 		__get_str(name), __entry->skbaddr, __entry->len, __entry->rc)
 );
 
@@ -215,7 +215,7 @@ DECLARE_EVENT_CLASS(net_dev_rx_verbose_template,
 		__entry->gso_type = skb_shinfo(skb)->gso_type;
 	),
 
-	TP_printk("dev=%s napi_id=%#x queue_mapping=%u skbaddr=%p vlan_tagged=%d vlan_proto=0x%04x vlan_tci=0x%04x protocol=0x%04x ip_summed=%d hash=0x%08x l4_hash=%d len=%u data_len=%u truesize=%u mac_header_valid=%d mac_header=%d nr_frags=%d gso_size=%d gso_type=%#x",
+	TP_printk("dev=%s napi_id=%#x queue_mapping=%u skbaddr=%px vlan_tagged=%d vlan_proto=0x%04x vlan_tci=0x%04x protocol=0x%04x ip_summed=%d hash=0x%08x l4_hash=%d len=%u data_len=%u truesize=%u mac_header_valid=%d mac_header=%d nr_frags=%d gso_size=%d gso_type=%#x",
 		  __get_str(name), __entry->napi_id, __entry->queue_mapping,
 		  __entry->skbaddr, __entry->vlan_tagged, __entry->vlan_proto,
 		  __entry->vlan_tci, __entry->protocol, __entry->ip_summed,
-- 
2.7.4

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Eric Dumazet]

On Fri, Jun 24, 2022 at 8:09 AM Subash Abhinov Kasiviswanathan
<quic_subashab@quicinc.com> wrote:
>
> Commit 65875073eddd ("net: use %px to print skb address in trace_netif_receive_skb")
> added support for printing the real addresses for the events using
> net_dev_template.
>

It is not clear why the 'real address' is needed in trace events.

I would rather do the opposite.

diff --git a/include/trace/events/net.h b/include/trace/events/net.h
index 032b431b987b63b5fab1d3c763643d6192f2c325..da611a7aaf970f541949cdd87ac9203c4c7e81b1
100644
--- a/include/trace/events/net.h
+++ b/include/trace/events/net.h
@@ -136,7 +136,7 @@ DECLARE_EVENT_CLASS(net_dev_template,
                __assign_str(name, skb->dev->name);
        ),

-       TP_printk("dev=%s skbaddr=%px len=%u",
+       TP_printk("dev=%s skbaddr=%p len=%u",
                __get_str(name), __entry->skbaddr, __entry->len)
 )

diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h
index 59c945b66f9c7469bc071e2a27efb8bfa9eb19f7..a3995925cb057021dc779344d19f7e3724f6df3c
100644
--- a/include/trace/events/qdisc.h
+++ b/include/trace/events/qdisc.h
@@ -41,7 +41,7 @@ TRACE_EVENT(qdisc_dequeue,
                __entry->txq_state      = txq->state;
        ),

-       TP_printk("dequeue ifindex=%d qdisc handle=0x%X parent=0x%X
txq_state=0x%lX packets=%d skbaddr=%px",
+       TP_printk("dequeue ifindex=%d qdisc handle=0x%X parent=0x%X
txq_state=0x%lX packets=%d skbaddr=%p",
                  __entry->ifindex, __entry->handle, __entry->parent,
                  __entry->txq_state, __entry->packets, __entry->skbaddr )
 );
@@ -70,7 +70,7 @@ TRACE_EVENT(qdisc_enqueue,
                __entry->parent  = qdisc->parent;
        ),

-       TP_printk("enqueue ifindex=%d qdisc handle=0x%X parent=0x%X
skbaddr=%px",
+       TP_printk("enqueue ifindex=%d qdisc handle=0x%X parent=0x%X skbaddr=%p",
                  __entry->ifindex, __entry->handle, __entry->parent,
__entry->skbaddr)
 );



> However, tracing the packet traversal shows a mix of hashes and real
> addresses. Pasting a sample trace for reference-
>
> ping-14249   [002] .....  3424.046612: netif_rx_entry: dev=lo napi_id=0x3 queue_mapping=0
> skbaddr=00000000dcbed83e vlan_tagged=0 vlan_proto=0x0000 vlan_tci=0x0000 protocol=0x0800
> ip_summed=0 hash=0x00000000 l4_hash=0 len=84 data_len=0 truesize=768 mac_header_valid=1
> mac_header=-14 nr_frags=0 gso_size=0 gso_type=0x0
> ping-14249   [002] .....  3424.046615: netif_rx: dev=lo skbaddr=ffffff888e5d1000 len=84
>
> Switch the trace print formats to %px for all the events to have a
> consistent format of printing the real addresses in all cases.
>
> Signed-off-by: Sean Tranchetti <quic_stranche@quicinc.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
> ---
>  include/trace/events/net.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/trace/events/net.h b/include/trace/events/net.h
> index 032b431..2651350 100644
> --- a/include/trace/events/net.h
> +++ b/include/trace/events/net.h
> @@ -58,7 +58,7 @@ TRACE_EVENT(net_dev_start_xmit,
>                 __entry->gso_type = skb_shinfo(skb)->gso_type;
>         ),
>
> -       TP_printk("dev=%s queue_mapping=%u skbaddr=%p vlan_tagged=%d vlan_proto=0x%04x vlan_tci=0x%04x protocol=0x%04x ip_summed=%d len=%u data_len=%u network_offset=%d transport_offset_valid=%d transport_offset=%d tx_flags=%d gso_size=%d gso_segs=%d gso_type=%#x",
> +       TP_printk("dev=%s queue_mapping=%u skbaddr=%px vlan_tagged=%d vlan_proto=0x%04x vlan_tci=0x%04x protocol=0x%04x ip_summed=%d len=%u data_len=%u network_offset=%d transport_offset_valid=%d transport_offset=%d tx_flags=%d gso_size=%d gso_segs=%d gso_type=%#x",
>                   __get_str(name), __entry->queue_mapping, __entry->skbaddr,
>                   __entry->vlan_tagged, __entry->vlan_proto, __entry->vlan_tci,
>                   __entry->protocol, __entry->ip_summed, __entry->len,
> @@ -91,7 +91,7 @@ TRACE_EVENT(net_dev_xmit,
>                 __assign_str(name, dev->name);
>         ),
>
> -       TP_printk("dev=%s skbaddr=%p len=%u rc=%d",
> +       TP_printk("dev=%s skbaddr=%px len=%u rc=%d",
>                 __get_str(name), __entry->skbaddr, __entry->len, __entry->rc)
>  );
>
> @@ -215,7 +215,7 @@ DECLARE_EVENT_CLASS(net_dev_rx_verbose_template,
>                 __entry->gso_type = skb_shinfo(skb)->gso_type;
>         ),
>
> -       TP_printk("dev=%s napi_id=%#x queue_mapping=%u skbaddr=%p vlan_tagged=%d vlan_proto=0x%04x vlan_tci=0x%04x protocol=0x%04x ip_summed=%d hash=0x%08x l4_hash=%d len=%u data_len=%u truesize=%u mac_header_valid=%d mac_header=%d nr_frags=%d gso_size=%d gso_type=%#x",
> +       TP_printk("dev=%s napi_id=%#x queue_mapping=%u skbaddr=%px vlan_tagged=%d vlan_proto=0x%04x vlan_tci=0x%04x protocol=0x%04x ip_summed=%d hash=0x%08x l4_hash=%d len=%u data_len=%u truesize=%u mac_header_valid=%d mac_header=%d nr_frags=%d gso_size=%d gso_type=%#x",
>                   __get_str(name), __entry->napi_id, __entry->queue_mapping,
>                   __entry->skbaddr, __entry->vlan_tagged, __entry->vlan_proto,
>                   __entry->vlan_tci, __entry->protocol, __entry->ip_summed,
> --
> 2.7.4
>

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Subash Abhinov Kasiviswanathan (KS)]

On 6/24/2022 12:27 AM, Eric Dumazet wrote:
> On Fri, Jun 24, 2022 at 8:09 AM Subash Abhinov Kasiviswanathan
> <quic_subashab@quicinc.com> wrote:
>>
>> Commit 65875073eddd ("net: use %px to print skb address in trace_netif_receive_skb")
>> added support for printing the real addresses for the events using
>> net_dev_template.
>>
> 
> It is not clear why the 'real address' is needed in trace events.
> 
> I would rather do the opposite.
> 

We don't need the real address. We just need the events to display in 
the same format - hashed address is fine.

> diff --git a/include/trace/events/net.h b/include/trace/events/net.h
> index 032b431b987b63b5fab1d3c763643d6192f2c325..da611a7aaf970f541949cdd87ac9203c4c7e81b1
> 100644
> --- a/include/trace/events/net.h
> +++ b/include/trace/events/net.h
> @@ -136,7 +136,7 @@ DECLARE_EVENT_CLASS(net_dev_template,
>                  __assign_str(name, skb->dev->name);
>          ),
> 
> -       TP_printk("dev=%s skbaddr=%px len=%u",
> +       TP_printk("dev=%s skbaddr=%p len=%u",
>                  __get_str(name), __entry->skbaddr, __entry->len)
>   )
> 

Qitao / Cong, do you have any concerns as it ends up reverting commit 
65875073eddd ("net: use %px to print skb address in 
trace_netif_receive_skb")

> diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h
> index 59c945b66f9c7469bc071e2a27efb8bfa9eb19f7..a3995925cb057021dc779344d19f7e3724f6df3c
> 100644
> --- a/include/trace/events/qdisc.h
> +++ b/include/trace/events/qdisc.h
> @@ -41,7 +41,7 @@ TRACE_EVENT(qdisc_dequeue,
>                  __entry->txq_state      = txq->state;
>          ),
> 
> -       TP_printk("dequeue ifindex=%d qdisc handle=0x%X parent=0x%X
> txq_state=0x%lX packets=%d skbaddr=%px",
> +       TP_printk("dequeue ifindex=%d qdisc handle=0x%X parent=0x%X
> txq_state=0x%lX packets=%d skbaddr=%p",
>                    __entry->ifindex, __entry->handle, __entry->parent,
>                    __entry->txq_state, __entry->packets, __entry->skbaddr )
>   );
> @@ -70,7 +70,7 @@ TRACE_EVENT(qdisc_enqueue,
>                  __entry->parent  = qdisc->parent;
>          ),
> 
> -       TP_printk("enqueue ifindex=%d qdisc handle=0x%X parent=0x%X
> skbaddr=%px",
> +       TP_printk("enqueue ifindex=%d qdisc handle=0x%X parent=0x%X skbaddr=%p",
>                    __entry->ifindex, __entry->handle, __entry->parent,
> __entry->skbaddr)
>   );
> 
> 

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Cong Wang]

On Fri, Jun 24, 2022 at 08:27:34AM +0200, Eric Dumazet wrote:
> On Fri, Jun 24, 2022 at 8:09 AM Subash Abhinov Kasiviswanathan
> <quic_subashab@quicinc.com> wrote:
> >
> > Commit 65875073eddd ("net: use %px to print skb address in trace_netif_receive_skb")
> > added support for printing the real addresses for the events using
> > net_dev_template.
> >
> 
> It is not clear why the 'real address' is needed in trace events.

Because hashed address is _further_ from being unique, we could even
observe same hashed addresses with a few manually injected packets.

Real address is much better. Although definitely it can't guarantee
uniqueness, it is already the cheapest way to identify the packets in
tracing. (Surely you can add an ID generator or something similiar, but
nothing is cheaper than just using the real address.)

> 
> I would rather do the opposite.
>

Strongly disagree. I will sent a revert.

Thanks.

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Cong Wang]

On Fri, Jun 24, 2022 at 02:48:24AM -0600, Subash Abhinov Kasiviswanathan (KS) wrote:
> 
> 
> On 6/24/2022 12:27 AM, Eric Dumazet wrote:
> > On Fri, Jun 24, 2022 at 8:09 AM Subash Abhinov Kasiviswanathan
> > <quic_subashab@quicinc.com> wrote:
> > > 
> > > Commit 65875073eddd ("net: use %px to print skb address in trace_netif_receive_skb")
> > > added support for printing the real addresses for the events using
> > > net_dev_template.
> > > 
> > 
> > It is not clear why the 'real address' is needed in trace events.
> > 
> > I would rather do the opposite.
> > 
> 
> We don't need the real address. We just need the events to display in the
> same format - hashed address is fine.
> 
> > diff --git a/include/trace/events/net.h b/include/trace/events/net.h
> > index 032b431b987b63b5fab1d3c763643d6192f2c325..da611a7aaf970f541949cdd87ac9203c4c7e81b1
> > 100644
> > --- a/include/trace/events/net.h
> > +++ b/include/trace/events/net.h
> > @@ -136,7 +136,7 @@ DECLARE_EVENT_CLASS(net_dev_template,
> >                  __assign_str(name, skb->dev->name);
> >          ),
> > 
> > -       TP_printk("dev=%s skbaddr=%px len=%u",
> > +       TP_printk("dev=%s skbaddr=%p len=%u",
> >                  __get_str(name), __entry->skbaddr, __entry->len)
> >   )
> > 
> 
> Qitao / Cong, do you have any concerns as it ends up reverting commit
> 65875073eddd ("net: use %px to print skb address in
> trace_netif_receive_skb")

Yes, see this example:
https://lists.openwall.net/netdev/2021/07/28/24

Thanks.

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Eric Dumazet]

On Mon, Jun 27, 2022 at 9:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Fri, Jun 24, 2022 at 08:27:34AM +0200, Eric Dumazet wrote:
> > On Fri, Jun 24, 2022 at 8:09 AM Subash Abhinov Kasiviswanathan
> > <quic_subashab@quicinc.com> wrote:
> > >
> > > Commit 65875073eddd ("net: use %px to print skb address in trace_netif_receive_skb")
> > > added support for printing the real addresses for the events using
> > > net_dev_template.
> > >
> >
> > It is not clear why the 'real address' is needed in trace events.
>
> Because hashed address is _further_ from being unique, we could even
> observe same hashed addresses with a few manually injected packets.
>
> Real address is much better. Although definitely it can't guarantee
> uniqueness, it is already the cheapest way to identify the packets in
> tracing. (Surely you can add an ID generator or something similiar, but
> nothing is cheaper than just using the real address.)
>
> >
> > I would rather do the opposite.
> >
>
> Strongly disagree. I will sent a revert.
>

Make sure to include lkml for this discussion :

Vast majority (100%) of TP_printk() using %p use %p, not %px

$ git grep -n TP_printk|grep %p|wc -l
425
$ git grep -n TP_printk|grep %px|wc -l
0


> Thanks.

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Cong Wang]

On Mon, Jun 27, 2022 at 12:33 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Jun 27, 2022 at 9:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Fri, Jun 24, 2022 at 08:27:34AM +0200, Eric Dumazet wrote:
> > > On Fri, Jun 24, 2022 at 8:09 AM Subash Abhinov Kasiviswanathan
> > > <quic_subashab@quicinc.com> wrote:
> > > >
> > > > Commit 65875073eddd ("net: use %px to print skb address in trace_netif_receive_skb")
> > > > added support for printing the real addresses for the events using
> > > > net_dev_template.
> > > >
> > >
> > > It is not clear why the 'real address' is needed in trace events.
> >
> > Because hashed address is _further_ from being unique, we could even
> > observe same hashed addresses with a few manually injected packets.
> >
> > Real address is much better. Although definitely it can't guarantee
> > uniqueness, it is already the cheapest way to identify the packets in
> > tracing. (Surely you can add an ID generator or something similiar, but
> > nothing is cheaper than just using the real address.)
> >
> > >
> > > I would rather do the opposite.
> > >
> >
> > Strongly disagree. I will sent a revert.
> >
>
> Make sure to include lkml for this discussion :

Already did:
https://lore.kernel.org/all/CAM_iQpV3Qm_GTfCX1E_OC0PXu+diT9QHtPt4OYcJdyGRcA37Sw@mail.gmail.com/

>
> Vast majority (100%) of TP_printk() using %p use %p, not %px
>
> $ git grep -n TP_printk|grep %p|wc -l
> 425
> $ git grep -n TP_printk|grep %px|wc -l
> 0

You are changing this topic, no one here in this thread cares about non-skb
addresses.

Thanks.

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Eric Dumazet]

On Mon, Jun 27, 2022 at 9:41 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Mon, Jun 27, 2022 at 12:33 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Mon, Jun 27, 2022 at 9:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Fri, Jun 24, 2022 at 08:27:34AM +0200, Eric Dumazet wrote:
> > > > On Fri, Jun 24, 2022 at 8:09 AM Subash Abhinov Kasiviswanathan
> > > > <quic_subashab@quicinc.com> wrote:
> > > > >
> > > > > Commit 65875073eddd ("net: use %px to print skb address in trace_netif_receive_skb")
> > > > > added support for printing the real addresses for the events using
> > > > > net_dev_template.
> > > > >
> > > >
> > > > It is not clear why the 'real address' is needed in trace events.
> > >
> > > Because hashed address is _further_ from being unique, we could even
> > > observe same hashed addresses with a few manually injected packets.
> > >
> > > Real address is much better. Although definitely it can't guarantee
> > > uniqueness, it is already the cheapest way to identify the packets in
> > > tracing. (Surely you can add an ID generator or something similiar, but
> > > nothing is cheaper than just using the real address.)
> > >
> > > >
> > > > I would rather do the opposite.
> > > >
> > >
> > > Strongly disagree. I will sent a revert.
> > >
> >
> > Make sure to include lkml for this discussion :
>
> Already did:
> https://lore.kernel.org/all/CAM_iQpV3Qm_GTfCX1E_OC0PXu+diT9QHtPt4OYcJdyGRcA37Sw@mail.gmail.com/
>
> >
> > Vast majority (100%) of TP_printk() using %p use %p, not %px
> >
> > $ git grep -n TP_printk|grep %p|wc -l
> > 425
> > $ git grep -n TP_printk|grep %px|wc -l
> > 0
>
> You are changing this topic, no one here in this thread cares about non-skb
> addresses.
>

I will not ack a revert.

Unless you get ACK from Linus Torvalds maybe.

We have ways for developers : no_hash_pointers

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Cong Wang]

On Mon, Jun 27, 2022 at 12:46 PM Eric Dumazet <edumazet@google.com> wrote:
>
> We have ways for developers : no_hash_pointers

I have no idea why you keep generalizing this topic to all pointers.

You have to realize no one even argues about any other pointers
than just skb's. You must be kidding when you suggest to disable
all hash pointers when people only want skb.

Re: [PATCH net-next] net: Print real skb addresses for all net events

[Cong Wang]

Hmm, I just noticed there is actually a hash-ptr option for trace ring
buffer:

  hash-ptr
        When set, "%p" in the event printk format displays the
        hashed pointer value instead of real address.
        This will be useful if you want to find out which hashed
        value is corresponding to the real value in trace log.

I am glad people noticed the usefulness of real addresses in tracepoint
before I did.

I will backport this to our 5.10 kernel.

Thanks.