[ovs-dev] 答复: [PATCH v2 1/5] Fix dp_packet_set_size error for multi-seg mbuf

[Yi Yang (杨燚)-云服务集团]

Flavio, thank you so much for reviewing, I'll fix them per your comments in next version.  Some replies per your comments inline.

-----邮件原件-----
发件人: dev [mailto:ovs-dev-bounces at openvswitch.org] 代表 Flavio Leitner
发送时间: 2020年7月11日 3:42
收件人: yang_y_yi at 163.com
抄送: ovs-dev at openvswitch.org
主题: Re: [ovs-dev] [PATCH v2 1/5] Fix dp_packet_set_size error for multi-seg mbuf


Hi Yi,

Thanks for putting this patch-set together.

On Wed, Jul 01, 2020 at 05:15:29PM +0800, yang_y_yi at 163.com wrote:
> From: Yi Yang <yangyi01 at inspur.com>
> 
> For multi-seg mbuf, pkt_len isn't equal to data_len, data_len is 
> data_len of the first seg, pkt_len is sum of data_len of all the segs, 
> so for such packets, dp_packet_set_size shouldn't change data_len.
> 
> Signed-off-by: Yi Yang <yangyi01 at inspur.com>
> ---
>  lib/dp-packet.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 0430cca..070d111 
> 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -575,7 +575,9 @@ dp_packet_set_size(struct dp_packet *b, uint32_t v)
>       * (and thus 'v') will always be <= UINT16_MAX; this means that there is no
>       * loss of accuracy in assigning 'v' to 'data_len'.
>       */
> -    b->mbuf.data_len = (uint16_t)v;  /* Current seg length. */
> +    if (b->mbuf.nb_segs <= 1) {
> +        b->mbuf.data_len = (uint16_t)v;  /* Current seg length. */
> +    }
>      b->mbuf.pkt_len = v;             /* Total length of all segments linked to
>                                        * this segment. */  }

Currently OVS doesn't support multi-seg mbuf, so although this patch wouldn't break anything it doesn't sound correct as it is.  It seems incomplete/limited as well.

I think at least the patch should add a comment explaining why and when that is needed. 

Another thing is that this change alone has no users.  Usually we do changes along with the first user. I am still reviewing the following patches, but I suspect this change is for GRO/GSO, so in my opinion it makes sense to be part of one of them.
Doing so helps to backtrack the reason for a specific change.

I think we should prioritize single mbuf as they carry less data, so OVS has less time to process them. Therefore, it seems appropriate to use OVS_LIKELY().

[Yi Yang] Make sense, I'll merge it in GRO patch, yes, GRO needs it, OVS_LIKELY will be better.