Message-ID: <521265b1-1a32-4d5e-9348-77559a5a0af4@redhat.com>
Date: Mon, 5 May 2025 12:20:18 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: John Ousterhout <ouster@...stanford.edu>, netdev@...r.kernel.org
Cc: edumazet@...gle.com, horms@...nel.org, kuba@...nel.org
Subject: Re: [PATCH net-next v8 03/15] net: homa: create shared Homa header
 files

On 5/3/25 1:37 AM, John Ousterhout wrote:
[...]
> +/* Forward declarations. */
> +struct homa;
> +struct homa_peer;
> +struct homa_rpc;
> +struct homa_sock;
> +
> +#define sizeof32(type) ((int)(sizeof(type)))

(u32) instead of (int). I think you should try to avoid using this
define, which is not very nice by itself.

> +
> +#ifdef __CHECKER__
> +#define __context__(x, y, z) __attribute__((context(x, y, z)))
> +#else
> +#define __context__(...)
> +#endif /* __CHECKER__ */

Why do you need to fiddle with the sparse annotation? Very likely this
should be dropped.

[...]
> +/**
> + * homa_get_skb_info() - Return the address of Homa's private information
> + * for an sk_buff.
> + * @skb:     Socket buffer whose info is needed.
> + * Return: address of Homa's private information for @skb.
> + */
> +static inline struct homa_skb_info *homa_get_skb_info(struct sk_buff *skb)
> +{
> +	return (struct homa_skb_info *)(skb_end_pointer(skb)) - 1;

This looks fragile. Why can't you use the skb control buffer here?

> +}
> +
> +/**
> + * homa_set_doff() - Fills in the doff TCP header field for a Homa packet.
> + * @h:     Packet header whose doff field is to be set.
> + * @size:  Size of the "header", bytes (must be a multiple of 4). This
> + *         information is used only for TSO; it's the number of bytes
> + *         that should be replicated in each segment. The bytes after
> + *         this will be distributed among segments.
> + */
> +static inline void homa_set_doff(struct homa_data_hdr *h, int size)
> +{
> +	/* Drop the 2 low-order bits from size and set the 4 high-order
> +	 * bits of doff from what's left.
> +	 */
> +	h->common.doff = size << 2;
> +}
> +
> +/** skb_is_ipv6() - Return true if the packet is encapsulated with IPv6,
> + *  false otherwise (presumably it's IPv4).
> + */
> +static inline bool skb_is_ipv6(const struct sk_buff *skb)
> +{
> +	return ipv6_hdr(skb)->version == 6;
> +}
> +
> +/**
> + * ipv6_to_ipv4() - Given an IPv6 address produced by ipv4_to_ipv6, return
> + * the original IPv4 address (in network byte order).
> + * @ip6:  IPv6 address; assumed to be a mapped IPv4 address.
> + * Return: IPv4 address stored in @ip6.
> + */
> +static inline __be32 ipv6_to_ipv4(const struct in6_addr ip6)
> +{
> +	return ip6.in6_u.u6_addr32[3];
> +}
> +
> +/**
> + * canonical_ipv6_addr() - Convert a socket address to the "standard"
> + * form used in Homa, which is always an IPv6 address; if the original address
> + * was IPv4, convert it to an IPv4-mapped IPv6 address.
> + * @addr:   Address to canonicalize (if NULL, "any" is returned).
> + * Return: IPv6 address corresponding to @addr.
> + */
> +static inline struct in6_addr canonical_ipv6_addr(const union sockaddr_in_union
> +						  *addr)
> +{
> +	struct in6_addr mapped;
> +
> +	if (addr) {
> +		if (addr->sa.sa_family == AF_INET6)
> +			return addr->in6.sin6_addr;
> +		ipv6_addr_set_v4mapped(addr->in4.sin_addr.s_addr, &mapped);
> +		return mapped;
> +	}
> +	return in6addr_any;
> +}
> +
> +/**
> + * skb_canonical_ipv6_saddr() - Given a packet buffer, return its source
> + * address in the "standard" form used in Homa, which is always an IPv6
> + * address; if the original address was IPv4, convert it to an IPv4-mapped
> + * IPv6 address.
> + * @skb:   The source address will be extracted from this packet buffer.
> + * Return: IPv6 address for @skb's source machine.
> + */
> +static inline struct in6_addr skb_canonical_ipv6_saddr(struct sk_buff *skb)
> +{
> +	struct in6_addr mapped;
> +
> +	if (skb_is_ipv6(skb))
> +		return ipv6_hdr(skb)->saddr;
> +	ipv6_addr_set_v4mapped(ip_hdr(skb)->saddr, &mapped);
> +	return mapped;
> +}
> +
> +static inline bool is_homa_pkt(struct sk_buff *skb)
> +{
> +	struct iphdr *iph = ip_hdr(skb);
> +
> +	return (iph->protocol == IPPROTO_HOMA);

What if this is an ipv6 packet? Also I don't see any use of this
function later on.

> +}
> +
> +/**
> + * homa_make_header_avl() - Invokes pskb_may_pull to make sure that all the
> + * Homa header information for a packet is in the linear part of the skb
> + * where it can be addressed using skb_transport_header.
> + * @skb:     Packet for which header is needed.
> + * Return:   The result of pskb_may_pull (true for success)
> + */
> +static inline bool homa_make_header_avl(struct sk_buff *skb)
> +{
> +	int pull_length;
> +
> +	pull_length = skb_transport_header(skb) - skb->data + HOMA_MAX_HEADER;
> +	if (pull_length > skb->len)
> +		pull_length = skb->len;
> +	return pskb_may_pull(skb, pull_length);
> +}
> +
> +#define UNIT_LOG(...)
> +#define UNIT_HOOK(...)

It looks like the above 2 define are unused later on.


> +extern unsigned int homa_net_id;
> +
> +void     homa_abort_rpcs(struct homa *homa, const struct in6_addr *addr,
> +			 int port, int error);
> +void     homa_abort_sock_rpcs(struct homa_sock *hsk, int error);
> +void     homa_ack_pkt(struct sk_buff *skb, struct homa_sock *hsk,
> +		      struct homa_rpc *rpc);
> +void     homa_add_packet(struct homa_rpc *rpc, struct sk_buff *skb);
> +int      homa_backlog_rcv(struct sock *sk, struct sk_buff *skb);
> +int      homa_bind(struct socket *sk, struct sockaddr *addr,
> +		   int addr_len);
> +void     homa_close(struct sock *sock, long timeout);
> +int      homa_copy_to_user(struct homa_rpc *rpc);
> +void     homa_data_pkt(struct sk_buff *skb, struct homa_rpc *rpc);
> +void     homa_destroy(struct homa *homa);
> +int      homa_disconnect(struct sock *sk, int flags);
> +void     homa_dispatch_pkts(struct sk_buff *skb, struct homa *homa);
> +int      homa_err_handler_v4(struct sk_buff *skb, u32 info);
> +int      homa_err_handler_v6(struct sk_buff *skb,
> +			     struct inet6_skb_parm *opt, u8 type,  u8 code,
> +			     int offset, __be32 info);
> +int      homa_fill_data_interleaved(struct homa_rpc *rpc,
> +				    struct sk_buff *skb, struct iov_iter *iter);
> +struct homa_gap *homa_gap_new(struct list_head *next, int start, int end);
> +void     homa_gap_retry(struct homa_rpc *rpc);
> +int      homa_get_port(struct sock *sk, unsigned short snum);
> +int      homa_getsockopt(struct sock *sk, int level, int optname,
> +			 char __user *optval, int __user *optlen);
> +int      homa_hash(struct sock *sk);
> +enum hrtimer_restart homa_hrtimer(struct hrtimer *timer);
> +int      homa_init(struct homa *homa, struct net *net);
> +int      homa_ioctl(struct sock *sk, int cmd, int *karg);
> +int      homa_load(void);
> +int      homa_message_out_fill(struct homa_rpc *rpc,
> +			       struct iov_iter *iter, int xmit);
> +void     homa_message_out_init(struct homa_rpc *rpc, int length);
> +void     homa_need_ack_pkt(struct sk_buff *skb, struct homa_sock *hsk,
> +			   struct homa_rpc *rpc);
> +struct sk_buff *homa_new_data_packet(struct homa_rpc *rpc,
> +				     struct iov_iter *iter, int offset,
> +				     int length, int max_seg_data);
> +int      homa_net_init(struct net *net);
> +void     homa_net_exit(struct net *net);
> +__poll_t homa_poll(struct file *file, struct socket *sock,
> +		   struct poll_table_struct *wait);
> +int      homa_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> +		      int flags, int *addr_len);
> +void     homa_resend_pkt(struct sk_buff *skb, struct homa_rpc *rpc,
> +			 struct homa_sock *hsk);
> +void     homa_rpc_abort(struct homa_rpc *crpc, int error);
> +void     homa_rpc_acked(struct homa_sock *hsk,
> +			const struct in6_addr *saddr, struct homa_ack *ack);
> +void     homa_rpc_end(struct homa_rpc *rpc);
> +void     homa_rpc_handoff(struct homa_rpc *rpc);
> +int      homa_sendmsg(struct sock *sk, struct msghdr *msg, size_t len);
> +int      homa_setsockopt(struct sock *sk, int level, int optname,
> +			 sockptr_t optval, unsigned int optlen);
> +int      homa_shutdown(struct socket *sock, int how);
> +int      homa_softirq(struct sk_buff *skb);
> +void     homa_spin(int ns);
> +void     homa_timer(struct homa *homa);
> +int      homa_timer_main(void *transport);
> +void     homa_unhash(struct sock *sk);
> +void     homa_rpc_unknown_pkt(struct sk_buff *skb, struct homa_rpc *rpc);
> +void     homa_unload(void);
> +int      homa_wait_private(struct homa_rpc *rpc, int nonblocking);
> +struct homa_rpc
> +	*homa_wait_shared(struct homa_sock *hsk, int nonblocking);
> +int      homa_xmit_control(enum homa_packet_type type, void *contents,
> +			   size_t length, struct homa_rpc *rpc);
> +int      __homa_xmit_control(void *contents, size_t length,
> +			     struct homa_peer *peer, struct homa_sock *hsk);
> +void     homa_xmit_data(struct homa_rpc *rpc, bool force);
> +void     homa_xmit_unknown(struct sk_buff *skb, struct homa_sock *hsk);
> +
> +int      homa_message_in_init(struct homa_rpc *rpc, int unsched);
> +void     homa_resend_data(struct homa_rpc *rpc, int start, int end);
> +void     __homa_xmit_data(struct sk_buff *skb, struct homa_rpc *rpc);

You should introduce the declaration of a given function in the same
patch introducing the implementation. That means the patches should be
sorted from the lowest level helper towards the upper layer.

[...]
> +static inline int homa_skb_append_to_frag(struct homa *homa,
> +					  struct sk_buff *skb, void *buf,
> +					  int length)
> +{
> +	char *dst = skb_put(skb, length);
> +
> +	memcpy(dst, buf, length);
> +	return 0;

The name is misleading as it does not append to an skb frag but to the
skb linear part

> +}
> +
> +static inline int  homa_skb_append_from_skb(struct homa *homa,
> +					    struct sk_buff *dst_skb,
> +					    struct sk_buff *src_skb,
> +					    int offset, int length)
> +{
> +	return homa_skb_append_to_frag(homa, dst_skb,
> +			skb_transport_header(src_skb) + offset, length);
> +}
> +
> +static inline void homa_skb_free_tx(struct homa *homa, struct sk_buff *skb)
> +{
> +	kfree_skb(skb);
> +}
> +
> +static inline void homa_skb_free_many_tx(struct homa *homa,
> +					 struct sk_buff **skbs, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++)
> +		kfree_skb(skbs[i]);

'home' is unused here.

> +}
> +
> +static inline void homa_skb_get(struct sk_buff *skb, void *dest, int offset,
> +				int length)
> +{
> +	memcpy(dest, skb_transport_header(skb) + offset, length);
> +}
> +
> +static inline struct sk_buff *homa_skb_new_tx(int length)

please use 'alloc' for allocator.

/P

Powered by blists - more mailing lists