| Message ID | 20240404123717.11857-4-aaptel@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | nvme-tcp receive offloads | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On 04/04/2024 15:37, Aurelien Aptel wrote: > From: Ben Ben-Ishay <benishay@nvidia.com> > > When using direct data placement (DDP) the NIC could write the payload > directly into the destination buffer and constructs SKBs such that > they point to this data. To skip copies when SKB data already resides > in the destination buffer we check if (src == dst), and skip the copy > when it's true. > > Signed-off-by: Ben Ben-Ishay <benishay@nvidia.com> > Signed-off-by: Boris Pismenny <borisp@nvidia.com> > Signed-off-by: Or Gerlitz <ogerlitz@nvidia.com> > Signed-off-by: Yoray Zack <yorayz@nvidia.com> > Signed-off-by: Shai Malin <smalin@nvidia.com> > Signed-off-by: Aurelien Aptel <aaptel@nvidia.com> > Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > lib/iov_iter.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/lib/iov_iter.c b/lib/iov_iter.c > index 4a6a9f419bd7..a85125485174 100644 > --- a/lib/iov_iter.c > +++ b/lib/iov_iter.c > @@ -62,7 +62,14 @@ static __always_inline > size_t memcpy_to_iter(void *iter_to, size_t progress, > size_t len, void *from, void *priv2) > { > - memcpy(iter_to, from + progress, len); > + /* > + * When using direct data placement (DDP) the hardware writes > + * data directly to the destination buffer, and constructs > + * IOVs such that they point to this data. > + * Thus, when the src == dst we skip the memcpy. > + */ > + if (iter_to != from + progress) > + memcpy(iter_to, from + progress, len); > return 0; > } > Looks good, Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
From: Aurelien Aptel > Sent: 04 April 2024 13:37 > > From: Ben Ben-Ishay <benishay@nvidia.com> > > When using direct data placement (DDP) the NIC could write the payload > directly into the destination buffer and constructs SKBs such that > they point to this data. To skip copies when SKB data already resides > in the destination buffer we check if (src == dst), and skip the copy > when it's true. ... > diff --git a/lib/iov_iter.c b/lib/iov_iter.c > index 4a6a9f419bd7..a85125485174 100644 > --- a/lib/iov_iter.c > +++ b/lib/iov_iter.c > @@ -62,7 +62,14 @@ static __always_inline > size_t memcpy_to_iter(void *iter_to, size_t progress, > size_t len, void *from, void *priv2) > { > - memcpy(iter_to, from + progress, len); > + /* > + * When using direct data placement (DDP) the hardware writes > + * data directly to the destination buffer, and constructs > + * IOVs such that they point to this data. > + * Thus, when the src == dst we skip the memcpy. > + */ > + if (iter_to != from + progress) > + memcpy(iter_to, from + progress, len); How must does this conditional cost for the normal case when it is true? I suspect it is mispredicted 50% of the time. So, while it may speed up your test, the overall system impact will be negative. David > return 0; > } > > -- > 2.34.1 > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, David Laight <David.Laight@ACULAB.COM> writes: > How must does this conditional cost for the normal case > when it is true? > I suspect it is mispredicted 50% of the time. > So, while it may speed up your test, the overall system > impact will be negative. We have done some measures to evaluate the cost of the additional test and we don't see any noticeable impact on performances. If this is still a concern, we can add compilation-time check on CONFIG_ULP_DDP i.e. ifdef or IS_ENABLED() (which gets optimized out). Thanks
diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 4a6a9f419bd7..a85125485174 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -62,7 +62,14 @@ static __always_inline size_t memcpy_to_iter(void *iter_to, size_t progress, size_t len, void *from, void *priv2) { - memcpy(iter_to, from + progress, len); + /* + * When using direct data placement (DDP) the hardware writes + * data directly to the destination buffer, and constructs + * IOVs such that they point to this data. + * Thus, when the src == dst we skip the memcpy. + */ + if (iter_to != from + progress) + memcpy(iter_to, from + progress, len); return 0; }