> >>> @@ -209,23 +218,79 @@ static int tipc_udp_send_msg(struct net *net,
struct sk_buff *skb,
> >>> if (skb_headroom(skb) < UDP_MIN_HEADROOM) {
> >>> err = pskb_expand_head(skb, UDP_MIN_HEADROOM, 0,
GFP_ATOMIC);
> >>> if (err)
> >>> - goto tx_error;
> >>> + goto err_out;
> >>> }
> >>>
> >>> skb_set_inner_protocol(skb, htons(ETH_P_TIPC));
> >>> ub = rcu_dereference_rtnl(b->media_ptr);
> >>> if (!ub) {
> >>> err = -ENODEV;
> >>> - goto tx_error;
> >>> + goto err_out;
> >>> + }
> >>> +
> >>> + /* Replicast, send an skb to each configured IP address */
> >>> + if (unlikely(addr->broadcast)) {
> >>> + bool first = true;
> >>> + struct udp_replicast *rcast;
> >>> +
> >>> + list_for_each_entry_rcu(rcast, &ub->rcast.list, list) {
> >>> + struct sk_buff *_skb;
> >>> +
> >>> + /* Avoid one extra skb copy */
> >>> + if (first) {
> >>> + dst = &rcast->addr;
> >>> + first = false;
> >>> + continue;
> >>> + }
> >>> +
> >>> + _skb = pskb_copy(skb, GFP_ATOMIC);
> >>> + if (!_skb) {
> >>> + err = -ENOMEM;
> >>> + goto err_out;
> >>> + }
> >>> +
> >>> + err = tipc_udp_xmit(net, _skb, ub, src,
&rcast->addr);
> >>> + if (err) {
> >>> + kfree_skb(_skb);
> >>> + goto err_out;
> >>> + }
> >>> + }
> >> Slightly confusing. Why don't you return here? Even when the list is
> >> non-empty you go on and send to the first given address, which may or
> >> may not be a multicast address.
> >> It is probably correct, but the logics is not intuitive. Maybe some
> >> refactoring and comments will help?
> > We avoid an extra skb clone.
> > The logic is as follows.
> >
> > We always have an skb when we come here.
> > * If this is a multicast bearer, we skip the replicast code and send it
> > as it is.
> > * If it's a peer-to-peer bearer we sent it to the first address in the
> > replicast list, i.e. our peer.
> I don't think the concept of peer-to-peer bearer even exists from now
> on. To me it is just a (very unusual) special case of a replicast bearer
> where N == 1, and it deserves no special consideration in the code if we
> do things right.
>
> > * If it's a replicast bearer, we send a skb copy to all but the first
> > peer, which gets the original skb. This way we avoid an extra
> > skb_copy() and skb_free().
> A cloning extra is nothing if you are anyway replicasting to N >> 1
> nodes. Code clarity should be more important.
>
> What about:
>
> if (!addr->broadcast || skb_queue_empty(list))
> return tipc_udp_xmit(net, skb, ub, src, dst);
>
> /* Replicast */
> list_for_each_entry(...) {
> clone;
> xmit;
> }
> skb_free(skb);
>
>
> I think this would be easier to understand. Here I assume that the peer
> list is empty if we have multicast bearer, and that all peer addresses
> only exist in the form of media addresses in the individual links. If
> the peer list for some reason I don't understand cannot be empty, we
> have to find some other way to distinguish between multicast and
> replicast, such as a separate 'replicast' value in tipc_media_address.
>
> >
> > This is an effect of moving the peer-to-peer address
> If the "peer-to-peer" address *is* a multicast address it should go to
> the broadcast field, otherwise it should be added at the first address
> in the list.
>
> BR
> ///jon
>
IMHO, If you cannot quantify the performance gain of adding special
consideration for the N=1 case, it's not needed.
//E
------------------------------------------------------------------------------
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
