On Tue, Jun 13, 2023 at 10:20:38AM -0700, Elliot Berman wrote:

Any reason why these two are not explicitly packed?

"sections" is sufficient.

+	struct gh_rm_mem_append_req_header *req_header;
+	size_t msg_size = 0;
+	void *msg;
+	int ret;
+
+	msg_size += sizeof(struct gh_rm_mem_append_req_header);

msg_size is sizeof(header) + sizeof(sections), but written this way you
state that it's:

msg_size is X + sizeof(header),
and msg_size is also sizeof(sections)


It's the same thing, but you're forcing the reader to look for the
initial value of msg_size.

+	msg_size += struct_size(mem_section, entries, n_mem_entries);
+
+	msg = kzalloc(msg_size, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	req_header = msg;

Can't you just make msg a strcut gh_rm_mem_append_req_header?

+	mem_section = (void *)(req_header + 1);
+
+	req_header->mem_handle = cpu_to_le32(mem_handle);
+	if (end_append)
+		req_header->flags |= GH_MEM_APPEND_REQ_FLAGS_END;
+
+	mem_section->n_entries = cpu_to_le16(n_mem_entries);
+	memcpy(mem_section->entries, mem_entries, sizeof(*mem_entries) * n_mem_entries);
+
+	ret = gh_rm_call(rm, GH_RM_RPC_MEM_APPEND, msg, msg_size, NULL, NULL);
+	kfree(msg);
+
+	return ret;
+}
+
+static int gh_rm_mem_append(struct gh_rm *rm, u32 mem_handle,
+			struct gh_rm_mem_entry *mem_entries, size_t n_mem_entries)

You can name these arguments "entries" and "n_entries" (or num_entries)
without loosing any infromation, but saving a whole bunch of characters
that people have to read.

+{
+	bool end_append;

Iiuc this means "is this the last entry", perhaps calling it "last"
would make its purpose immediately obvious.

+	int ret = 0;
+	size_t n;
+
+	while (n_mem_entries) {
+		if (n_mem_entries > GH_RM_MAX_MEM_ENTRIES) {
+			end_append = false;
+			n = GH_RM_MAX_MEM_ENTRIES;
+		} else {
+			end_append = true;
+			n = n_mem_entries;
+		}
+
+		ret = _gh_rm_mem_append(rm, mem_handle, end_append, mem_entries, n);

Every loop here you kzallc() and kfree() the same chunk of scratch
memory. If you inline _gh_rm_mem_append() here and do the allocation
once based on GH_RM_MAX_MEM_ENTRIES this would be faster and seemingly
easier to read/follow.

+		if (ret)
+			break;
+
+		mem_entries += n;
+		n_mem_entries -= n;
+	}
+
+	return ret;
+}
+
+static int gh_rm_mem_lend_common(struct gh_rm *rm, u32 message_id, struct gh_rm_mem_parcel *p)
+{
+	size_t msg_size = 0, initial_mem_entries = p->n_mem_entries, resp_size;
+	size_t acl_section_size, mem_section_size;

You keep all other variables one per line, and in reverse x-mas style.
Please do the same for this.

Naming these acl_size and mem_size would be beneficial.

+	struct gh_rm_mem_share_req_acl_section *acl_section;
+	struct gh_rm_mem_share_req_mem_section *mem_section;

Naming these acl and mem will make below easier to read.

+	struct gh_rm_mem_share_req_header *req_header;
+	u32 *attr_section;
+	__le32 *resp;
+	void *msg;
+	int ret;
+
+	if (!p->acl_entries || !p->n_acl_entries || !p->mem_entries || !p->n_mem_entries ||
+	    p->n_acl_entries > U8_MAX || p->mem_handle != GH_MEM_HANDLE_INVAL)
+		return -EINVAL;
+
+	if (initial_mem_entries > GH_RM_MAX_MEM_ENTRIES)
+		initial_mem_entries = GH_RM_MAX_MEM_ENTRIES;
+
+	acl_section_size = struct_size(acl_section, entries, p->n_acl_entries);
+	mem_section_size = struct_size(mem_section, entries, initial_mem_entries);

An empty line here seems reasonable.

+	/* The format of the message goes:
+	 * request header
+	 * ACL entries (which VMs get what kind of access to this memory parcel)
+	 * Memory entries (list of memory regions to share)
+	 * Memory attributes (currently unused, we'll hard-code the size to 0)
+	 */
+	msg_size += sizeof(struct gh_rm_mem_share_req_header);

Hidden above is the confirmation that msg_size was initialized to 0,
drop the '+' here to make it clear.

+	msg_size += acl_section_size;
+	msg_size += mem_section_size;
+	msg_size += sizeof(u32); /* for memory attributes, currently unused */
+
+	msg = kzalloc(msg_size, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	req_header = msg;

I don't think you need to keep req_header and msg separately.

+	acl_section = (void *)req_header + sizeof(*req_header);
+	mem_section = (void *)acl_section + acl_section_size;
+	attr_section = (void *)mem_section + mem_section_size;
+
+	req_header->mem_type = p->mem_type;
+	if (initial_mem_entries != p->n_mem_entries)

I think this will only happen if p->n_mem_entries >
GH_RM_MAX_MEM_ENTRIES, for which you already have a condition earlier.
Set the flag in that condition rather than spreading it over two (three)
code paths.

+		req_header->flags |= GH_MEM_SHARE_REQ_FLAGS_APPEND;
+	req_header->label = cpu_to_le32(p->label);
+
+	acl_section->n_entries = cpu_to_le32(p->n_acl_entries);
+	memcpy(acl_section->entries, p->acl_entries,
+		flex_array_size(acl_section, entries, p->n_acl_entries));
+
+	mem_section->n_entries = cpu_to_le16(initial_mem_entries);
+	memcpy(mem_section->entries, p->mem_entries,
+		flex_array_size(mem_section, entries, initial_mem_entries));
+
+	/* Set n_entries for memory attribute section to 0 */
+	*attr_section = 0;
+
+	ret = gh_rm_call(rm, message_id, msg, msg_size, (void **)&resp, &resp_size);
+	kfree(msg);
+
+	if (ret)
+		return ret;
+
+	p->mem_handle = le32_to_cpu(*resp);
+	kfree(resp);
+
+	if (initial_mem_entries != p->n_mem_entries) {

Another "we took that conditional path above", please make it obvious.


Can you please also write a comment describing that this operation is
split in a main one, followed by one of more appended operations as
necessary. This took me too long time to realize...

In particular I was wondering why "initial" didn't mean "the original
value", but now I see that it probably means "the value relevant for the
initial request".

+		ret = gh_rm_mem_append(rm, p->mem_handle,
+					&p->mem_entries[initial_mem_entries],
+					p->n_mem_entries - initial_mem_entries);
+		if (ret) {
+			gh_rm_mem_reclaim(rm, p);
+			p->mem_handle = GH_MEM_HANDLE_INVAL;

What happens to the entries that was already lended or shared?

Regards,
Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel