On Mon, Mar 22, 2021 at 02:13:41PM +0900, Namjae Jeon wrote:
> +++ b/fs/cifsd/mgmt/ksmbd_ida.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2018 Samsung Electronics Co., Ltd.
> + */
> +
> +#include "ksmbd_ida.h"
> +
> +struct ksmbd_ida *ksmbd_ida_alloc(void)
> +{
> + struct ksmbd_ida *ida;
> +
> + ida = kmalloc(sizeof(struct ksmbd_ida), GFP_KERNEL);
> + if (!ida)
> + return NULL;
> +
> + ida_init(&ida->map);
> + return ida;
> +}
... why? Everywhere that you call ksmbd_ida_alloc(), you would
be better off just embedding the struct ida into the struct that
currently has a pointer to it. Or declaring it statically. Then
you can even initialise it statically using DEFINE_IDA() and
eliminate the initialiser functions.
I'd remove the ksmbd_ida abstraction, although I like this wrapper:
> +int ksmbd_acquire_smb2_tid(struct ksmbd_ida *ida)
> +{
> + int id;
> +
> + do {
> + id = __acquire_id(ida, 0, 0);
> + } while (id == 0xFFFF);
> +
> + return id;
Very clever, given your constraint. I might do it as:
int id = ida_alloc(ida, GFP_KERNEL);
if (id == 0xffff)
id = ida_alloc(ida, GFP_KERNEL);
return id;
Although ...
> + tree_conn = ksmbd_alloc(sizeof(struct ksmbd_tree_connect));
> + if (!tree_conn) {
> + status.ret = -ENOMEM;
> + goto out_error;
> + }
> +
> + tree_conn->id = ksmbd_acquire_tree_conn_id(sess);
> + if (tree_conn->id < 0) {
> + status.ret = -EINVAL;
> + goto out_error;
> + }
> +
> + peer_addr = KSMBD_TCP_PEER_SOCKADDR(sess->conn);
> + resp = ksmbd_ipc_tree_connect_request(sess,
> + sc,
> + tree_conn,
> + peer_addr);
> + if (!resp) {
> + status.ret = -EINVAL;
> + goto out_error;
> + }
> +
> + status.ret = resp->status;
> + if (status.ret != KSMBD_TREE_CONN_STATUS_OK)
> + goto out_error;
> +
> + tree_conn->flags = resp->connection_flags;
> + tree_conn->user = sess->user;
> + tree_conn->share_conf = sc;
> + status.tree_conn = tree_conn;
> +
> + list_add(&tree_conn->list, &sess->tree_conn_list);
This is basically the only function which calls that, and this is a relatively
common anti-pattern when using the IDA -- you've allocated a unique ID,
but then you stuff the object in a list and ...
> +struct ksmbd_tree_connect *ksmbd_tree_conn_lookup(struct ksmbd_session *sess,
> + unsigned int id)
> +{
> + struct ksmbd_tree_connect *tree_conn;
> + struct list_head *tmp;
> +
> + list_for_each(tmp, &sess->tree_conn_list) {
> + tree_conn = list_entry(tmp, struct ksmbd_tree_connect, list);
> + if (tree_conn->id == id)
> + return tree_conn;
> + }
... walk the linked list looking for an ID match. You'd be much better
off using an allocating XArray:
https://www.kernel.org/doc/html/latest/core-api/xarray.html
Then you could lookup tree connections in O(log(n)) time instead of
O(n) time.
