| Message ID | 20190730085400.10376-4-kevin.laatz@intel.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [bpf-next,v4,01/11] i40e: simplify Rx buffer recycle | expand |
On Tue, 30 Jul 2019 at 19:43, Kevin Laatz <kevin.laatz@intel.com> wrote: > > This patch adds a 'flags' field to the umem_config and umem_reg structs. > This will allow for more options to be added for configuring umems. > > The first use for the flags field is to add a flag for unaligned chunks > mode. These flags can either be user-provided or filled with a default. > > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > --- > v2: > - Removed the headroom check from this patch. It has moved to the > previous patch. > > v4: > - modified chunk flag define > --- > tools/include/uapi/linux/if_xdp.h | 9 +++++++-- > tools/lib/bpf/xsk.c | 3 +++ > tools/lib/bpf/xsk.h | 2 ++ > 3 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h > index faaa5ca2a117..a691802d7915 100644 > --- a/tools/include/uapi/linux/if_xdp.h > +++ b/tools/include/uapi/linux/if_xdp.h > @@ -17,6 +17,10 @@ > #define XDP_COPY (1 << 1) /* Force copy-mode */ > #define XDP_ZEROCOPY (1 << 2) /* Force zero-copy mode */ > > +/* Flags for xsk_umem_config flags */ > +#define XDP_UMEM_UNALIGNED_CHUNK_FLAG_SHIFT 15 > +#define XDP_UMEM_UNALIGNED_CHUNK_FLAG (1 << XDP_UMEM_UNALIGNED_CHUNK_FLAG_SHIFT) > + > struct sockaddr_xdp { > __u16 sxdp_family; > __u16 sxdp_flags; > @@ -49,8 +53,9 @@ struct xdp_mmap_offsets { > #define XDP_OPTIONS 8 > > struct xdp_umem_reg { > - __u64 addr; /* Start of packet data area */ > - __u64 len; /* Length of packet data area */ > + __u64 addr; /* Start of packet data area */ > + __u64 len:48; /* Length of packet data area */ > + __u64 flags:16; /* Flags for umem */ So, the flags member moved from struct sockaddr_xdp to struct xdp_umem_reg. Makes sense. However, I'm not a fan of the bitfield. Why not just add the flags member after the last member (headroom) and deal with it in xsk.c/xsk_setsockopt and libbpf? The bitfield preserves the size, but makes it hard to read/error prone IMO. Björn > __u32 chunk_size; > __u32 headroom; > }; > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 5007b5d4fd2c..5e7e4d420ee0 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -116,6 +116,7 @@ static void xsk_set_umem_config(struct xsk_umem_config *cfg, > cfg->comp_size = XSK_RING_CONS__DEFAULT_NUM_DESCS; > cfg->frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE; > cfg->frame_headroom = XSK_UMEM__DEFAULT_FRAME_HEADROOM; > + cfg->flags = XSK_UMEM__DEFAULT_FLAGS; > return; > } > > @@ -123,6 +124,7 @@ static void xsk_set_umem_config(struct xsk_umem_config *cfg, > cfg->comp_size = usr_cfg->comp_size; > cfg->frame_size = usr_cfg->frame_size; > cfg->frame_headroom = usr_cfg->frame_headroom; > + cfg->flags = usr_cfg->flags; > } > > static int xsk_set_xdp_socket_config(struct xsk_socket_config *cfg, > @@ -182,6 +184,7 @@ int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size, > mr.len = size; > mr.chunk_size = umem->config.frame_size; > mr.headroom = umem->config.frame_headroom; > + mr.flags = umem->config.flags; > > err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_REG, &mr, sizeof(mr)); > if (err) { > diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h > index 833a6e60d065..44a03d8c34b9 100644 > --- a/tools/lib/bpf/xsk.h > +++ b/tools/lib/bpf/xsk.h > @@ -170,12 +170,14 @@ LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk); > #define XSK_UMEM__DEFAULT_FRAME_SHIFT 12 /* 4096 bytes */ > #define XSK_UMEM__DEFAULT_FRAME_SIZE (1 << XSK_UMEM__DEFAULT_FRAME_SHIFT) > #define XSK_UMEM__DEFAULT_FRAME_HEADROOM 0 > +#define XSK_UMEM__DEFAULT_FLAGS 0 > > struct xsk_umem_config { > __u32 fill_size; > __u32 comp_size; > __u32 frame_size; > __u32 frame_headroom; > + __u32 flags; > }; > > /* Flags for the libbpf_flags field. */ > -- > 2.17.1 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Tue, 30 Jul 2019 at 19:43, Kevin Laatz <kevin.laatz@intel.com> wrote: > > This patch adds a 'flags' field to the umem_config and umem_reg structs. > This will allow for more options to be added for configuring umems. > > The first use for the flags field is to add a flag for unaligned chunks > mode. These flags can either be user-provided or filled with a default. > > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > --- > v2: > - Removed the headroom check from this patch. It has moved to the > previous patch. > > v4: > - modified chunk flag define > --- > tools/include/uapi/linux/if_xdp.h | 9 +++++++-- > tools/lib/bpf/xsk.c | 3 +++ > tools/lib/bpf/xsk.h | 2 ++ > 3 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h > index faaa5ca2a117..a691802d7915 100644 > --- a/tools/include/uapi/linux/if_xdp.h > +++ b/tools/include/uapi/linux/if_xdp.h > @@ -17,6 +17,10 @@ > #define XDP_COPY (1 << 1) /* Force copy-mode */ > #define XDP_ZEROCOPY (1 << 2) /* Force zero-copy mode */ > > +/* Flags for xsk_umem_config flags */ > +#define XDP_UMEM_UNALIGNED_CHUNK_FLAG_SHIFT 15 > +#define XDP_UMEM_UNALIGNED_CHUNK_FLAG (1 << XDP_UMEM_UNALIGNED_CHUNK_FLAG_SHIFT) > + > struct sockaddr_xdp { > __u16 sxdp_family; > __u16 sxdp_flags; > @@ -49,8 +53,9 @@ struct xdp_mmap_offsets { > #define XDP_OPTIONS 8 > > struct xdp_umem_reg { > - __u64 addr; /* Start of packet data area */ > - __u64 len; /* Length of packet data area */ > + __u64 addr; /* Start of packet data area */ > + __u64 len:48; /* Length of packet data area */ > + __u64 flags:16; /* Flags for umem */ > __u32 chunk_size; > __u32 headroom; > }; > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 5007b5d4fd2c..5e7e4d420ee0 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -116,6 +116,7 @@ static void xsk_set_umem_config(struct xsk_umem_config *cfg, > cfg->comp_size = XSK_RING_CONS__DEFAULT_NUM_DESCS; > cfg->frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE; > cfg->frame_headroom = XSK_UMEM__DEFAULT_FRAME_HEADROOM; > + cfg->flags = XSK_UMEM__DEFAULT_FLAGS; > return; > } > > @@ -123,6 +124,7 @@ static void xsk_set_umem_config(struct xsk_umem_config *cfg, > cfg->comp_size = usr_cfg->comp_size; > cfg->frame_size = usr_cfg->frame_size; > cfg->frame_headroom = usr_cfg->frame_headroom; > + cfg->flags = usr_cfg->flags; > } > > static int xsk_set_xdp_socket_config(struct xsk_socket_config *cfg, > @@ -182,6 +184,7 @@ int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size, > mr.len = size; > mr.chunk_size = umem->config.frame_size; > mr.headroom = umem->config.frame_headroom; > + mr.flags = umem->config.flags; > > err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_REG, &mr, sizeof(mr)); > if (err) { > diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h > index 833a6e60d065..44a03d8c34b9 100644 > --- a/tools/lib/bpf/xsk.h > +++ b/tools/lib/bpf/xsk.h > @@ -170,12 +170,14 @@ LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk); > #define XSK_UMEM__DEFAULT_FRAME_SHIFT 12 /* 4096 bytes */ > #define XSK_UMEM__DEFAULT_FRAME_SIZE (1 << XSK_UMEM__DEFAULT_FRAME_SHIFT) > #define XSK_UMEM__DEFAULT_FRAME_HEADROOM 0 > +#define XSK_UMEM__DEFAULT_FLAGS 0 > > struct xsk_umem_config { > __u32 fill_size; > __u32 comp_size; > __u32 frame_size; > __u32 frame_headroom; > + __u32 flags; And the flags addition here, unfortunately, requires symbol versioning of xsk_umem__create(). That'll be the first in libbpf! :-) Björn > }; > > /* Flags for the libbpf_flags field. */ > -- > 2.17.1 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Wed, Jul 31, 2019 at 8:21 AM Björn Töpel <bjorn.topel@gmail.com> wrote: > > On Tue, 30 Jul 2019 at 19:43, Kevin Laatz <kevin.laatz@intel.com> wrote: > > > > This patch adds a 'flags' field to the umem_config and umem_reg structs. > > This will allow for more options to be added for configuring umems. > > > > The first use for the flags field is to add a flag for unaligned chunks > > mode. These flags can either be user-provided or filled with a default. > > > > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > > > --- > > v2: > > - Removed the headroom check from this patch. It has moved to the > > previous patch. > > > > v4: > > - modified chunk flag define > > --- [...] > > diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h > > index 833a6e60d065..44a03d8c34b9 100644 > > --- a/tools/lib/bpf/xsk.h > > +++ b/tools/lib/bpf/xsk.h > > @@ -170,12 +170,14 @@ LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk); > > #define XSK_UMEM__DEFAULT_FRAME_SHIFT 12 /* 4096 bytes */ > > #define XSK_UMEM__DEFAULT_FRAME_SIZE (1 << XSK_UMEM__DEFAULT_FRAME_SHIFT) > > #define XSK_UMEM__DEFAULT_FRAME_HEADROOM 0 > > +#define XSK_UMEM__DEFAULT_FLAGS 0 > > > > struct xsk_umem_config { > > __u32 fill_size; > > __u32 comp_size; > > __u32 frame_size; > > __u32 frame_headroom; > > + __u32 flags; > > And the flags addition here, unfortunately, requires symbol versioning > of xsk_umem__create(). That'll be the first in libbpf! :-) xsk_umem_config is passed by pointer to xsk_umem__create(), so this doesn't break ABI, does it? > > > Björn > > > }; > > > > /* Flags for the libbpf_flags field. */ > > -- > > 2.17.1 > > > > _______________________________________________ > > Intel-wired-lan mailing list > > Intel-wired-lan@osuosl.org > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Thu, 1 Aug 2019 at 08:59, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Jul 31, 2019 at 8:21 AM Björn Töpel <bjorn.topel@gmail.com> wrote: > > > > On Tue, 30 Jul 2019 at 19:43, Kevin Laatz <kevin.laatz@intel.com> wrote: > > > > > > This patch adds a 'flags' field to the umem_config and umem_reg structs. > > > This will allow for more options to be added for configuring umems. > > > > > > The first use for the flags field is to add a flag for unaligned chunks > > > mode. These flags can either be user-provided or filled with a default. > > > > > > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > > > > > --- > > > v2: > > > - Removed the headroom check from this patch. It has moved to the > > > previous patch. > > > > > > v4: > > > - modified chunk flag define > > > --- > > [...] > > > > diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h > > > index 833a6e60d065..44a03d8c34b9 100644 > > > --- a/tools/lib/bpf/xsk.h > > > +++ b/tools/lib/bpf/xsk.h > > > @@ -170,12 +170,14 @@ LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk); > > > #define XSK_UMEM__DEFAULT_FRAME_SHIFT 12 /* 4096 bytes */ > > > #define XSK_UMEM__DEFAULT_FRAME_SIZE (1 << XSK_UMEM__DEFAULT_FRAME_SHIFT) > > > #define XSK_UMEM__DEFAULT_FRAME_HEADROOM 0 > > > +#define XSK_UMEM__DEFAULT_FLAGS 0 > > > > > > struct xsk_umem_config { > > > __u32 fill_size; > > > __u32 comp_size; > > > __u32 frame_size; > > > __u32 frame_headroom; > > > + __u32 flags; > > > > And the flags addition here, unfortunately, requires symbol versioning > > of xsk_umem__create(). That'll be the first in libbpf! :-) > > xsk_umem_config is passed by pointer to xsk_umem__create(), so this > doesn't break ABI, does it? > Old application, dynamically linked to new libbpf.so will crash, right? Old application passes old version of xsk_umem_config, and new library accesses (non-existing) flag struct member. Björn > > > > > > Björn > > > > > }; > > > > > > /* Flags for the libbpf_flags field. */ > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > Intel-wired-lan mailing list > > > Intel-wired-lan@osuosl.org > > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Thu, Aug 1, 2019 at 12:34 AM Björn Töpel <bjorn.topel@gmail.com> wrote: > > On Thu, 1 Aug 2019 at 08:59, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Jul 31, 2019 at 8:21 AM Björn Töpel <bjorn.topel@gmail.com> wrote: > > > > > > On Tue, 30 Jul 2019 at 19:43, Kevin Laatz <kevin.laatz@intel.com> wrote: > > > > > > > > This patch adds a 'flags' field to the umem_config and umem_reg structs. > > > > This will allow for more options to be added for configuring umems. > > > > > > > > The first use for the flags field is to add a flag for unaligned chunks > > > > mode. These flags can either be user-provided or filled with a default. > > > > > > > > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> > > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > > > > > > > --- > > > > v2: > > > > - Removed the headroom check from this patch. It has moved to the > > > > previous patch. > > > > > > > > v4: > > > > - modified chunk flag define > > > > --- > > > > [...] > > > > > > diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h > > > > index 833a6e60d065..44a03d8c34b9 100644 > > > > --- a/tools/lib/bpf/xsk.h > > > > +++ b/tools/lib/bpf/xsk.h > > > > @@ -170,12 +170,14 @@ LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk); > > > > #define XSK_UMEM__DEFAULT_FRAME_SHIFT 12 /* 4096 bytes */ > > > > #define XSK_UMEM__DEFAULT_FRAME_SIZE (1 << XSK_UMEM__DEFAULT_FRAME_SHIFT) > > > > #define XSK_UMEM__DEFAULT_FRAME_HEADROOM 0 > > > > +#define XSK_UMEM__DEFAULT_FLAGS 0 > > > > > > > > struct xsk_umem_config { > > > > __u32 fill_size; > > > > __u32 comp_size; > > > > __u32 frame_size; > > > > __u32 frame_headroom; > > > > + __u32 flags; > > > > > > And the flags addition here, unfortunately, requires symbol versioning > > > of xsk_umem__create(). That'll be the first in libbpf! :-) > > > > xsk_umem_config is passed by pointer to xsk_umem__create(), so this > > doesn't break ABI, does it? > > > > Old application, dynamically linked to new libbpf.so will crash, > right? Old application passes old version of xsk_umem_config, and new > library accesses (non-existing) flag struct member. I think we have similar problems for all the _xattr type of commands (as well some of btf stuff accepting extra opts structs). How is this problem solved in general? Do we version same function multiple times, one for each added field? It feels like there should be some better way to handle this... > > > Björn > > > > > > > > > > > Björn > > > > > > > }; > > > > > > > > /* Flags for the libbpf_flags field. */ > > > > -- > > > > 2.17.1 > > > > > > > > _______________________________________________ > > > > Intel-wired-lan mailing list > > > > Intel-wired-lan@osuosl.org > > > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Fri, 2 Aug 2019 at 09:19, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Aug 1, 2019 at 12:34 AM Björn Töpel <bjorn.topel@gmail.com> wrote: > > [...] > > > > Old application, dynamically linked to new libbpf.so will crash, > > right? Old application passes old version of xsk_umem_config, and new > > library accesses (non-existing) flag struct member. > > I think we have similar problems for all the _xattr type of commands > (as well some of btf stuff accepting extra opts structs). How is this > problem solved in general? Do we version same function multiple times, > one for each added field? It feels like there should be some better > way to handle this... > If the size of the struct was passed as an argument (and extra care is taken when adding members to the struct), it could be handled w/o versioning... but that's not the case here. :-( Versioning is a mess to deal with, so I'd be happy if it could be avoided...
diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h index faaa5ca2a117..a691802d7915 100644 --- a/tools/include/uapi/linux/if_xdp.h +++ b/tools/include/uapi/linux/if_xdp.h @@ -17,6 +17,10 @@ #define XDP_COPY (1 << 1) /* Force copy-mode */ #define XDP_ZEROCOPY (1 << 2) /* Force zero-copy mode */ +/* Flags for xsk_umem_config flags */ +#define XDP_UMEM_UNALIGNED_CHUNK_FLAG_SHIFT 15 +#define XDP_UMEM_UNALIGNED_CHUNK_FLAG (1 << XDP_UMEM_UNALIGNED_CHUNK_FLAG_SHIFT) + struct sockaddr_xdp { __u16 sxdp_family; __u16 sxdp_flags; @@ -49,8 +53,9 @@ struct xdp_mmap_offsets { #define XDP_OPTIONS 8 struct xdp_umem_reg { - __u64 addr; /* Start of packet data area */ - __u64 len; /* Length of packet data area */ + __u64 addr; /* Start of packet data area */ + __u64 len:48; /* Length of packet data area */ + __u64 flags:16; /* Flags for umem */ __u32 chunk_size; __u32 headroom; }; diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index 5007b5d4fd2c..5e7e4d420ee0 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -116,6 +116,7 @@ static void xsk_set_umem_config(struct xsk_umem_config *cfg, cfg->comp_size = XSK_RING_CONS__DEFAULT_NUM_DESCS; cfg->frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE; cfg->frame_headroom = XSK_UMEM__DEFAULT_FRAME_HEADROOM; + cfg->flags = XSK_UMEM__DEFAULT_FLAGS; return; } @@ -123,6 +124,7 @@ static void xsk_set_umem_config(struct xsk_umem_config *cfg, cfg->comp_size = usr_cfg->comp_size; cfg->frame_size = usr_cfg->frame_size; cfg->frame_headroom = usr_cfg->frame_headroom; + cfg->flags = usr_cfg->flags; } static int xsk_set_xdp_socket_config(struct xsk_socket_config *cfg, @@ -182,6 +184,7 @@ int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size, mr.len = size; mr.chunk_size = umem->config.frame_size; mr.headroom = umem->config.frame_headroom; + mr.flags = umem->config.flags; err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_REG, &mr, sizeof(mr)); if (err) { diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h index 833a6e60d065..44a03d8c34b9 100644 --- a/tools/lib/bpf/xsk.h +++ b/tools/lib/bpf/xsk.h @@ -170,12 +170,14 @@ LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk); #define XSK_UMEM__DEFAULT_FRAME_SHIFT 12 /* 4096 bytes */ #define XSK_UMEM__DEFAULT_FRAME_SIZE (1 << XSK_UMEM__DEFAULT_FRAME_SHIFT) #define XSK_UMEM__DEFAULT_FRAME_HEADROOM 0 +#define XSK_UMEM__DEFAULT_FLAGS 0 struct xsk_umem_config { __u32 fill_size; __u32 comp_size; __u32 frame_size; __u32 frame_headroom; + __u32 flags; }; /* Flags for the libbpf_flags field. */