| Message ID | 167906359575.2706833.545256364239637451.stgit@firesoul (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | XDP-hints kfuncs for Intel driver igc | expand |
On 03/17, Jesper Dangaard Brouer wrote: > When driver doesn't implement a bpf_xdp_metadata kfunc the fallback > implementation returns EOPNOTSUPP, which indicate device driver doesn't > implement this kfunc. > Currently many drivers also return EOPNOTSUPP when the hint isn't > available, which is inconsistent from an API point of view. Instead > change drivers to return ENODATA in these cases. > There can be natural cases why a driver doesn't provide any hardware > info for a specific hint, even on a frame to frame basis (e.g. PTP). > Lets keep these cases as separate return codes. > When describing the return values, adjust the function kernel-doc layout > to get proper rendering for the return values. > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> I don't remember whether the previous discussion ended in something? IIRC Martin was preferring to use xdp-features for this instead? Personally I'm fine with having this convention, but I'm not sure how well we'll be able to enforce them. (In general, I'm not a fan of userspace changing it's behavior based on errno. If it's mostly for debugging/development - seems ok) > --- > Documentation/networking/xdp-rx-metadata.rst | 7 +++++-- > drivers/net/ethernet/mellanox/mlx4/en_rx.c | 4 ++-- > drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 ++-- > drivers/net/veth.c | 4 ++-- > net/core/xdp.c | 10 ++++++++-- > 5 files changed, 19 insertions(+), 10 deletions(-) > diff --git a/Documentation/networking/xdp-rx-metadata.rst > b/Documentation/networking/xdp-rx-metadata.rst > index aac63fc2d08b..25ce72af81c2 100644 > --- a/Documentation/networking/xdp-rx-metadata.rst > +++ b/Documentation/networking/xdp-rx-metadata.rst > @@ -23,10 +23,13 @@ metadata is supported, this set will grow: > An XDP program can use these kfuncs to read the metadata into stack > variables for its own consumption. Or, to pass the metadata on to other > consumers, an XDP program can store it into the metadata area carried > -ahead of the packet. > +ahead of the packet. Not all packets will necessary have the requested > +metadata available in which case the driver returns ``-ENODATA``. > Not all kfuncs have to be implemented by the device driver; when not > -implemented, the default ones that return ``-EOPNOTSUPP`` will be used. > +implemented, the default ones that return ``-EOPNOTSUPP`` will be used > +to indicate the device driver have not implemented this kfunc. > + > Within an XDP frame, the metadata layout (accessed via ``xdp_buff``) is > as follows:: > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > index 0869d4fff17b..4b5e459b6d49 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > @@ -674,7 +674,7 @@ int mlx4_en_xdp_rx_timestamp(const struct xdp_md > *ctx, u64 *timestamp) > struct mlx4_en_xdp_buff *_ctx = (void *)ctx; > if (unlikely(_ctx->ring->hwtstamp_rx_filter != HWTSTAMP_FILTER_ALL)) > - return -EOPNOTSUPP; > + return -ENODATA; > *timestamp = mlx4_en_get_hwtstamp(_ctx->mdev, > mlx4_en_get_cqe_ts(_ctx->cqe)); > @@ -686,7 +686,7 @@ int mlx4_en_xdp_rx_hash(const struct xdp_md *ctx, u32 > *hash) > struct mlx4_en_xdp_buff *_ctx = (void *)ctx; > if (unlikely(!(_ctx->dev->features & NETIF_F_RXHASH))) > - return -EOPNOTSUPP; > + return -ENODATA; > *hash = be32_to_cpu(_ctx->cqe->immed_rss_invalid); > return 0; > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > index bcd6370de440..c5dae48b7932 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > @@ -162,7 +162,7 @@ static int mlx5e_xdp_rx_timestamp(const struct xdp_md > *ctx, u64 *timestamp) > const struct mlx5e_xdp_buff *_ctx = (void *)ctx; > if (unlikely(!mlx5e_rx_hw_stamp(_ctx->rq->tstamp))) > - return -EOPNOTSUPP; > + return -ENODATA; > *timestamp = mlx5e_cqe_ts_to_ns(_ctx->rq->ptp_cyc2time, > _ctx->rq->clock, get_cqe_ts(_ctx->cqe)); > @@ -174,7 +174,7 @@ static int mlx5e_xdp_rx_hash(const struct xdp_md > *ctx, u32 *hash) > const struct mlx5e_xdp_buff *_ctx = (void *)ctx; > if (unlikely(!(_ctx->xdp.rxq->dev->features & NETIF_F_RXHASH))) > - return -EOPNOTSUPP; > + return -ENODATA; > *hash = be32_to_cpu(_ctx->cqe->rss_hash_result); > return 0; > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 1bb54de7124d..046461ee42ea 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -1610,7 +1610,7 @@ static int veth_xdp_rx_timestamp(const struct > xdp_md *ctx, u64 *timestamp) > struct veth_xdp_buff *_ctx = (void *)ctx; > if (!_ctx->skb) > - return -EOPNOTSUPP; > + return -ENODATA; > *timestamp = skb_hwtstamps(_ctx->skb)->hwtstamp; > return 0; > @@ -1621,7 +1621,7 @@ static int veth_xdp_rx_hash(const struct xdp_md > *ctx, u32 *hash) > struct veth_xdp_buff *_ctx = (void *)ctx; > if (!_ctx->skb) > - return -EOPNOTSUPP; > + return -ENODATA; > *hash = skb_get_hash(_ctx->skb); > return 0; > diff --git a/net/core/xdp.c b/net/core/xdp.c > index 8d3ad315f18d..7133017bcd74 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -705,7 +705,10 @@ __diag_ignore_all("-Wmissing-prototypes", > * @ctx: XDP context pointer. > * @timestamp: Return value pointer. > * > - * Returns 0 on success or ``-errno`` on error. > + * Return: > + * * Returns 0 on success or ``-errno`` on error. > + * * ``-EOPNOTSUPP`` : means device driver does not implement kfunc > + * * ``-ENODATA`` : means no RX-timestamp available for this frame > */ > __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, > u64 *timestamp) > { > @@ -717,7 +720,10 @@ __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const > struct xdp_md *ctx, u64 *tim > * @ctx: XDP context pointer. > * @hash: Return value pointer. > * > - * Returns 0 on success or ``-errno`` on error. > + * Return: > + * * Returns 0 on success or ``-errno`` on error. > + * * ``-EOPNOTSUPP`` : means device driver doesn't implement kfunc > + * * ``-ENODATA`` : means no RX-hash available for this frame > */ > __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 > *hash) > {
On 17/03/2023 22.21, Stanislav Fomichev wrote: > On 03/17, Jesper Dangaard Brouer wrote: >> When driver doesn't implement a bpf_xdp_metadata kfunc the fallback >> implementation returns EOPNOTSUPP, which indicate device driver doesn't >> implement this kfunc. > >> Currently many drivers also return EOPNOTSUPP when the hint isn't >> available, which is inconsistent from an API point of view. Instead >> change drivers to return ENODATA in these cases. > >> There can be natural cases why a driver doesn't provide any hardware >> info for a specific hint, even on a frame to frame basis (e.g. PTP). >> Lets keep these cases as separate return codes. > >> When describing the return values, adjust the function kernel-doc layout >> to get proper rendering for the return values. > >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > I don't remember whether the previous discussion ended in something? > IIRC Martin was preferring to use xdp-features for this instead? > IIRC Martin asked for a second vote/opinion to settle the vote. The xdp-features use is orthogonal and this patch does not prohibit the later implementation of xdp-features, to detect if driver doesn't implement kfuncs via using global vars. Not applying this patch leaves the API in an strange inconsistent state, because of an argument that in the *future* we can use xdp-features to solve *one* of the discussed use-cases for another return code. I argued for a practical PTP use-case where not all frames contain the PTP timestamp. This patch solve this use-case *now*, so I don't see why we should stall solving this, because of a "future" feature we might never get around to implement, which require the user to use global vars. > Personally I'm fine with having this convention, but I'm not sure how well > we'll be able to enforce them. (In general, I'm not a fan of userspace > changing it's behavior based on errno. If it's mostly for > debugging/development - seems ok) > We enforce the API by documenting the return behavior, like below. If a driver violate this, then we will fix the driver code with a fixes tag. My ask is simply let not have ambiguous return codes. >> --- >> Documentation/networking/xdp-rx-metadata.rst | 7 +++++-- >> drivers/net/ethernet/mellanox/mlx4/en_rx.c | 4 ++-- >> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 ++-- >> drivers/net/veth.c | 4 ++-- >> net/core/xdp.c | 10 ++++++++-- >> 5 files changed, 19 insertions(+), 10 deletions(-) > [...] >> diff --git a/net/core/xdp.c b/net/core/xdp.c >> index 8d3ad315f18d..7133017bcd74 100644 >> --- a/net/core/xdp.c >> +++ b/net/core/xdp.c >> @@ -705,7 +705,10 @@ __diag_ignore_all("-Wmissing-prototypes", >> * @ctx: XDP context pointer. >> * @timestamp: Return value pointer. >> * >> - * Returns 0 on success or ``-errno`` on error. >> + * Return: >> + * * Returns 0 on success or ``-errno`` on error. >> + * * ``-EOPNOTSUPP`` : means device driver does not implement kfunc >> + * * ``-ENODATA`` : means no RX-timestamp available for this frame >> */ >> __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md >> *ctx, u64 *timestamp) >> { >> @@ -717,7 +720,10 @@ __bpf_kfunc int >> bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *tim >> * @ctx: XDP context pointer. >> * @hash: Return value pointer. >> * >> - * Returns 0 on success or ``-errno`` on error. >> + * Return: >> + * * Returns 0 on success or ``-errno`` on error. >> + * * ``-EOPNOTSUPP`` : means device driver doesn't implement kfunc >> + * * ``-ENODATA`` : means no RX-hash available for this frame >> */ >> __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, >> u32 *hash) >> { > >
Jesper Dangaard Brouer <jbrouer@redhat.com> writes: > On 17/03/2023 22.21, Stanislav Fomichev wrote: >> On 03/17, Jesper Dangaard Brouer wrote: >>> When driver doesn't implement a bpf_xdp_metadata kfunc the fallback >>> implementation returns EOPNOTSUPP, which indicate device driver doesn't >>> implement this kfunc. >> >>> Currently many drivers also return EOPNOTSUPP when the hint isn't >>> available, which is inconsistent from an API point of view. Instead >>> change drivers to return ENODATA in these cases. >> >>> There can be natural cases why a driver doesn't provide any hardware >>> info for a specific hint, even on a frame to frame basis (e.g. PTP). >>> Lets keep these cases as separate return codes. >> >>> When describing the return values, adjust the function kernel-doc layout >>> to get proper rendering for the return values. >> >>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >> >> I don't remember whether the previous discussion ended in something? >> IIRC Martin was preferring to use xdp-features for this instead? >> > > IIRC Martin asked for a second vote/opinion to settle the vote. > The xdp-features use is orthogonal and this patch does not prohibit the > later implementation of xdp-features, to detect if driver doesn't > implement kfuncs via using global vars. Not applying this patch leaves > the API in an strange inconsistent state, because of an argument that in > the *future* we can use xdp-features to solve *one* of the discussed > use-cases for another return code. > I argued for a practical PTP use-case where not all frames contain the > PTP timestamp. This patch solve this use-case *now*, so I don't see why > we should stall solving this, because of a "future" feature we might > never get around to implement, which require the user to use global vars. > > >> Personally I'm fine with having this convention, but I'm not sure how well >> we'll be able to enforce them. (In general, I'm not a fan of userspace >> changing it's behavior based on errno. If it's mostly for >> debugging/development - seems ok) >> > > We enforce the API by documenting the return behavior, like below. If a > driver violate this, then we will fix the driver code with a fixes tag. > > My ask is simply let not have ambiguous return codes. FWIW I don't get the opposition to this patch: having distinct return codes strictly increases the amount of information that is available to the caller. Even if some driver happens to use the "wrong" return code, it's still an improvement for all the drivers that do the right thing (and, well, we can fix broken drivers). And if a BPF program doesn't care about the type of failure they can just ignore treat all error codes the same; realistically, that is what most programs will do, but that doesn't mean we can't provide the more-granular error codes to the programs that do care. My only concern with this patch is that it targets bpf-next and carries no Fixes tag, so we'll end up with a kernel release that doesn't have this change... -Toke
On 21/03/2023 13.24, Toke Høiland-Jørgensen wrote: > Jesper Dangaard Brouer <jbrouer@redhat.com> writes: > >> On 17/03/2023 22.21, Stanislav Fomichev wrote: >>> On 03/17, Jesper Dangaard Brouer wrote: >>>> When driver doesn't implement a bpf_xdp_metadata kfunc the fallback >>>> implementation returns EOPNOTSUPP, which indicate device driver doesn't >>>> implement this kfunc. >>> >>>> Currently many drivers also return EOPNOTSUPP when the hint isn't >>>> available, which is inconsistent from an API point of view. Instead >>>> change drivers to return ENODATA in these cases. >>> >>>> There can be natural cases why a driver doesn't provide any hardware >>>> info for a specific hint, even on a frame to frame basis (e.g. PTP). >>>> Lets keep these cases as separate return codes. >>> >>>> When describing the return values, adjust the function kernel-doc layout >>>> to get proper rendering for the return values. >>> >>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >>> >>> I don't remember whether the previous discussion ended in something? >>> IIRC Martin was preferring to use xdp-features for this instead? >>> >> >> IIRC Martin asked for a second vote/opinion to settle the vote. >> The xdp-features use is orthogonal and this patch does not prohibit the >> later implementation of xdp-features, to detect if driver doesn't >> implement kfuncs via using global vars. Not applying this patch leaves >> the API in an strange inconsistent state, because of an argument that in >> the *future* we can use xdp-features to solve *one* of the discussed >> use-cases for another return code. >> I argued for a practical PTP use-case where not all frames contain the >> PTP timestamp. This patch solve this use-case *now*, so I don't see why >> we should stall solving this, because of a "future" feature we might >> never get around to implement, which require the user to use global vars. >> >> >>> Personally I'm fine with having this convention, but I'm not sure how well >>> we'll be able to enforce them. (In general, I'm not a fan of userspace >>> changing it's behavior based on errno. If it's mostly for >>> debugging/development - seems ok) >>> >> >> We enforce the API by documenting the return behavior, like below. If a >> driver violate this, then we will fix the driver code with a fixes tag. >> >> My ask is simply let not have ambiguous return codes. > > FWIW I don't get the opposition to this patch: having distinct return > codes strictly increases the amount of information that is available to > the caller. Even if some driver happens to use the "wrong" return code, > it's still an improvement for all the drivers that do the right thing > (and, well, we can fix broken drivers). And if a BPF program doesn't > care about the type of failure they can just ignore treat all error > codes the same; realistically, that is what most programs will do, but > that doesn't mean we can't provide the more-granular error codes to the > programs that do care. > > My only concern with this patch is that it targets bpf-next and carries > no Fixes tag, so we'll end up with a kernel release that doesn't have > this change... > Good point, I'll send this patch against 'bpf' tree instead. --Jesper
diff --git a/Documentation/networking/xdp-rx-metadata.rst b/Documentation/networking/xdp-rx-metadata.rst index aac63fc2d08b..25ce72af81c2 100644 --- a/Documentation/networking/xdp-rx-metadata.rst +++ b/Documentation/networking/xdp-rx-metadata.rst @@ -23,10 +23,13 @@ metadata is supported, this set will grow: An XDP program can use these kfuncs to read the metadata into stack variables for its own consumption. Or, to pass the metadata on to other consumers, an XDP program can store it into the metadata area carried -ahead of the packet. +ahead of the packet. Not all packets will necessary have the requested +metadata available in which case the driver returns ``-ENODATA``. Not all kfuncs have to be implemented by the device driver; when not -implemented, the default ones that return ``-EOPNOTSUPP`` will be used. +implemented, the default ones that return ``-EOPNOTSUPP`` will be used +to indicate the device driver have not implemented this kfunc. + Within an XDP frame, the metadata layout (accessed via ``xdp_buff``) is as follows:: diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c index 0869d4fff17b..4b5e459b6d49 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c @@ -674,7 +674,7 @@ int mlx4_en_xdp_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp) struct mlx4_en_xdp_buff *_ctx = (void *)ctx; if (unlikely(_ctx->ring->hwtstamp_rx_filter != HWTSTAMP_FILTER_ALL)) - return -EOPNOTSUPP; + return -ENODATA; *timestamp = mlx4_en_get_hwtstamp(_ctx->mdev, mlx4_en_get_cqe_ts(_ctx->cqe)); @@ -686,7 +686,7 @@ int mlx4_en_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash) struct mlx4_en_xdp_buff *_ctx = (void *)ctx; if (unlikely(!(_ctx->dev->features & NETIF_F_RXHASH))) - return -EOPNOTSUPP; + return -ENODATA; *hash = be32_to_cpu(_ctx->cqe->immed_rss_invalid); return 0; diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c index bcd6370de440..c5dae48b7932 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c @@ -162,7 +162,7 @@ static int mlx5e_xdp_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp) const struct mlx5e_xdp_buff *_ctx = (void *)ctx; if (unlikely(!mlx5e_rx_hw_stamp(_ctx->rq->tstamp))) - return -EOPNOTSUPP; + return -ENODATA; *timestamp = mlx5e_cqe_ts_to_ns(_ctx->rq->ptp_cyc2time, _ctx->rq->clock, get_cqe_ts(_ctx->cqe)); @@ -174,7 +174,7 @@ static int mlx5e_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash) const struct mlx5e_xdp_buff *_ctx = (void *)ctx; if (unlikely(!(_ctx->xdp.rxq->dev->features & NETIF_F_RXHASH))) - return -EOPNOTSUPP; + return -ENODATA; *hash = be32_to_cpu(_ctx->cqe->rss_hash_result); return 0; diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 1bb54de7124d..046461ee42ea 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1610,7 +1610,7 @@ static int veth_xdp_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp) struct veth_xdp_buff *_ctx = (void *)ctx; if (!_ctx->skb) - return -EOPNOTSUPP; + return -ENODATA; *timestamp = skb_hwtstamps(_ctx->skb)->hwtstamp; return 0; @@ -1621,7 +1621,7 @@ static int veth_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash) struct veth_xdp_buff *_ctx = (void *)ctx; if (!_ctx->skb) - return -EOPNOTSUPP; + return -ENODATA; *hash = skb_get_hash(_ctx->skb); return 0; diff --git a/net/core/xdp.c b/net/core/xdp.c index 8d3ad315f18d..7133017bcd74 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -705,7 +705,10 @@ __diag_ignore_all("-Wmissing-prototypes", * @ctx: XDP context pointer. * @timestamp: Return value pointer. * - * Returns 0 on success or ``-errno`` on error. + * Return: + * * Returns 0 on success or ``-errno`` on error. + * * ``-EOPNOTSUPP`` : means device driver does not implement kfunc + * * ``-ENODATA`` : means no RX-timestamp available for this frame */ __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp) { @@ -717,7 +720,10 @@ __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *tim * @ctx: XDP context pointer. * @hash: Return value pointer. * - * Returns 0 on success or ``-errno`` on error. + * Return: + * * Returns 0 on success or ``-errno`` on error. + * * ``-EOPNOTSUPP`` : means device driver doesn't implement kfunc + * * ``-ENODATA`` : means no RX-hash available for this frame */ __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash) {
When driver doesn't implement a bpf_xdp_metadata kfunc the fallback implementation returns EOPNOTSUPP, which indicate device driver doesn't implement this kfunc. Currently many drivers also return EOPNOTSUPP when the hint isn't available, which is inconsistent from an API point of view. Instead change drivers to return ENODATA in these cases. There can be natural cases why a driver doesn't provide any hardware info for a specific hint, even on a frame to frame basis (e.g. PTP). Lets keep these cases as separate return codes. When describing the return values, adjust the function kernel-doc layout to get proper rendering for the return values. Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- Documentation/networking/xdp-rx-metadata.rst | 7 +++++-- drivers/net/ethernet/mellanox/mlx4/en_rx.c | 4 ++-- drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 ++-- drivers/net/veth.c | 4 ++-- net/core/xdp.c | 10 ++++++++-- 5 files changed, 19 insertions(+), 10 deletions(-)