Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kishen Maloor <kishen.maloor@intel.com>
To: "Jesper Dangaard Brouer" <jbrouer@redhat.com>,
	bpf@vger.kernel.org, netdev@vger.kernel.org, hawk@kernel.org,
	magnus.karlsson@intel.com, "Björn Töpel" <bjorn@kernel.org>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Jithu Joseph" <jithu.joseph@intel.com>
Cc: <brouer@redhat.com>
Subject: Re: [RFC bpf-next 2/5] libbpf: SO_TXTIME support in AF_XDP
Date: Thu, 19 Aug 2021 15:32:55 -0400	[thread overview]
Message-ID: <b3c05e8d-bb93-0287-2a96-e8d7f690be83@intel.com> (raw)
In-Reply-To: <31fb6a84-562e-a41d-0614-061e1f475db3@redhat.com>

On 8/18/21 5:49 AM, Jesper Dangaard Brouer wrote:
> 
> 
> On 03/08/2021 19.10, Kishen Maloor wrote:
>> This change adds userspace support for SO_TXTIME in AF_XDP
>> to include a specific TXTIME (aka "Launch Time")
>> with XDP frames issued from userspace XDP applications.
>>
>> The userspace API has been expanded with two helper functons:
>>
>> - int xsk_socket__enable_so_txtime(struct xsk_socket *xsk, bool enable)
>>     Sets the SO_TXTIME option on the AF_XDP socket (using setsockopt()).
>>
>> - void xsk_umem__set_md_txtime(void *umem_area, __u64 chunkAddr,
>>                                 __s64 txtime)
>>     Packages the application supplied TXTIME into struct xdp_user_tx_metadata:
>>     struct xdp_user_tx_metadata {
> 
> Struct name is important and becomes UAPI. I'm not 100% convinced this is a good name.

Sure, we can choose a better name. Open to suggestions.

> 
> For BPF programs libbpf can at load-time lookup the 'btf_id' via:
> 
>   btf_id = bpf_core_type_id_kernel(struct xdp_user_tx_metadata);
> 
> Example see[1]
>  [1] https://github.com/xdp-project/bpf-examples/commit/2390b4b11079
> 
> I know this is AF_XDP userspace, but I hope Andrii can help guide us howto expose the bpf_core_type_id_kernel() API via libbpf, to be used by the AF_XDP userspace program.
> 
> 
>>          __u64 timestamp;
>>          __u32 md_valid;
>>          __u32 btf_id;
>>     };
> 
> I assume this struct is intended to be BTF "described".

Yes, the intention here was to be future-proof in a sense, as once all the related infrastructure/tooling for BTF comes into existence, it should hopefully be a 
simple matter to adapt this code to work with that.

> 
> Struct member *names* are very important for BTF. (E.g. see how 'spinlock' have special meaning and is matched internally by kernel).
> 
> The member name 'timestamp' seems too generic.  This is a very specific 'LaunchTime' feature, which could be reflected in the name.

Yes, LaunchTime is the specific use case addressed by this RFC, so open to suggestions on the struct member name (e.g. launch_time?).

> 
> Later it looks like you are encoding the "type" in md_valid, which I guess it is needed as timestamps can have different "types".

No, the purpose of md_valid is to flag all those md fields that have been set/conveyed by this metadata. So, XDP_METADATA_USER_TX_TIMESTAMP is 
meant as a reference to the 'timestamp' field in struct xdp_user_tx_metadata.

> E.g. some of the clockid_t types from clock_gettime(2):
>  CLOCK_REALTIME
>  CLOCK_TAI
>  CLOCK_MONOTONIC
>  CLOCK_BOOTTIME
> 
> Which of these timestamp does XDP_METADATA_USER_TX_TIMESTAMP represent?
> Or what timestamp type is the expected one?

CLOCK_TAI is what we use for LaunchTime, and supported by IGC/i225.

> 
> In principle we could name the member 'Launch_Time_CLOCK_TAI' to encoded the clockid_t type in the name, but I think that would be too much (and require too advanced BTF helpers to extract type, having a clock_type member is easier to understand/consume from C).
> 
> 
>>     and stores it in the XDP metadata area, which precedes the XDP frame.
>>
>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
>> ---
>>   tools/include/uapi/linux/if_xdp.h     |  2 ++
>>   tools/include/uapi/linux/xdp_md_std.h | 14 ++++++++++++++
>>   tools/lib/bpf/xsk.h                   | 27 ++++++++++++++++++++++++++-
>>   3 files changed, 42 insertions(+), 1 deletion(-)
>>   create mode 100644 tools/include/uapi/linux/xdp_md_std.h
>>
>> diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h
>> index a78a8096f4ce..31f81f82ed86 100644
>> --- a/tools/include/uapi/linux/if_xdp.h
>> +++ b/tools/include/uapi/linux/if_xdp.h
>> @@ -106,6 +106,8 @@ struct xdp_desc {
>>       __u32 options;
>>   };
>>   +#define XDP_DESC_OPTION_METADATA (1 << 0)
>> +
>>   /* UMEM descriptor is __u64 */
>>     #endif /* _LINUX_IF_XDP_H */
>> diff --git a/tools/include/uapi/linux/xdp_md_std.h b/tools/include/uapi/linux/xdp_md_std.h
>> new file mode 100644
>> index 000000000000..f00996a61639
>> --- /dev/null
>> +++ b/tools/include/uapi/linux/xdp_md_std.h
>> @@ -0,0 +1,14 @@
>> +#ifndef _UAPI_LINUX_XDP_MD_STD_H
>> +#define _UAPI_LINUX_XDP_MD_STD_H
>> +
>> +#include <linux/types.h>
>> +
>> +#define XDP_METADATA_USER_TX_TIMESTAMP 0x1
>> +
>> +struct xdp_user_tx_metadata {
>> +    __u64 timestamp;
>> +    __u32 md_valid;
>> +    __u32 btf_id;
>> +};
>> +
>> +#endif /* _UAPI_LINUX_XDP_MD_STD_H */
>> diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
>> index 01c12dca9c10..1b52ffe1c9a3 100644
>> --- a/tools/lib/bpf/xsk.h
>> +++ b/tools/lib/bpf/xsk.h
>> @@ -16,7 +16,8 @@
>>   #include <stdint.h>
>>   #include <stdbool.h>
>>   #include <linux/if_xdp.h>
>> -
>> +#include <linux/xdp_md_std.h>
>> +#include <errno.h>
>>   #include "libbpf.h"
>>     #ifdef __cplusplus
>> @@ -248,6 +249,30 @@ static inline __u64 xsk_umem__add_offset_to_addr(__u64 addr)
>>   LIBBPF_API int xsk_umem__fd(const struct xsk_umem *umem);
>>   LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk);
>>   +/* Helpers for SO_TXTIME */
>> +
>> +static inline void xsk_umem__set_md_txtime(void *umem_area, __u64 addr, __s64 txtime)
>> +{
>> +    struct xdp_user_tx_metadata *md;
>> +
>> +    md = (struct xdp_user_tx_metadata *)&((char *)umem_area)[addr];
>> +
>> +    md->timestamp = txtime;
>> +    md->md_valid |= XDP_METADATA_USER_TX_TIMESTAMP;
> 
> Is this encoding the "type" of the timestamp?

No, it is hinting at the field (timestamp) being conveyed in this metadata.

> 
> I don't see the btf_id being updated.  Does that happen in another patch?

No. As I understand, BTF support and it's overall applicability (particularly from AF_XDP userspace) is still a WIP. So, btf_id currently exists as a 
placeholder to facilitate future BTF integration.
 
Meanwhile, this RFC re-purposes SO_TXTIME on the control path from AF_XDP userspace to exercise the LaunchTime feature in the presented pattern.

> 
> As I note above we are current;y lacking an libbpf equivalent bpf_core_type_id_kernel() lookup function in userspace.
> 
>> +}
>> +
>> +static inline int xsk_socket__enable_so_txtime(struct xsk_socket *xsk, bool enable)
>> +{
>> +    unsigned int val = (enable) ? 1 : 0;
>> +    int err;
>> +
>> +    err = setsockopt(xsk_socket__fd(xsk), SOL_XDP, SO_TXTIME, &val, sizeof(val));
>> +
>> +    if (err)
>> +        return -errno;
>> +    return 0;
>> +}
>> +
>>   #define XSK_RING_CONS__DEFAULT_NUM_DESCS      2048
>>   #define XSK_RING_PROD__DEFAULT_NUM_DESCS      2048
>>   #define XSK_UMEM__DEFAULT_FRAME_SHIFT    12 /* 4096 bytes */
>>
> 


  reply	other threads:[~2021-08-19 19:33 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-03 17:10 [RFC bpf-next 0/5] " Kishen Maloor
2021-08-03 17:10 ` [RFC bpf-next 1/5] net: xdp: " Kishen Maloor
2021-08-03 17:10 ` [RFC bpf-next 2/5] libbpf: " Kishen Maloor
2021-08-06 23:08   ` Andrii Nakryiko
2021-08-18  9:49   ` Jesper Dangaard Brouer
2021-08-19 19:32     ` Kishen Maloor [this message]
2021-08-03 17:10 ` [RFC bpf-next 3/5] igc: Launchtime support in XDP Tx ZC path Kishen Maloor
2021-08-05 17:53   ` Kishen Maloor
2021-08-03 17:10 ` [RFC bpf-next 4/5] samples/bpf/xdpsock_user.c: Make get_nsecs() generic Kishen Maloor
2021-08-03 17:10 ` [RFC bpf-next 5/5] samples/bpf/xdpsock_user.c: Launchtime/TXTIME API usage Kishen Maloor
2021-08-18  8:54   ` Jesper Dangaard Brouer
2021-08-19 19:32     ` Kishen Maloor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b3c05e8d-bb93-0287-2a96-e8d7f690be83@intel.com \
    --to=kishen.maloor@intel.com \
    --cc=andrii@kernel.org \
    --cc=bjorn@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=hawk@kernel.org \
    --cc=jbrouer@redhat.com \
    --cc=jithu.joseph@intel.com \
    --cc=magnus.karlsson@intel.com \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [RFC bpf-next 2/5] libbpf: SO_TXTIME support in AF_XDP' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).