Hi Ying,
Thanks for your comments!
Regarding your last statement, yes when making the patch, I noticed that the
"tipc_msg_build()" and "tipc_msg_fragment()" do a similar task, also I tried to
think a way to combine them but didn't because of the reasons:
1- The "core" functions to copy the data are different since the
"tipc_msg_build()" plays with user data in the iov buffers, whereas, for the
other, it's skb data.
Also, the outputs are different, the first function will set the messages' type
in header such as "FIRST_FRAGMENT", "FRAGMENT" or "LAST_FRAGMENT", but not with
the other because it will overwrite the tunnel messages' type... that I had to
use the other field (fragm_no/nof_fragms) to determine this at the receiving
side...
2- I don't want to touch the old code that can be risky :(
BR/Tuong
-----Original Message-----
From: Ying Xue <ying....@windriver.com>
Sent: Sunday, June 16, 2019 1:42 PM
To: Tuong Lien <tuong.t.l...@dektech.com.au>;
tipc-discussion@lists.sourceforge.net; jon.ma...@ericsson.com; ma...@donjonn.com
Subject: Re: [PATCH RFC 2/2] tipc: fix changeover issues due to large packet
> 2) The same scenario above can happen more easily in case the MTU of
> the links is set differently or when changing. In that case, as long as
> a large message in the failure link's transmq queue was built and
> fragmented with its link's MTU > the other link's one, the issue will
> happen (there is no need of a link synching in advance).
>
> 3) The link synching procedure also faces with the same issue but since
> the link synching is only started upon receipt of a SYNCH_MSG, dropping
> the message will not result in a state deadlock, but it is not expected
> as design.
>
> The 1) & 3) issues are resolved by the previous commit 81e4dd94b214
This is the same as previous commit. The commit ID might be invalid
after it's merged into upstream.
> ("tipc: optimize link synching mechanism") by generating only a dummy
> SYNCH_MSG (i.e. without data) at the link synching, so the size of a
> FAILOVER_MSG if any then will never exceed the link's MTU.
> /**
> + * tipc_msg_fragment - build a fragment skb list for TIPC message
> + *
> + * @skb: TIPC message skb
> + * @hdr: internal msg header to be put on the top of the fragments
> + * @pktmax: max size of a fragment incl. the header
> + * @frags: returned fragment skb list
> + *
> + * Returns 0 if the fragmentation is successful, otherwise: -EINVAL
> + * or -ENOMEM
> + */
> +int tipc_msg_fragment(struct sk_buff *skb, const struct tipc_msg *hdr,
> + int pktmax, struct sk_buff_head *frags)
> +{
> + int pktno, nof_fragms, dsz, dmax, eat;
> + struct tipc_msg *_hdr;
> + struct sk_buff *_skb;
> + u8 *data;
> +
> + /* Non-linear buffer? */
> + if (skb_linearize(skb))
> + return -ENOMEM;
> +
> + data = (u8 *)skb->data;
> + dsz = msg_size(buf_msg(skb));
> + dmax = pktmax - INT_H_SIZE;
> +
> + if (dsz <= dmax || !dmax)
> + return -EINVAL;
> +
> + nof_fragms = dsz / dmax + 1;
> +
> + for (pktno = 1; pktno <= nof_fragms; pktno++) {
> + if (pktno < nof_fragms)
> + eat = dmax;
> + else
> + eat = dsz % dmax;
> +
> + _skb = tipc_buf_acquire(INT_H_SIZE + eat, GFP_ATOMIC);
> + if (!_skb)
> + goto error;
> +
> + skb_orphan(_skb);
> + __skb_queue_tail(frags, _skb);
> +
> + skb_copy_to_linear_data(_skb, hdr, INT_H_SIZE);
> + skb_copy_to_linear_data_offset(_skb, INT_H_SIZE, data, eat);
> + data += eat;
> +
> + _hdr = buf_msg(_skb);
> + msg_set_fragm_no(_hdr, pktno);
> + msg_set_nof_fragms(_hdr, nof_fragms);
> + msg_set_size(_hdr, INT_H_SIZE + eat);
> + }
> + return 0;
> +
In fact we have similar code in tipc_msg_build() where we also fragment
packet if necessary. In order to eliminate redundant code, I suggest we
should extract the common code into a separate function and then
tipc_msg_build() and tipc_msg_fragment() call it.
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
