As threatened a few months ago https://lwn.net/Articles/750154/, here's a
patch series to convert all users of the non-simple IDA API to ida_alloc*.
Basically every driver is improved by doing this. There are a long
list of people on the bcc for this cover letter, plus patches 2 and 3.
All patches are being sent to linux-kernel. The entire series is also
available as a git tree at
git://git.infradead.org/users/willy/linux-dax.git ida
Matthew Wilcox (26):
radix tree test suite: fix build
ida: Lock the IDA in ida_destroy
ida: Add new API
mtip32xx: Convert to new IDA API
fs: Convert unnamed_dev_ida to new API
fs: Convert namespace IDAs to new API
devpts: Convert to new IDA API
sd: Convert to new IDA interface
osd: Convert to new IDA API
rsxx: Convert to new IDA API
cb710: Convert to new IDA API
Convert net_namespace to new IDA API
ppc: Convert mmu context allocation to new IDA API
media: Convert entity ID allocation to new IDA API
ppc: Convert vas ID allocation to new IDA API
dmaengine: Convert to new IDA API
drm/vmwgfx: Convert to new IDA API
target/iscsi: Allocate session IDs from an IDA
ida: Start new test_ida module
idr-test: Convert ida_check_nomem to new API
test_ida: Move ida_check_leaf
test_ida: Move ida_check_max
test_ida: Convert check_ida_conv to new API
test_ida: check_ida_destroy and check_ida_alloc
ida: Remove old API
ida: Change ida_get_new_above to return the id
arch/powerpc/mm/mmu_context_book3s64.c | 44 +---
arch/powerpc/platforms/powernv/vas-window.c | 26 +--
drivers/block/mtip32xx/mtip32xx.c | 29 +--
drivers/block/rsxx/core.c | 21 +-
drivers/dma/dmaengine.c | 20 +-
drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 41 +---
drivers/media/media-device.c | 16 +-
drivers/misc/cb710/core.c | 23 +-
drivers/scsi/osd/osd_uld.c | 22 +-
drivers/scsi/sd.c | 21 +-
drivers/target/iscsi/iscsi_target.c | 10 +-
drivers/target/iscsi/iscsi_target.h | 4 +-
drivers/target/iscsi/iscsi_target_login.c | 20 +-
fs/devpts/inode.c | 47 ++--
fs/namespace.c | 50 +---
fs/super.c | 63 ++----
include/linux/idr.h | 68 ++++--
lib/Kconfig.debug | 3 +
lib/Makefile | 1 +
lib/idr.c | 155 +++++--------
lib/radix-tree.c | 9 -
lib/test_ida.c | 173 ++++++++++++++
net/core/net_namespace.c | 16 +-
tools/testing/radix-tree/Makefile | 1 +
tools/testing/radix-tree/idr-test.c | 214 ++++--------------
tools/testing/radix-tree/linux/xarray.h | 2 +
tools/testing/radix-tree/main.c | 3 +-
tools/testing/radix-tree/test.h | 3 +-
28 files changed, 450 insertions(+), 655 deletions(-)
create mode 100644 lib/test_ida.c
create mode 100644 tools/testing/radix-tree/linux/xarray.h
--
2.17.1
Removes a call to ida_pre_get().
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/media/media-device.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index ae59c3177555..d51088bcd735 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -575,18 +575,12 @@ int __must_check media_device_register_entity(struct media_device *mdev,
entity->num_links = 0;
entity->num_backlinks = 0;
- if (!ida_pre_get(&mdev->entity_internal_idx, GFP_KERNEL))
- return -ENOMEM;
-
- mutex_lock(&mdev->graph_mutex);
-
- ret = ida_get_new_above(&mdev->entity_internal_idx, 1,
- &entity->internal_idx);
- if (ret < 0) {
- mutex_unlock(&mdev->graph_mutex);
+ ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
+ if (ret < 0)
return ret;
- }
+ entity->internal_idx = ret;
+ mutex_lock(&mdev->graph_mutex);
mdev->entity_internal_idx_max =
max(mdev->entity_internal_idx_max, entity->internal_idx);
@@ -632,7 +626,7 @@ static void __media_device_unregister_entity(struct media_entity *entity)
struct media_interface *intf;
unsigned int i;
- ida_simple_remove(&mdev->entity_internal_idx, entity->internal_idx);
+ ida_free(&mdev->entity_internal_idx, entity->internal_idx);
/* Remove all interface links pointing to this entity */
list_for_each_entry(intf, &mdev->interfaces, graph_obj.list) {
--
2.17.1
Eliminate the custom spinlock and the call to ida_pre_get.
Also add a call to ida_free() in the card remove routine, which I believe
fixes a bug in this driver.
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/block/rsxx/core.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index b7d71914a32a..f2c631ce793c 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -58,7 +58,6 @@ MODULE_PARM_DESC(sync_start, "On by Default: Driver load will not complete "
"until the card startup has completed.");
static DEFINE_IDA(rsxx_disk_ida);
-static DEFINE_SPINLOCK(rsxx_ida_lock);
/* --------------------Debugfs Setup ------------------- */
@@ -771,19 +770,10 @@ static int rsxx_pci_probe(struct pci_dev *dev,
card->dev = dev;
pci_set_drvdata(dev, card);
- do {
- if (!ida_pre_get(&rsxx_disk_ida, GFP_KERNEL)) {
- st = -ENOMEM;
- goto failed_ida_get;
- }
-
- spin_lock(&rsxx_ida_lock);
- st = ida_get_new(&rsxx_disk_ida, &card->disk_id);
- spin_unlock(&rsxx_ida_lock);
- } while (st == -EAGAIN);
-
- if (st)
+ st = ida_alloc(&rsxx_disk_ida, GFP_KERNEL);
+ if (st < 0)
goto failed_ida_get;
+ card->disk_id = st;
st = pci_enable_device(dev);
if (st)
@@ -985,9 +975,7 @@ static int rsxx_pci_probe(struct pci_dev *dev,
failed_dma_mask:
pci_disable_device(dev);
failed_enable:
- spin_lock(&rsxx_ida_lock);
- ida_remove(&rsxx_disk_ida, card->disk_id);
- spin_unlock(&rsxx_ida_lock);
+ ida_free(&rsxx_disk_ida, card->disk_id);
failed_ida_get:
kfree(card);
@@ -1050,6 +1038,7 @@ static void rsxx_pci_remove(struct pci_dev *dev)
pci_disable_device(dev);
pci_release_regions(dev);
+ ida_free(&rsxx_disk_ida, card->disk_id);
kfree(card);
}
--
2.17.1
Allows us to remove an explicit spinlock.
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/scsi/sd.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 9421d9877730..8a493e64856c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -123,7 +123,6 @@ static void scsi_disk_release(struct device *cdev);
static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);
static void sd_print_result(const struct scsi_disk *, const char *, int);
-static DEFINE_SPINLOCK(sd_index_lock);
static DEFINE_IDA(sd_index_ida);
/* This semaphore is used to mediate the 0->1 reference get in the
@@ -3339,16 +3338,8 @@ static int sd_probe(struct device *dev)
if (!gd)
goto out_free;
- do {
- if (!ida_pre_get(&sd_index_ida, GFP_KERNEL))
- goto out_put;
-
- spin_lock(&sd_index_lock);
- error = ida_get_new(&sd_index_ida, &index);
- spin_unlock(&sd_index_lock);
- } while (error == -EAGAIN);
-
- if (error) {
+ index = ida_alloc(&sd_index_ida, GFP_KERNEL);
+ if (index < 0) {
sdev_printk(KERN_WARNING, sdp, "sd_probe: memory exhausted.\n");
goto out_put;
}
@@ -3392,9 +3383,7 @@ static int sd_probe(struct device *dev)
return 0;
out_free_index:
- spin_lock(&sd_index_lock);
- ida_remove(&sd_index_ida, index);
- spin_unlock(&sd_index_lock);
+ ida_free(&sd_index_ida, index);
out_put:
put_disk(gd);
out_free:
@@ -3459,9 +3448,7 @@ static void scsi_disk_release(struct device *dev)
struct scsi_disk *sdkp = to_scsi_disk(dev);
struct gendisk *disk = sdkp->disk;
- spin_lock(&sd_index_lock);
- ida_remove(&sd_index_ida, sdkp->index);
- spin_unlock(&sd_index_lock);
+ ida_free(&sd_index_ida, sdkp->index);
disk->private_data = NULL;
put_disk(disk);
--
2.17.1
Removes a custom spinlock and simplifies the code.
Signed-off-by: Matthew Wilcox <[email protected]>
---
arch/powerpc/platforms/powernv/vas-window.c | 26 ++++-----------------
1 file changed, 4 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index ff9f48812331..2a5e68a2664d 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -515,35 +515,17 @@ int init_winctx_regs(struct vas_window *window, struct vas_winctx *winctx)
return 0;
}
-static DEFINE_SPINLOCK(vas_ida_lock);
-
static void vas_release_window_id(struct ida *ida, int winid)
{
- spin_lock(&vas_ida_lock);
- ida_remove(ida, winid);
- spin_unlock(&vas_ida_lock);
+ ida_free(ida, winid);
}
static int vas_assign_window_id(struct ida *ida)
{
- int rc, winid;
-
- do {
- rc = ida_pre_get(ida, GFP_KERNEL);
- if (!rc)
- return -EAGAIN;
-
- spin_lock(&vas_ida_lock);
- rc = ida_get_new(ida, &winid);
- spin_unlock(&vas_ida_lock);
- } while (rc == -EAGAIN);
-
- if (rc)
- return rc;
+ int winid = ida_alloc_max(ida, VAX_WINDOWS_PER_CHIP, GFP_KERNEL);
- if (winid > VAS_WINDOWS_PER_CHIP) {
- pr_err("Too many (%d) open windows\n", winid);
- vas_release_window_id(ida, winid);
+ if (winid == -ENOSPC) {
+ pr_err("Too many (%d) open windows\n", VAX_WINDOWS_PER_CHIP);
return -EAGAIN;
}
--
2.17.1
We don't need to keep track of the starting value; the IDA is efficient.
Signed-off-by: Matthew Wilcox <[email protected]>
---
fs/namespace.c | 50 ++++++++++++--------------------------------------
1 file changed, 12 insertions(+), 38 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index 8ddd14806799..bfd33eaab5aa 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -61,9 +61,6 @@ __setup("mphash_entries=", set_mphash_entries);
static u64 event;
static DEFINE_IDA(mnt_id_ida);
static DEFINE_IDA(mnt_group_ida);
-static DEFINE_SPINLOCK(mnt_id_lock);
-static int mnt_id_start = 0;
-static int mnt_group_start = 1;
static struct hlist_head *mount_hashtable __read_mostly;
static struct hlist_head *mountpoint_hashtable __read_mostly;
@@ -101,50 +98,30 @@ static inline struct hlist_head *mp_hash(struct dentry *dentry)
static int mnt_alloc_id(struct mount *mnt)
{
- int res;
+ int res = ida_alloc(&mnt_id_ida, GFP_KERNEL);
-retry:
- ida_pre_get(&mnt_id_ida, GFP_KERNEL);
- spin_lock(&mnt_id_lock);
- res = ida_get_new_above(&mnt_id_ida, mnt_id_start, &mnt->mnt_id);
- if (!res)
- mnt_id_start = mnt->mnt_id + 1;
- spin_unlock(&mnt_id_lock);
- if (res == -EAGAIN)
- goto retry;
-
- return res;
+ if (res < 0)
+ return res;
+ mnt->mnt_id = res;
+ return 0;
}
static void mnt_free_id(struct mount *mnt)
{
- int id = mnt->mnt_id;
- spin_lock(&mnt_id_lock);
- ida_remove(&mnt_id_ida, id);
- if (mnt_id_start > id)
- mnt_id_start = id;
- spin_unlock(&mnt_id_lock);
+ ida_free(&mnt_id_ida, mnt->mnt_id);
}
/*
* Allocate a new peer group ID
- *
- * mnt_group_ida is protected by namespace_sem
*/
static int mnt_alloc_group_id(struct mount *mnt)
{
- int res;
+ int res = ida_alloc_min(&mnt_group_ida, 1, GFP_KERNEL);
- if (!ida_pre_get(&mnt_group_ida, GFP_KERNEL))
- return -ENOMEM;
-
- res = ida_get_new_above(&mnt_group_ida,
- mnt_group_start,
- &mnt->mnt_group_id);
- if (!res)
- mnt_group_start = mnt->mnt_group_id + 1;
-
- return res;
+ if (res < 0)
+ return res;
+ mnt->mnt_group_id = res;
+ return 0;
}
/*
@@ -152,10 +129,7 @@ static int mnt_alloc_group_id(struct mount *mnt)
*/
void mnt_release_group_id(struct mount *mnt)
{
- int id = mnt->mnt_group_id;
- ida_remove(&mnt_group_ida, id);
- if (mnt_group_start > id)
- mnt_group_start = id;
+ ida_free(&mnt_group_ida, mnt->mnt_group_id);
mnt->mnt_group_id = 0;
}
--
2.17.1
Eliminates the custom spinlock and the call to ida_pre_get.
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/misc/cb710/core.c | 23 +++++------------------
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/drivers/misc/cb710/core.c b/drivers/misc/cb710/core.c
index 4d4acf763b65..2c43fd09d602 100644
--- a/drivers/misc/cb710/core.c
+++ b/drivers/misc/cb710/core.c
@@ -16,7 +16,6 @@
#include <linux/gfp.h>
static DEFINE_IDA(cb710_ida);
-static DEFINE_SPINLOCK(cb710_ida_lock);
void cb710_pci_update_config_reg(struct pci_dev *pdev,
int reg, uint32_t mask, uint32_t xor)
@@ -205,7 +204,6 @@ static int cb710_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
{
struct cb710_chip *chip;
- unsigned long flags;
u32 val;
int err;
int n = 0;
@@ -256,18 +254,10 @@ static int cb710_probe(struct pci_dev *pdev,
if (err)
return err;
- do {
- if (!ida_pre_get(&cb710_ida, GFP_KERNEL))
- return -ENOMEM;
-
- spin_lock_irqsave(&cb710_ida_lock, flags);
- err = ida_get_new(&cb710_ida, &chip->platform_id);
- spin_unlock_irqrestore(&cb710_ida_lock, flags);
-
- if (err && err != -EAGAIN)
- return err;
- } while (err);
-
+ err = ida_alloc(&cb710_ida, GFP_KERNEL);
+ if (err < 0)
+ return err;
+ chip->platform_id = err;
dev_info(&pdev->dev, "id %d, IO 0x%p, IRQ %d\n",
chip->platform_id, chip->iobase, pdev->irq);
@@ -308,7 +298,6 @@ static int cb710_probe(struct pci_dev *pdev,
static void cb710_remove_one(struct pci_dev *pdev)
{
struct cb710_chip *chip = pci_get_drvdata(pdev);
- unsigned long flags;
cb710_unregister_slot(chip, CB710_SLOT_SM);
cb710_unregister_slot(chip, CB710_SLOT_MS);
@@ -317,9 +306,7 @@ static void cb710_remove_one(struct pci_dev *pdev)
BUG_ON(atomic_read(&chip->slot_refs_count) != 0);
#endif
- spin_lock_irqsave(&cb710_ida_lock, flags);
- ida_remove(&cb710_ida, chip->platform_id);
- spin_unlock_irqrestore(&cb710_ida_lock, flags);
+ ida_free(&cb710_ida, chip->platform_id);
}
static const struct pci_device_id cb710_pci_tbl[] = {
--
2.17.1
Start transitioning the IDA tests into kernel space. Framework heavily
cribbed from test_xarray.c.
Signed-off-by: Matthew Wilcox <[email protected]>
---
lib/Kconfig.debug | 3 +++
lib/Makefile | 1 +
lib/test_ida.c | 42 +++++++++++++++++++++++++++++
tools/testing/radix-tree/Makefile | 1 +
tools/testing/radix-tree/idr-test.c | 22 ++++++++++++---
tools/testing/radix-tree/main.c | 3 +--
tools/testing/radix-tree/test.h | 3 +--
7 files changed, 68 insertions(+), 7 deletions(-)
create mode 100644 lib/test_ida.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 8838d1158d19..2fff661d9070 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1827,6 +1827,9 @@ config TEST_HASH
This is intended to help people writing architecture-specific
optimized versions. If unsure, say N.
+config TEST_IDA
+ tristate "Perform selftest on IDA functions"
+
config TEST_PARMAN
tristate "Perform selftest on priority array manager"
default n
diff --git a/lib/Makefile b/lib/Makefile
index 8153fdab287f..2ad90b716995 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_TEST_BPF) += test_bpf.o
obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
+obj-$(CONFIG_TEST_IDA) += test_ida.o
obj-$(CONFIG_TEST_KASAN) += test_kasan.o
CFLAGS_test_kasan.o += -fno-builtin
obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o
diff --git a/lib/test_ida.c b/lib/test_ida.c
new file mode 100644
index 000000000000..5a5a742d5f09
--- /dev/null
+++ b/lib/test_ida.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * test_ida.c: Test the IDA API
+ * Copyright (c) 2016-2018 Microsoft Corporation
+ * Copyright (c) 2018 Oracle Corporation
+ * Author: Matthew Wilcox <[email protected]>
+ */
+
+static unsigned int tests_run;
+static unsigned int tests_passed;
+
+#ifdef __KERNEL__
+void ida_dump(struct ida *ida) { }
+#endif
+#define IDA_BUG_ON(ida, x) do { \
+ tests_run++; \
+ if (x) { \
+ ida_dump(ida); \
+ dump_stack(); \
+ } else { \
+ tests_passed++; \
+ } \
+} while (0)
+
+static int ida_checks(void)
+{
+ DEFINE_IDA(ida);
+
+ IDA_BUG_ON(&ida, !ida_is_empty(&ida));
+
+ printk("IDA: %u of %u tests passed\n", tests_passed, tests_run);
+ return (tests_run != tests_passed) ? 0 : -EINVAL;
+}
+
+static void ida_exit(void)
+{
+}
+
+module_init(ida_checks);
+module_exit(ida_exit);
+MODULE_AUTHOR("Matthew Wilcox <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/radix-tree/Makefile b/tools/testing/radix-tree/Makefile
index db66f8a0d4be..b5a9db9e6a31 100644
--- a/tools/testing/radix-tree/Makefile
+++ b/tools/testing/radix-tree/Makefile
@@ -21,6 +21,7 @@ targets: generated/map-shift.h $(TARGETS)
main: $(OFILES)
+idr-test.o: ../../../lib/test_ida.c
idr-test: idr-test.o $(CORE_OFILES)
multiorder: multiorder.o $(CORE_OFILES)
diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index ee820fcc29b0..604b51dc9b38 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -309,6 +309,15 @@ void idr_checks(void)
idr_u32_test(0);
}
+#define module_init(x)
+#define module_exit(x)
+#define MODULE_AUTHOR(x)
+#define MODULE_LICENSE(x)
+#define dump_stack() assert(0)
+void ida_dump(struct ida *);
+
+#include "../../../lib/test_ida.c"
+
/*
* Check that we get the correct error when we run out of memory doing
* allocations. To ensure we run out of memory, just "forget" to preload.
@@ -488,7 +497,7 @@ void ida_simple_get_remove_test(void)
ida_destroy(&ida);
}
-void ida_checks(void)
+void user_ida_checks(void)
{
DEFINE_IDA(ida);
int id;
@@ -582,12 +591,19 @@ void ida_thread_tests(void)
pthread_join(threads[i], NULL);
}
+void ida_tests(void)
+{
+ user_ida_checks();
+ ida_checks();
+ ida_exit();
+ ida_thread_tests();
+}
+
int __weak main(void)
{
radix_tree_init();
idr_checks();
- ida_checks();
- ida_thread_tests();
+ ida_tests();
radix_tree_cpu_dead(1);
rcu_barrier();
if (nr_allocated)
diff --git a/tools/testing/radix-tree/main.c b/tools/testing/radix-tree/main.c
index 257f3f8aacaa..c69a49b825aa 100644
--- a/tools/testing/radix-tree/main.c
+++ b/tools/testing/radix-tree/main.c
@@ -322,7 +322,7 @@ static void single_thread_tests(bool long_run)
printv(2, "after dynamic_height_check: %d allocated, preempt %d\n",
nr_allocated, preempt_count);
idr_checks();
- ida_checks();
+ ida_tests();
rcu_barrier();
printv(2, "after idr_checks: %d allocated, preempt %d\n",
nr_allocated, preempt_count);
@@ -369,7 +369,6 @@ int main(int argc, char **argv)
iteration_test(0, 10 + 90 * long_run);
iteration_test(7, 10 + 90 * long_run);
single_thread_tests(long_run);
- ida_thread_tests();
/* Free any remaining preallocated nodes */
radix_tree_cpu_dead(0);
diff --git a/tools/testing/radix-tree/test.h b/tools/testing/radix-tree/test.h
index 31f1d9b6f506..92d901eacf49 100644
--- a/tools/testing/radix-tree/test.h
+++ b/tools/testing/radix-tree/test.h
@@ -39,8 +39,7 @@ void multiorder_checks(void);
void iteration_test(unsigned order, unsigned duration);
void benchmark(void);
void idr_checks(void);
-void ida_checks(void);
-void ida_thread_tests(void);
+void ida_tests(void);
struct item *
item_tag_set(struct radix_tree_root *root, unsigned long index, int tag);
--
2.17.1
This calling convention makes more sense for the implementation as well
as the callers. It even shaves 32 bytes off the compiled code size.
Signed-off-by: Matthew Wilcox <[email protected]>
---
lib/idr.c | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/lib/idr.c b/lib/idr.c
index 1dc52191acb6..fab2fd5bc326 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -363,7 +363,7 @@ EXPORT_SYMBOL(idr_replace);
#define IDA_MAX (0x80000000U / IDA_BITMAP_BITS - 1)
-static int ida_get_new_above(struct ida *ida, int start, int *id)
+static int ida_get_new_above(struct ida *ida, int start)
{
struct radix_tree_root *root = &ida->ida_rt;
void __rcu **slot;
@@ -402,8 +402,8 @@ static int ida_get_new_above(struct ida *ida, int start, int *id)
if (ebit < BITS_PER_LONG) {
tmp |= 1UL << ebit;
rcu_assign_pointer(*slot, (void *)tmp);
- *id = new + ebit - RADIX_TREE_EXCEPTIONAL_SHIFT;
- return 0;
+ return new + ebit -
+ RADIX_TREE_EXCEPTIONAL_SHIFT;
}
bitmap = this_cpu_xchg(ida_bitmap, NULL);
if (!bitmap)
@@ -434,8 +434,7 @@ static int ida_get_new_above(struct ida *ida, int start, int *id)
RADIX_TREE_EXCEPTIONAL_ENTRY);
radix_tree_iter_replace(root, &iter, slot,
bitmap);
- *id = new;
- return 0;
+ return new;
}
bitmap = this_cpu_xchg(ida_bitmap, NULL);
if (!bitmap)
@@ -444,8 +443,7 @@ static int ida_get_new_above(struct ida *ida, int start, int *id)
radix_tree_iter_replace(root, &iter, slot, bitmap);
}
- *id = new;
- return 0;
+ return new;
}
}
@@ -534,7 +532,7 @@ EXPORT_SYMBOL(ida_destroy);
int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
gfp_t gfp)
{
- int ret, id = 0;
+ int id = 0;
unsigned long flags;
if ((int)min < 0)
@@ -545,24 +543,20 @@ int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
again:
xa_lock_irqsave(&ida->ida_rt, flags);
- ret = ida_get_new_above(ida, min, &id);
- if (!ret) {
- if (id > max) {
- ida_remove(ida, id);
- ret = -ENOSPC;
- } else {
- ret = id;
- }
+ id = ida_get_new_above(ida, min);
+ if (id > (int)max) {
+ ida_remove(ida, id);
+ id = -ENOSPC;
}
xa_unlock_irqrestore(&ida->ida_rt, flags);
- if (unlikely(ret == -EAGAIN)) {
+ if (unlikely(id == -EAGAIN)) {
if (!ida_pre_get(ida, gfp))
return -ENOMEM;
goto again;
}
- return ret;
+ return id;
}
EXPORT_SYMBOL(ida_alloc_range);
--
2.17.1
Delete ida_pre_get(), ida_get_new(), ida_get_new_above() and ida_remove()
from the public API. Some of these functions still exist as internal
helpers, but they should not be called by consumers.
Signed-off-by: Matthew Wilcox <[email protected]>
---
include/linux/idr.h | 21 ++++--------------
lib/idr.c | 52 ++++++++-------------------------------------
lib/radix-tree.c | 9 --------
3 files changed, 13 insertions(+), 69 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index cd339da0b1aa..2e1db0e36486 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -225,13 +225,9 @@ struct ida {
}
#define DEFINE_IDA(name) struct ida name = IDA_INIT(name)
-int ida_pre_get(struct ida *ida, gfp_t gfp_mask);
-int ida_get_new_above(struct ida *ida, int starting_id, int *p_id);
-void ida_remove(struct ida *ida, int id);
-void ida_destroy(struct ida *ida);
-
int ida_alloc_range(struct ida *, unsigned int min, unsigned int max, gfp_t);
void ida_free(struct ida *, unsigned int id);
+void ida_destroy(struct ida *ida);
/**
* ida_alloc() - Allocate an unused ID.
@@ -292,20 +288,11 @@ static inline void ida_init(struct ida *ida)
ida_alloc_range(ida, start, (end) - 1, gfp)
#define ida_simple_remove(ida, id) ida_free(ida, id)
-/**
- * ida_get_new - allocate new ID
- * @ida: idr handle
- * @p_id: pointer to the allocated handle
- *
- * Simple wrapper around ida_get_new_above() w/ @starting_id of zero.
- */
-static inline int ida_get_new(struct ida *ida, int *p_id)
-{
- return ida_get_new_above(ida, 0, p_id);
-}
-
static inline bool ida_is_empty(const struct ida *ida)
{
return radix_tree_empty(&ida->ida_rt);
}
+
+/* in lib/radix-tree.c */
+int ida_pre_get(struct ida *ida, gfp_t gfp_mask);
#endif /* __IDR_H__ */
diff --git a/lib/idr.c b/lib/idr.c
index 8b3a5e7eb734..1dc52191acb6 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -317,19 +317,12 @@ EXPORT_SYMBOL(idr_replace);
* bit per ID, and so is more space efficient than an IDR. To use an IDA,
* define it using DEFINE_IDA() (or embed a &struct ida in a data structure,
* then initialise it using ida_init()). To allocate a new ID, call
- * ida_alloc(), ida_alloc_max() or ida_alloc_range(). To free an ID, call
- * ida_free().
+ * ida_alloc(), ida_alloc_min(), ida_alloc_max() or ida_alloc_range().
+ * To free an ID, call ida_free().
*
- * If you have more complex locking requirements, use a loop around
- * ida_pre_get() and ida_get_new() to allocate a new ID. Then use
- * ida_remove() to free an ID. You must make sure that ida_get_new() and
- * ida_remove() cannot be called at the same time as each other for the
- * same IDA.
- *
- * You can also use ida_get_new_above() if you need an ID to be allocated
- * above a particular number. ida_destroy() can be used to dispose of an
- * IDA without needing to free the individual IDs in it. You can use
- * ida_is_empty() to find out whether the IDA has any IDs currently allocated.
+ * ida_destroy() can be used to dispose of an IDA without needing to
+ * free the individual IDs in it. You can use ida_is_empty() to find
+ * out whether the IDA has any IDs currently allocated.
*
* IDs are currently limited to the range [0-INT_MAX]. If this is an awkward
* limitation, it should be quite straightforward to raise the maximum.
@@ -370,25 +363,7 @@ EXPORT_SYMBOL(idr_replace);
#define IDA_MAX (0x80000000U / IDA_BITMAP_BITS - 1)
-/**
- * ida_get_new_above - allocate new ID above or equal to a start id
- * @ida: ida handle
- * @start: id to start search at
- * @id: pointer to the allocated handle
- *
- * Allocate new ID above or equal to @start. It should be called
- * with any required locks to ensure that concurrent calls to
- * ida_get_new_above() / ida_get_new() / ida_remove() are not allowed.
- * Consider using ida_alloc_range() if you do not have complex locking
- * requirements.
- *
- * If memory is required, it will return %-EAGAIN, you should unlock
- * and go back to the ida_pre_get() call. If the ida is full, it will
- * return %-ENOSPC. On success, it will return 0.
- *
- * @id returns a value in the range @start ... %0x7fffffff.
- */
-int ida_get_new_above(struct ida *ida, int start, int *id)
+static int ida_get_new_above(struct ida *ida, int start, int *id)
{
struct radix_tree_root *root = &ida->ida_rt;
void __rcu **slot;
@@ -473,16 +448,8 @@ int ida_get_new_above(struct ida *ida, int start, int *id)
return 0;
}
}
-EXPORT_SYMBOL(ida_get_new_above);
-/**
- * ida_remove - Free the given ID
- * @ida: ida handle
- * @id: ID to free
- *
- * This function should not be called at the same time as ida_get_new_above().
- */
-void ida_remove(struct ida *ida, int id)
+static void ida_remove(struct ida *ida, int id)
{
unsigned long index = id / IDA_BITMAP_BITS;
unsigned offset = id % IDA_BITMAP_BITS;
@@ -519,9 +486,8 @@ void ida_remove(struct ida *ida, int id)
}
return;
err:
- WARN(1, "ida_remove called for id=%d which is not allocated.\n", id);
+ WARN(1, "ida_free called for id=%d which is not allocated.\n", id);
}
-EXPORT_SYMBOL(ida_remove);
/**
* ida_destroy() - Free all IDs.
@@ -568,7 +534,7 @@ EXPORT_SYMBOL(ida_destroy);
int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
gfp_t gfp)
{
- int ret, id;
+ int ret, id = 0;
unsigned long flags;
if ((int)min < 0)
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index a9e41aed6de4..c12b87981778 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -2106,14 +2106,6 @@ void idr_preload(gfp_t gfp_mask)
}
EXPORT_SYMBOL(idr_preload);
-/**
- * ida_pre_get - reserve resources for ida allocation
- * @ida: ida handle
- * @gfp: memory allocation flags
- *
- * This function should be called before calling ida_get_new_above(). If it
- * is unable to allocate memory, it will return %0. On success, it returns %1.
- */
int ida_pre_get(struct ida *ida, gfp_t gfp)
{
/*
@@ -2134,7 +2126,6 @@ int ida_pre_get(struct ida *ida, gfp_t gfp)
return 1;
}
-EXPORT_SYMBOL(ida_pre_get);
void __rcu **idr_get_free(struct radix_tree_root *root,
struct radix_tree_iter *iter, gfp_t gfp,
--
2.17.1
Move these tests from the userspace test-suite to the kernel test-suite.
Also convert check_ida_random to the new API.
Signed-off-by: Matthew Wilcox <[email protected]>
---
lib/test_ida.c | 55 ++++++++++++++++++++++-
tools/testing/radix-tree/idr-test.c | 70 ++---------------------------
2 files changed, 58 insertions(+), 67 deletions(-)
diff --git a/lib/test_ida.c b/lib/test_ida.c
index 15e013800171..433cb8a69732 100644
--- a/lib/test_ida.c
+++ b/lib/test_ida.c
@@ -22,6 +22,58 @@ void ida_dump(struct ida *ida) { }
} \
} while (0)
+/*
+ * Straightforward checks that allocating and freeing IDs work.
+ */
+static void ida_check_alloc(struct ida *ida)
+{
+ int i, id;
+
+ for (i = 0; i < 10000; i++)
+ IDA_BUG_ON(ida, ida_alloc(ida, GFP_KERNEL) != i);
+
+ ida_free(ida, 20);
+ ida_free(ida, 21);
+ for (i = 0; i < 3; i++) {
+ id = ida_alloc(ida, GFP_KERNEL);
+ IDA_BUG_ON(ida, id < 0);
+ if (i == 2)
+ IDA_BUG_ON(ida, id != 10000);
+ }
+
+ for (i = 0; i < 5000; i++)
+ ida_free(ida, i);
+
+ IDA_BUG_ON(ida, ida_alloc_min(ida, 5000, GFP_KERNEL) != 10001);
+ ida_destroy(ida);
+
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+}
+
+/* Destroy an IDA with a single entry at @base */
+static void ida_check_destroy_1(struct ida *ida, unsigned int base)
+{
+ IDA_BUG_ON(ida, ida_alloc_min(ida, base, GFP_KERNEL) != base);
+ IDA_BUG_ON(ida, ida_is_empty(ida));
+ ida_destroy(ida);
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+}
+
+/* Check that ida_destroy and ida_is_empty work */
+static void ida_check_destroy(struct ida *ida)
+{
+ /* Destroy an already-empty IDA */
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+ ida_destroy(ida);
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+
+ ida_check_destroy_1(ida, 0);
+ ida_check_destroy_1(ida, 1);
+ ida_check_destroy_1(ida, 1023);
+ ida_check_destroy_1(ida, 1024);
+ ida_check_destroy_1(ida, 12345678);
+}
+
/*
* Check what happens when we fill a leaf and then delete it. This may
* discover mishandling of IDR_FREE.
@@ -71,7 +123,6 @@ static void ida_check_max(struct ida *ida)
static void ida_check_conv(struct ida *ida)
{
unsigned long i;
- int id;
for (i = 0; i < IDA_BITMAP_BITS * 2; i += IDA_BITMAP_BITS) {
IDA_BUG_ON(ida, ida_alloc_min(ida, i + 1, GFP_KERNEL) != i + 1);
@@ -100,6 +151,8 @@ static int ida_checks(void)
DEFINE_IDA(ida);
IDA_BUG_ON(&ida, !ida_is_empty(&ida));
+ ida_check_alloc(&ida);
+ ida_check_destroy(&ida);
ida_check_leaf(&ida, 0);
ida_check_leaf(&ida, 1024);
ida_check_leaf(&ida, 1024 * 64);
diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index f948e38b9a6b..052c0c4cc0f3 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -364,7 +364,6 @@ void ida_check_random(void)
{
DEFINE_IDA(ida);
DECLARE_BITMAP(bitmap, 2048);
- int id, err;
unsigned int i;
time_t s = time(NULL);
@@ -375,15 +374,11 @@ void ida_check_random(void)
int bit = i & 2047;
if (test_bit(bit, bitmap)) {
__clear_bit(bit, bitmap);
- ida_remove(&ida, bit);
+ ida_free(&ida, bit);
} else {
__set_bit(bit, bitmap);
- do {
- ida_pre_get(&ida, GFP_KERNEL);
- err = ida_get_new_above(&ida, bit, &id);
- } while (err == -EAGAIN);
- assert(!err);
- assert(id == bit);
+ IDA_BUG_ON(&ida, ida_alloc_min(&ida, bit, GFP_KERNEL)
+ != bit);
}
}
ida_destroy(&ida);
@@ -411,66 +406,9 @@ void ida_simple_get_remove_test(void)
void user_ida_checks(void)
{
- DEFINE_IDA(ida);
- int id;
- unsigned long i;
-
radix_tree_cpu_dead(1);
- ida_check_nomem();
-
- for (i = 0; i < 10000; i++) {
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new(&ida, &id));
- assert(id == i);
- }
-
- ida_remove(&ida, 20);
- ida_remove(&ida, 21);
- for (i = 0; i < 3; i++) {
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new(&ida, &id));
- if (i == 2)
- assert(id == 10000);
- }
-
- for (i = 0; i < 5000; i++)
- ida_remove(&ida, i);
-
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new_above(&ida, 5000, &id));
- assert(id == 10001);
-
- ida_destroy(&ida);
-
- assert(ida_is_empty(&ida));
-
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new_above(&ida, 1, &id));
- assert(id == 1);
-
- ida_remove(&ida, id);
- assert(ida_is_empty(&ida));
- ida_destroy(&ida);
- assert(ida_is_empty(&ida));
-
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new_above(&ida, 1, &id));
- ida_destroy(&ida);
- assert(ida_is_empty(&ida));
-
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new_above(&ida, 1, &id));
- assert(id == 1);
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new_above(&ida, 1025, &id));
- assert(id == 1025);
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new_above(&ida, 10000, &id));
- assert(id == 10000);
- ida_remove(&ida, 1025);
- ida_destroy(&ida);
- assert(ida_is_empty(&ida));
+ ida_check_nomem();
ida_check_conv_user();
ida_check_random();
ida_simple_get_remove_test();
--
2.17.1
Convert to new API and move to kernel space.
Signed-off-by: Matthew Wilcox <[email protected]>
---
lib/test_ida.c | 23 +++++++++++++++++++++++
tools/testing/radix-tree/idr-test.c | 28 ----------------------------
2 files changed, 23 insertions(+), 28 deletions(-)
diff --git a/lib/test_ida.c b/lib/test_ida.c
index 50dce991be6b..e8ab544032f9 100644
--- a/lib/test_ida.c
+++ b/lib/test_ida.c
@@ -43,6 +43,28 @@ static void ida_check_leaf(struct ida *ida, unsigned int base)
IDA_BUG_ON(ida, !ida_is_empty(ida));
}
+/*
+ * Check allocations up to and slightly above the maximum allowed (2^31-1) ID.
+ * Allocating up to 2^31-1 should succeed, and then allocating the next one
+ * should fail.
+ */
+static void ida_check_max(struct ida *ida)
+{
+ unsigned long i, j;
+
+ for (j = 1; j < 65537; j *= 2) {
+ unsigned long base = (1UL << 31) - j;
+ for (i = 0; i < j; i++) {
+ IDA_BUG_ON(ida, ida_alloc_min(ida, base, GFP_KERNEL) !=
+ base + i);
+ }
+ IDA_BUG_ON(ida, ida_alloc_min(ida, base, GFP_KERNEL) !=
+ -ENOSPC);
+ ida_destroy(ida);
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+ }
+}
+
static int ida_checks(void)
{
DEFINE_IDA(ida);
@@ -51,6 +73,7 @@ static int ida_checks(void)
ida_check_leaf(&ida, 0);
ida_check_leaf(&ida, 1024);
ida_check_leaf(&ida, 1024 * 64);
+ ida_check_max(&ida);
printk("IDA: %u of %u tests passed\n", tests_passed, tests_run);
return (tests_run != tests_passed) ? 0 : -EINVAL;
diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index fef1f45b927b..bd9699327f95 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -396,33 +396,6 @@ void ida_check_conv(void)
ida_destroy(&ida);
}
-/*
- * Check allocations up to and slightly above the maximum allowed (2^31-1) ID.
- * Allocating up to 2^31-1 should succeed, and then allocating the next one
- * should fail.
- */
-void ida_check_max(void)
-{
- DEFINE_IDA(ida);
- int id, err;
- unsigned long i, j;
-
- for (j = 1; j < 65537; j *= 2) {
- unsigned long base = (1UL << 31) - j;
- for (i = 0; i < j; i++) {
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new_above(&ida, base, &id));
- assert(id == base + i);
- }
- assert(ida_pre_get(&ida, GFP_KERNEL));
- err = ida_get_new_above(&ida, base, &id);
- assert(err == -ENOSPC);
- ida_destroy(&ida);
- assert(ida_is_empty(&ida));
- rcu_barrier();
- }
-}
-
void ida_check_random(void)
{
DEFINE_IDA(ida);
@@ -534,7 +507,6 @@ void user_ida_checks(void)
ida_destroy(&ida);
assert(ida_is_empty(&ida));
- ida_check_max();
ida_check_conv();
ida_check_random();
ida_simple_get_remove_test();
--
2.17.1
Move as much as possible to kernel space; leave the parts in user space
that rely on checking memory allocation failures to detect the
transition between an exceptional entry and a bitmap.
Signed-off-by: Matthew Wilcox <[email protected]>
---
lib/test_ida.c | 31 ++++++++++++++++
tools/testing/radix-tree/idr-test.c | 56 ++++++-----------------------
2 files changed, 41 insertions(+), 46 deletions(-)
diff --git a/lib/test_ida.c b/lib/test_ida.c
index e8ab544032f9..15e013800171 100644
--- a/lib/test_ida.c
+++ b/lib/test_ida.c
@@ -65,6 +65,36 @@ static void ida_check_max(struct ida *ida)
}
}
+/*
+ * Check handling of conversions between exceptional entries and full bitmaps.
+ */
+static void ida_check_conv(struct ida *ida)
+{
+ unsigned long i;
+ int id;
+
+ for (i = 0; i < IDA_BITMAP_BITS * 2; i += IDA_BITMAP_BITS) {
+ IDA_BUG_ON(ida, ida_alloc_min(ida, i + 1, GFP_KERNEL) != i + 1);
+ IDA_BUG_ON(ida, ida_alloc_min(ida, i + BITS_PER_LONG,
+ GFP_KERNEL) != i + BITS_PER_LONG);
+ ida_free(ida, i + 1);
+ ida_free(ida, i + BITS_PER_LONG);
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+ }
+
+ for (i = 0; i < IDA_BITMAP_BITS * 2; i++)
+ IDA_BUG_ON(ida, ida_alloc(ida, GFP_KERNEL) != i);
+ for (i = IDA_BITMAP_BITS * 2; i > 0; i--)
+ ida_free(ida, i - 1);
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+
+ for (i = 0; i < IDA_BITMAP_BITS + BITS_PER_LONG - 4; i++)
+ IDA_BUG_ON(ida, ida_alloc(ida, GFP_KERNEL) != i);
+ for (i = IDA_BITMAP_BITS + BITS_PER_LONG - 4; i > 0; i--)
+ ida_free(ida, i - 1);
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+}
+
static int ida_checks(void)
{
DEFINE_IDA(ida);
@@ -74,6 +104,7 @@ static int ida_checks(void)
ida_check_leaf(&ida, 1024);
ida_check_leaf(&ida, 1024 * 64);
ida_check_max(&ida);
+ ida_check_conv(&ida);
printk("IDA: %u of %u tests passed\n", tests_passed, tests_run);
return (tests_run != tests_passed) ? 0 : -EINVAL;
diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index bd9699327f95..f948e38b9a6b 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -339,59 +339,23 @@ void ida_check_nomem(void)
/*
* Check handling of conversions between exceptional entries and full bitmaps.
*/
-void ida_check_conv(void)
+void ida_check_conv_user(void)
{
DEFINE_IDA(ida);
- int id;
unsigned long i;
- for (i = 0; i < IDA_BITMAP_BITS * 2; i += IDA_BITMAP_BITS) {
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new_above(&ida, i + 1, &id));
- assert(id == i + 1);
- assert(!ida_get_new_above(&ida, i + BITS_PER_LONG, &id));
- assert(id == i + BITS_PER_LONG);
- ida_remove(&ida, i + 1);
- ida_remove(&ida, i + BITS_PER_LONG);
- assert(ida_is_empty(&ida));
- }
-
- assert(ida_pre_get(&ida, GFP_KERNEL));
-
- for (i = 0; i < IDA_BITMAP_BITS * 2; i++) {
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new(&ida, &id));
- assert(id == i);
- }
-
- for (i = IDA_BITMAP_BITS * 2; i > 0; i--) {
- ida_remove(&ida, i - 1);
- }
- assert(ida_is_empty(&ida));
-
- for (i = 0; i < IDA_BITMAP_BITS + BITS_PER_LONG - 4; i++) {
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new(&ida, &id));
- assert(id == i);
- }
-
- for (i = IDA_BITMAP_BITS + BITS_PER_LONG - 4; i > 0; i--) {
- ida_remove(&ida, i - 1);
- }
- assert(ida_is_empty(&ida));
-
radix_tree_cpu_dead(1);
for (i = 0; i < 1000000; i++) {
- int err = ida_get_new(&ida, &id);
- if (err == -EAGAIN) {
- assert((i % IDA_BITMAP_BITS) == (BITS_PER_LONG - 2));
- assert(ida_pre_get(&ida, GFP_KERNEL));
- err = ida_get_new(&ida, &id);
+ int id = ida_alloc(&ida, GFP_NOWAIT);
+ if (id == -ENOMEM) {
+ IDA_BUG_ON(&ida, (i % IDA_BITMAP_BITS) !=
+ (BITS_PER_LONG - 2));
+ id = ida_alloc(&ida, GFP_KERNEL);
} else {
- assert((i % IDA_BITMAP_BITS) != (BITS_PER_LONG - 2));
+ IDA_BUG_ON(&ida, (i % IDA_BITMAP_BITS) ==
+ (BITS_PER_LONG - 2));
}
- assert(!err);
- assert(id == i);
+ IDA_BUG_ON(&ida, id != i);
}
ida_destroy(&ida);
}
@@ -507,7 +471,7 @@ void user_ida_checks(void)
ida_destroy(&ida);
assert(ida_is_empty(&ida));
- ida_check_conv();
+ ida_check_conv_user();
ida_check_random();
ida_simple_get_remove_test();
--
2.17.1
Reorder allocation to avoid an awkward lock/unlock/lock sequence.
Simpler code due to being able to use ida_alloc_max(), even if we can't
eliminate the driver's spinlock.
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 41 ++++++-------------
1 file changed, 12 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
index f2f9d88131f2..2d6291373efb 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
@@ -51,51 +51,34 @@ static int vmw_gmrid_man_get_node(struct ttm_mem_type_manager *man,
{
struct vmwgfx_gmrid_man *gman =
(struct vmwgfx_gmrid_man *)man->priv;
- int ret = 0;
int id;
mem->mm_node = NULL;
+ id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
+ if (id < 0)
+ return id;
+
spin_lock(&gman->lock);
if (gman->max_gmr_pages > 0) {
gman->used_gmr_pages += bo->num_pages;
if (unlikely(gman->used_gmr_pages > gman->max_gmr_pages))
- goto out_err_locked;
+ goto nospace;
}
- do {
- spin_unlock(&gman->lock);
- if (unlikely(ida_pre_get(&gman->gmr_ida, GFP_KERNEL) == 0)) {
- ret = -ENOMEM;
- goto out_err;
- }
- spin_lock(&gman->lock);
-
- ret = ida_get_new(&gman->gmr_ida, &id);
- if (unlikely(ret == 0 && id >= gman->max_gmr_ids)) {
- ida_remove(&gman->gmr_ida, id);
- ret = 0;
- goto out_err_locked;
- }
- } while (ret == -EAGAIN);
-
- if (likely(ret == 0)) {
- mem->mm_node = gman;
- mem->start = id;
- mem->num_pages = bo->num_pages;
- } else
- goto out_err_locked;
+ mem->mm_node = gman;
+ mem->start = id;
+ mem->num_pages = bo->num_pages;
spin_unlock(&gman->lock);
return 0;
-out_err:
- spin_lock(&gman->lock);
-out_err_locked:
+nospace:
gman->used_gmr_pages -= bo->num_pages;
spin_unlock(&gman->lock);
- return ret;
+ ida_free(&gman->gmr_ida, id);
+ return 0;
}
static void vmw_gmrid_man_put_node(struct ttm_mem_type_manager *man,
@@ -105,8 +88,8 @@ static void vmw_gmrid_man_put_node(struct ttm_mem_type_manager *man,
(struct vmwgfx_gmrid_man *)man->priv;
if (mem->mm_node) {
+ ida_free(&gman->gmr_ida, mem->start);
spin_lock(&gman->lock);
- ida_remove(&gman->gmr_ida, mem->start);
gman->used_gmr_pages -= mem->num_pages;
spin_unlock(&gman->lock);
mem->mm_node = NULL;
--
2.17.1
Convert to new API and move to kernel space. Take the opportunity to
test the situation a little more thoroughly (ie at different offsets).
Signed-off-by: Matthew Wilcox <[email protected]>
---
lib/test_ida.c | 24 ++++++++++++++++++++++++
tools/testing/radix-tree/idr-test.c | 27 ---------------------------
2 files changed, 24 insertions(+), 27 deletions(-)
diff --git a/lib/test_ida.c b/lib/test_ida.c
index 5a5a742d5f09..50dce991be6b 100644
--- a/lib/test_ida.c
+++ b/lib/test_ida.c
@@ -22,11 +22,35 @@ void ida_dump(struct ida *ida) { }
} \
} while (0)
+/*
+ * Check what happens when we fill a leaf and then delete it. This may
+ * discover mishandling of IDR_FREE.
+ */
+static void ida_check_leaf(struct ida *ida, unsigned int base)
+{
+ unsigned long i;
+
+ for (i = 0; i < IDA_BITMAP_BITS; i++) {
+ IDA_BUG_ON(ida, ida_alloc_min(ida, base, GFP_KERNEL) !=
+ base + i);
+ }
+
+ ida_destroy(ida);
+ assert(ida_is_empty(ida));
+
+ IDA_BUG_ON(ida, ida_alloc(ida, GFP_KERNEL) != 0);
+ ida_free(ida, 0);
+ IDA_BUG_ON(ida, !ida_is_empty(ida));
+}
+
static int ida_checks(void)
{
DEFINE_IDA(ida);
IDA_BUG_ON(&ida, !ida_is_empty(&ida));
+ ida_check_leaf(&ida, 0);
+ ida_check_leaf(&ida, 1024);
+ ida_check_leaf(&ida, 1024 * 64);
printk("IDA: %u of %u tests passed\n", tests_passed, tests_run);
return (tests_run != tests_passed) ? 0 : -EINVAL;
diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index 0f557784327d..fef1f45b927b 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -336,32 +336,6 @@ void ida_check_nomem(void)
IDA_BUG_ON(&ida, !ida_is_empty(&ida));
}
-/*
- * Check what happens when we fill a leaf and then delete it. This may
- * discover mishandling of IDR_FREE.
- */
-void ida_check_leaf(void)
-{
- DEFINE_IDA(ida);
- int id;
- unsigned long i;
-
- for (i = 0; i < IDA_BITMAP_BITS; i++) {
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new(&ida, &id));
- assert(id == i);
- }
-
- ida_destroy(&ida);
- assert(ida_is_empty(&ida));
-
- assert(ida_pre_get(&ida, GFP_KERNEL));
- assert(!ida_get_new(&ida, &id));
- assert(id == 0);
- ida_destroy(&ida);
- assert(ida_is_empty(&ida));
-}
-
/*
* Check handling of conversions between exceptional entries and full bitmaps.
*/
@@ -560,7 +534,6 @@ void user_ida_checks(void)
ida_destroy(&ida);
assert(ida_is_empty(&ida));
- ida_check_leaf();
ida_check_max();
ida_check_conv();
ida_check_random();
--
2.17.1
Signed-off-by: Matthew Wilcox <[email protected]>
---
net/core/net_namespace.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index a11e03f920d3..f447cebdcea3 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -973,22 +973,18 @@ static int register_pernet_operations(struct list_head *list,
int error;
if (ops->id) {
-again:
- error = ida_get_new_above(&net_generic_ids, MIN_PERNET_OPS_ID, ops->id);
- if (error < 0) {
- if (error == -EAGAIN) {
- ida_pre_get(&net_generic_ids, GFP_KERNEL);
- goto again;
- }
+ error = ida_alloc_min(&net_generic_ids, MIN_PERNET_OPS_ID,
+ GFP_KERNEL);
+ if (error < 0)
return error;
- }
+ *ops->id = error;
max_gen_ptrs = max(max_gen_ptrs, *ops->id + 1);
}
error = __register_pernet_operations(list, ops);
if (error) {
rcu_barrier();
if (ops->id)
- ida_remove(&net_generic_ids, *ops->id);
+ ida_free(&net_generic_ids, *ops->id);
}
return error;
@@ -999,7 +995,7 @@ static void unregister_pernet_operations(struct pernet_operations *ops)
__unregister_pernet_operations(ops);
rcu_barrier();
if (ops->id)
- ida_remove(&net_generic_ids, *ops->id);
+ ida_free(&net_generic_ids, *ops->id);
}
/**
--
2.17.1
Slightly simpler code.
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/scsi/osd/osd_uld.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index 0e56f1eb05dc..eaf36ccf58db 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -423,19 +423,11 @@ static int osd_probe(struct device *dev)
if (scsi_device->type != TYPE_OSD)
return -ENODEV;
- do {
- if (!ida_pre_get(&osd_minor_ida, GFP_KERNEL))
- return -ENODEV;
-
- error = ida_get_new(&osd_minor_ida, &minor);
- } while (error == -EAGAIN);
-
- if (error)
- return error;
- if (minor >= SCSI_OSD_MAX_MINOR) {
- error = -EBUSY;
- goto err_retract_minor;
- }
+ minor = ida_alloc_max(&osd_minor_ida, SCSI_OSD_MAX_MINOR, GFP_KERNEL);
+ if (minor == -ENOSPC)
+ return -EBUSY;
+ if (minor < 0)
+ return -ENODEV;
error = -ENOMEM;
oud = kzalloc(sizeof(*oud), GFP_KERNEL);
@@ -499,7 +491,7 @@ static int osd_probe(struct device *dev)
err_free_osd:
put_device(&oud->class_dev);
err_retract_minor:
- ida_remove(&osd_minor_ida, minor);
+ ida_free(&osd_minor_ida, minor);
return error;
}
@@ -514,7 +506,7 @@ static int osd_remove(struct device *dev)
}
cdev_device_del(&oud->cdev, &oud->class_dev);
- ida_remove(&osd_minor_ida, oud->minor);
+ ida_free(&osd_minor_ida, oud->minor);
put_device(&oud->class_dev);
return 0;
--
2.17.1
ida_alloc_max() matches what this driver wants to do. Also removes a
call to ida_pre_get(). We no longer need the protection of the mutex,
so convert pty_count to an atomic_t and remove the mutex entirely.
Signed-off-by: Matthew Wilcox <[email protected]>
---
fs/devpts/inode.c | 47 +++++++++++++----------------------------------
1 file changed, 13 insertions(+), 34 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index e072e955ce33..c53814539070 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -46,7 +46,7 @@ static int pty_limit = NR_UNIX98_PTY_DEFAULT;
static int pty_reserve = NR_UNIX98_PTY_RESERVE;
static int pty_limit_min;
static int pty_limit_max = INT_MAX;
-static int pty_count;
+static atomic_t pty_count = ATOMIC_INIT(0);
static struct ctl_table pty_table[] = {
{
@@ -93,8 +93,6 @@ static struct ctl_table pty_root_table[] = {
{}
};
-static DEFINE_MUTEX(allocated_ptys_lock);
-
struct pts_mount_opts {
int setuid;
int setgid;
@@ -533,44 +531,25 @@ static struct file_system_type devpts_fs_type = {
int devpts_new_index(struct pts_fs_info *fsi)
{
- int index;
- int ida_ret;
-
-retry:
- if (!ida_pre_get(&fsi->allocated_ptys, GFP_KERNEL))
- return -ENOMEM;
-
- mutex_lock(&allocated_ptys_lock);
- if (pty_count >= (pty_limit -
- (fsi->mount_opts.reserve ? 0 : pty_reserve))) {
- mutex_unlock(&allocated_ptys_lock);
- return -ENOSPC;
- }
+ int index = -ENOSPC;
- ida_ret = ida_get_new(&fsi->allocated_ptys, &index);
- if (ida_ret < 0) {
- mutex_unlock(&allocated_ptys_lock);
- if (ida_ret == -EAGAIN)
- goto retry;
- return -EIO;
- }
+ if (atomic_inc_return(&pty_count) >= (pty_limit -
+ (fsi->mount_opts.reserve ? 0 : pty_reserve)))
+ goto out;
- if (index >= fsi->mount_opts.max) {
- ida_remove(&fsi->allocated_ptys, index);
- mutex_unlock(&allocated_ptys_lock);
- return -ENOSPC;
- }
- pty_count++;
- mutex_unlock(&allocated_ptys_lock);
+ index = ida_alloc_max(&fsi->allocated_ptys, fsi->mount_opts.max - 1,
+ GFP_KERNEL);
+
+out:
+ if (index < 0)
+ atomic_dec(&pty_count);
return index;
}
void devpts_kill_index(struct pts_fs_info *fsi, int idx)
{
- mutex_lock(&allocated_ptys_lock);
- ida_remove(&fsi->allocated_ptys, idx);
- pty_count--;
- mutex_unlock(&allocated_ptys_lock);
+ ida_free(&fsi->allocated_ptys, idx);
+ atomic_dec(&pty_count);
}
/**
--
2.17.1
ida_alloc_range is the perfect fit for this use case. Eliminates
a custom spinlock, a call to ida_pre_get and a local check for the
allocated ID exceeding a maximum.
Signed-off-by: Matthew Wilcox <[email protected]>
---
arch/powerpc/mm/mmu_context_book3s64.c | 44 +++-----------------------
1 file changed, 4 insertions(+), 40 deletions(-)
diff --git a/arch/powerpc/mm/mmu_context_book3s64.c b/arch/powerpc/mm/mmu_context_book3s64.c
index f3d4b4a0e561..5a0cf2cc8ba0 100644
--- a/arch/powerpc/mm/mmu_context_book3s64.c
+++ b/arch/powerpc/mm/mmu_context_book3s64.c
@@ -26,48 +26,16 @@
#include <asm/mmu_context.h>
#include <asm/pgalloc.h>
-static DEFINE_SPINLOCK(mmu_context_lock);
static DEFINE_IDA(mmu_context_ida);
static int alloc_context_id(int min_id, int max_id)
{
- int index, err;
-
-again:
- if (!ida_pre_get(&mmu_context_ida, GFP_KERNEL))
- return -ENOMEM;
-
- spin_lock(&mmu_context_lock);
- err = ida_get_new_above(&mmu_context_ida, min_id, &index);
- spin_unlock(&mmu_context_lock);
-
- if (err == -EAGAIN)
- goto again;
- else if (err)
- return err;
-
- if (index > max_id) {
- spin_lock(&mmu_context_lock);
- ida_remove(&mmu_context_ida, index);
- spin_unlock(&mmu_context_lock);
- return -ENOMEM;
- }
-
- return index;
+ return ida_alloc_range(&mmu_context_ida, min_id, max_id, GFP_KERNEL);
}
void hash__reserve_context_id(int id)
{
- int rc, result = 0;
-
- do {
- if (!ida_pre_get(&mmu_context_ida, GFP_KERNEL))
- break;
-
- spin_lock(&mmu_context_lock);
- rc = ida_get_new_above(&mmu_context_ida, id, &result);
- spin_unlock(&mmu_context_lock);
- } while (rc == -EAGAIN);
+ int result = ida_alloc_range(&mmu_context_ida, id, id, GFP_KERNEL);
WARN(result != id, "mmu: Failed to reserve context id %d (rc %d)\n", id, result);
}
@@ -172,9 +140,7 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
void __destroy_context(int context_id)
{
- spin_lock(&mmu_context_lock);
- ida_remove(&mmu_context_ida, context_id);
- spin_unlock(&mmu_context_lock);
+ ida_free(&mmu_context_ida, context_id);
}
EXPORT_SYMBOL_GPL(__destroy_context);
@@ -182,13 +148,11 @@ static void destroy_contexts(mm_context_t *ctx)
{
int index, context_id;
- spin_lock(&mmu_context_lock);
for (index = 0; index < ARRAY_SIZE(ctx->extended_id); index++) {
context_id = ctx->extended_id[index];
if (context_id)
- ida_remove(&mmu_context_ida, context_id);
+ ida_free(&mmu_context_ida, context_id);
}
- spin_unlock(&mmu_context_lock);
}
static void pte_frag_destroy(void *pte_frag)
--
2.17.1
Since the session is never looked up by ID, we can use the more
space-efficient IDA instead of the IDR. I think I found a bug where
we never reuse session ID 0, but there may be a good reason for it,
so I've merely documented it in this patch. Not reusing session ID 0
doesn't even leak memory.
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/target/iscsi/iscsi_target.c | 10 ++--------
drivers/target/iscsi/iscsi_target.h | 4 +---
drivers/target/iscsi/iscsi_target_login.c | 20 ++++++--------------
3 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 8e223799347a..94bad43c41ff 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -57,9 +57,8 @@ static DEFINE_SPINLOCK(tiqn_lock);
static DEFINE_MUTEX(np_lock);
static struct idr tiqn_idr;
-struct idr sess_idr;
+DEFINE_IDA(sess_ida);
struct mutex auth_id_lock;
-spinlock_t sess_idr_lock;
struct iscsit_global *iscsit_global;
@@ -700,9 +699,7 @@ static int __init iscsi_target_init_module(void)
spin_lock_init(&iscsit_global->ts_bitmap_lock);
mutex_init(&auth_id_lock);
- spin_lock_init(&sess_idr_lock);
idr_init(&tiqn_idr);
- idr_init(&sess_idr);
ret = target_register_template(&iscsi_ops);
if (ret)
@@ -4375,10 +4372,7 @@ int iscsit_close_session(struct iscsi_session *sess)
pr_debug("Decremented number of active iSCSI Sessions on"
" iSCSI TPG: %hu to %u\n", tpg->tpgt, tpg->nsessions);
- spin_lock(&sess_idr_lock);
- idr_remove(&sess_idr, sess->session_index);
- spin_unlock(&sess_idr_lock);
-
+ ida_free(&sess_ida, sess->session_index);
kfree(sess->sess_ops);
sess->sess_ops = NULL;
spin_unlock_bh(&se_tpg->session_lock);
diff --git a/drivers/target/iscsi/iscsi_target.h b/drivers/target/iscsi/iscsi_target.h
index 42de1843aa40..48bac0acf8c7 100644
--- a/drivers/target/iscsi/iscsi_target.h
+++ b/drivers/target/iscsi/iscsi_target.h
@@ -55,9 +55,7 @@ extern struct kmem_cache *lio_ooo_cache;
extern struct kmem_cache *lio_qr_cache;
extern struct kmem_cache *lio_r2t_cache;
-extern struct idr sess_idr;
+extern struct ida sess_ida;
extern struct mutex auth_id_lock;
-extern spinlock_t sess_idr_lock;
-
#endif /*** ISCSI_TARGET_H ***/
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 99501785cdc1..561d2ad38989 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -336,22 +336,16 @@ static int iscsi_login_zero_tsih_s1(
timer_setup(&sess->time2retain_timer,
iscsit_handle_time2retain_timeout, 0);
- idr_preload(GFP_KERNEL);
- spin_lock_bh(&sess_idr_lock);
- ret = idr_alloc(&sess_idr, NULL, 0, 0, GFP_NOWAIT);
- if (ret >= 0)
- sess->session_index = ret;
- spin_unlock_bh(&sess_idr_lock);
- idr_preload_end();
-
+ ret = ida_alloc(&sess_ida, GFP_KERNEL);
if (ret < 0) {
- pr_err("idr_alloc() for sess_idr failed\n");
+ pr_err("Session ID allocation failed %d\n", ret);
iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
ISCSI_LOGIN_STATUS_NO_RESOURCES);
kfree(sess);
return -ENOMEM;
}
+ sess->session_index = ret;
sess->creation_time = get_jiffies_64();
/*
* The FFP CmdSN window values will be allocated from the TPG's
@@ -1163,11 +1157,9 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn,
goto old_sess_out;
if (conn->sess->se_sess)
transport_free_session(conn->sess->se_sess);
- if (conn->sess->session_index != 0) {
- spin_lock_bh(&sess_idr_lock);
- idr_remove(&sess_idr, conn->sess->session_index);
- spin_unlock_bh(&sess_idr_lock);
- }
+ /* Um, 0 is a valid ID. I suppose we never free it? */
+ if (conn->sess->session_index != 0)
+ ida_free(&sess_ida, conn->sess->session_index);
kfree(conn->sess->sess_ops);
kfree(conn->sess);
conn->sess = NULL;
--
2.17.1
Simpler and shorter code.
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/dma/dmaengine.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 08ba8473a284..1e9bdadfc312 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -161,9 +161,7 @@ static void chan_dev_release(struct device *dev)
chan_dev = container_of(dev, typeof(*chan_dev), device);
if (atomic_dec_and_test(chan_dev->idr_ref)) {
- mutex_lock(&dma_list_mutex);
- ida_remove(&dma_ida, chan_dev->dev_id);
- mutex_unlock(&dma_list_mutex);
+ ida_free(&dma_ida, chan_dev->dev_id);
kfree(chan_dev->idr_ref);
}
kfree(chan_dev);
@@ -896,16 +894,10 @@ static bool device_has_all_tx_types(struct dma_device *device)
static int get_dma_id(struct dma_device *device)
{
- int rc;
-
- do {
- if (!ida_pre_get(&dma_ida, GFP_KERNEL))
- return -ENOMEM;
- mutex_lock(&dma_list_mutex);
- rc = ida_get_new(&dma_ida, &device->dev_id);
- mutex_unlock(&dma_list_mutex);
- } while (rc == -EAGAIN);
+ int rc = ida_alloc(&dma_ida, GFP_KERNEL);
+ if (rc >= 0)
+ device->dev_id = rc;
return rc;
}
@@ -1090,9 +1082,7 @@ int dma_async_device_register(struct dma_device *device)
err_out:
/* if we never registered a channel just release the idr */
if (atomic_read(idr_ref) == 0) {
- mutex_lock(&dma_list_mutex);
- ida_remove(&dma_ida, device->dev_id);
- mutex_unlock(&dma_list_mutex);
+ ida_free(&dma_ida, device->dev_id);
kfree(idr_ref);
return rc;
}
--
2.17.1
Removes a use of ida_pre_get() and a personalised spinlock.
Signed-off-by: Matthew Wilcox <[email protected]>
---
drivers/block/mtip32xx/mtip32xx.c | 29 ++++++-----------------------
1 file changed, 6 insertions(+), 23 deletions(-)
diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index c73626decb46..2b6d6bce76df 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -118,7 +118,6 @@ static struct dentry *dfs_device_status;
static u32 cpu_use[NR_CPUS];
-static DEFINE_SPINLOCK(rssd_index_lock);
static DEFINE_IDA(rssd_index_ida);
static int mtip_block_initialize(struct driver_data *dd);
@@ -3768,20 +3767,10 @@ static int mtip_block_initialize(struct driver_data *dd)
goto alloc_disk_error;
}
- /* Generate the disk name, implemented same as in sd.c */
- do {
- if (!ida_pre_get(&rssd_index_ida, GFP_KERNEL)) {
- rv = -ENOMEM;
- goto ida_get_error;
- }
-
- spin_lock(&rssd_index_lock);
- rv = ida_get_new(&rssd_index_ida, &index);
- spin_unlock(&rssd_index_lock);
- } while (rv == -EAGAIN);
-
- if (rv)
+ rv = ida_alloc(&rssd_index_ida, GFP_KERNEL);
+ if (rv < 0)
goto ida_get_error;
+ index = rv;
rv = rssd_disk_name_format("rssd",
index,
@@ -3923,9 +3912,7 @@ static int mtip_block_initialize(struct driver_data *dd)
block_queue_alloc_tag_error:
mtip_hw_debugfs_exit(dd);
disk_index_error:
- spin_lock(&rssd_index_lock);
- ida_remove(&rssd_index_ida, index);
- spin_unlock(&rssd_index_lock);
+ ida_free(&rssd_index_ida, index);
ida_get_error:
put_disk(dd->disk);
@@ -4013,9 +4000,7 @@ static int mtip_block_remove(struct driver_data *dd)
}
dd->disk = NULL;
- spin_lock(&rssd_index_lock);
- ida_remove(&rssd_index_ida, dd->index);
- spin_unlock(&rssd_index_lock);
+ ida_free(&rssd_index_ida, dd->index);
/* De-initialize the protocol layer. */
mtip_hw_exit(dd);
@@ -4055,9 +4040,7 @@ static int mtip_block_shutdown(struct driver_data *dd)
dd->queue = NULL;
}
- spin_lock(&rssd_index_lock);
- ida_remove(&rssd_index_ida, dd->index);
- spin_unlock(&rssd_index_lock);
+ ida_free(&rssd_index_ida, dd->index);
return 0;
}
--
2.17.1
We can't move this test to kernel space because there's no way to
force kmalloc to fail. But we can use the new API and check this
works when the test is in userspace.
Signed-off-by: Matthew Wilcox <[email protected]>
---
tools/testing/radix-tree/idr-test.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index 604b51dc9b38..0f557784327d 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -320,19 +320,20 @@ void ida_dump(struct ida *);
/*
* Check that we get the correct error when we run out of memory doing
- * allocations. To ensure we run out of memory, just "forget" to preload.
+ * allocations. In userspace, GFP_NOWAIT will always fail an allocation.
* The first test is for not having a bitmap available, and the second test
* is for not being able to allocate a level of the radix tree.
*/
void ida_check_nomem(void)
{
DEFINE_IDA(ida);
- int id, err;
+ int id;
- err = ida_get_new_above(&ida, 256, &id);
- assert(err == -EAGAIN);
- err = ida_get_new_above(&ida, 1UL << 30, &id);
- assert(err == -EAGAIN);
+ id = ida_alloc_min(&ida, 256, GFP_NOWAIT);
+ IDA_BUG_ON(&ida, id != -ENOMEM);
+ id = ida_alloc_min(&ida, 1UL << 30, GFP_NOWAIT);
+ IDA_BUG_ON(&ida, id != -ENOMEM);
+ IDA_BUG_ON(&ida, !ida_is_empty(&ida));
}
/*
--
2.17.1
The new API is much easier for this user. Also add kerneldoc for
get_anon_bdev().
Signed-off-by: Matthew Wilcox <[email protected]>
---
fs/super.c | 63 +++++++++++++++++++-----------------------------------
1 file changed, 22 insertions(+), 41 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index 50728d9c1a05..3e7a0aea716a 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -978,58 +978,42 @@ void emergency_thaw_all(void)
}
}
-/*
- * Unnamed block devices are dummy devices used by virtual
- * filesystems which don't use real block-devices. -- jrs
- */
-
static DEFINE_IDA(unnamed_dev_ida);
-static DEFINE_SPINLOCK(unnamed_dev_lock);/* protects the above */
-/* Many userspace utilities consider an FSID of 0 invalid.
- * Always return at least 1 from get_anon_bdev.
- */
-static int unnamed_dev_start = 1;
+/**
+ * get_anon_bdev - Allocate a block device for filesystems which don't have one.
+ * @p: Pointer to a dev_t.
+ *
+ * Filesystems which don't use real block devices can call this function
+ * to allocate a virtual block device.
+ *
+ * Context: Any context. Frequently called while holding sb_lock.
+ * Return: 0 on success, -EMFILE if there are no anonymous bdevs left
+ * or -EAGAIN if memory allocation failed.
+ */
int get_anon_bdev(dev_t *p)
{
int dev;
- int error;
- retry:
- if (ida_pre_get(&unnamed_dev_ida, GFP_ATOMIC) == 0)
- return -ENOMEM;
- spin_lock(&unnamed_dev_lock);
- error = ida_get_new_above(&unnamed_dev_ida, unnamed_dev_start, &dev);
- if (!error)
- unnamed_dev_start = dev + 1;
- spin_unlock(&unnamed_dev_lock);
- if (error == -EAGAIN)
- /* We raced and lost with another CPU. */
- goto retry;
- else if (error)
+ /*
+ * Many userspace utilities consider an FSID of 0 invalid.
+ * Always return at least 1 from get_anon_bdev.
+ */
+ dev = ida_alloc_range(&unnamed_dev_ida, 1, (1 << MINORBITS) - 1,
+ GFP_ATOMIC);
+ if (dev == -ENOSPC)
+ return -EMFILE;
+ if (dev < 0)
return -EAGAIN;
- if (dev >= (1 << MINORBITS)) {
- spin_lock(&unnamed_dev_lock);
- ida_remove(&unnamed_dev_ida, dev);
- if (unnamed_dev_start > dev)
- unnamed_dev_start = dev;
- spin_unlock(&unnamed_dev_lock);
- return -EMFILE;
- }
- *p = MKDEV(0, dev & MINORMASK);
+ *p = MKDEV(0, dev);
return 0;
}
EXPORT_SYMBOL(get_anon_bdev);
void free_anon_bdev(dev_t dev)
{
- int slot = MINOR(dev);
- spin_lock(&unnamed_dev_lock);
- ida_remove(&unnamed_dev_ida, slot);
- if (slot < unnamed_dev_start)
- unnamed_dev_start = slot;
- spin_unlock(&unnamed_dev_lock);
+ ida_free(&unnamed_dev_ida, MINOR(dev));
}
EXPORT_SYMBOL(free_anon_bdev);
@@ -1037,7 +1021,6 @@ int set_anon_super(struct super_block *s, void *data)
{
return get_anon_bdev(&s->s_dev);
}
-
EXPORT_SYMBOL(set_anon_super);
void kill_anon_super(struct super_block *sb)
@@ -1046,7 +1029,6 @@ void kill_anon_super(struct super_block *sb)
generic_shutdown_super(sb);
free_anon_bdev(dev);
}
-
EXPORT_SYMBOL(kill_anon_super);
void kill_litter_super(struct super_block *sb)
@@ -1055,7 +1037,6 @@ void kill_litter_super(struct super_block *sb)
d_genocide(sb->s_root);
kill_anon_super(sb);
}
-
EXPORT_SYMBOL(kill_litter_super);
static int ns_test_super(struct super_block *sb, void *data)
--
2.17.1
The user has no need to handle locking between ida_simple_get() and
ida_simple_remove(). They shouldn't be forced to think about whether
ida_destroy() might be called at the same time as any of their other
IDA manipulation calls. Improve the documnetation while I'm in here.
Signed-off-by: Matthew Wilcox <[email protected]>
---
lib/idr.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/lib/idr.c b/lib/idr.c
index ed9c169c12bd..352c6160e2e0 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -523,25 +523,30 @@ void ida_remove(struct ida *ida, int id)
EXPORT_SYMBOL(ida_remove);
/**
- * ida_destroy - Free the contents of an ida
- * @ida: ida handle
+ * ida_destroy() - Free all IDs.
+ * @ida: IDA handle.
+ *
+ * Calling this function frees all IDs and releases all resources used
+ * by an IDA. When this call returns, the IDA is empty and can be reused
+ * or freed. If the IDA is already empty, there is no need to call this
+ * function.
*
- * Calling this function releases all resources associated with an IDA. When
- * this call returns, the IDA is empty and can be reused or freed. The caller
- * should not allow ida_remove() or ida_get_new_above() to be called at the
- * same time.
+ * Context: Any context.
*/
void ida_destroy(struct ida *ida)
{
+ unsigned long flags;
struct radix_tree_iter iter;
void __rcu **slot;
+ xa_lock_irqsave(&ida->ida_rt, flags);
radix_tree_for_each_slot(slot, &ida->ida_rt, &iter, 0) {
struct ida_bitmap *bitmap = rcu_dereference_raw(*slot);
if (!radix_tree_exception(bitmap))
kfree(bitmap);
radix_tree_iter_delete(&ida->ida_rt, &iter, slot);
}
+ xa_unlock_irqrestore(&ida->ida_rt, flags);
}
EXPORT_SYMBOL(ida_destroy);
--
2.17.1
Add ida_alloc(), ida_alloc_min(), ida_alloc_max(), ida_alloc_range()
and ida_free(). The ida_alloc_max() and ida_alloc_range() functions
differ from ida_simple_get() in that they take an inclusive 'max'
parameter instead of an exclusive 'end' parameter. Callers are about
evenly split whether they'd like inclusive or exclusive parameters and
'max' is easier to document than 'end'.
Change the IDA allocation to first attempt to allocate a bit using
existing memory, and only allocate memory afterwards. Also change the
behaviour of 'min' > INT_MAX from being a BUG() to returning -ENOSPC.
Leave compatibility wrappers in place for ida_simple_get() and
ida_simple_remove() to avoid changing all callers.
Signed-off-by: Matthew Wilcox <[email protected]>
---
include/linux/idr.h | 59 ++++++++++++++++++++++++++++++++++++--
lib/idr.c | 70 ++++++++++++++++++++-------------------------
2 files changed, 87 insertions(+), 42 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index e856f4e0ab35..cd339da0b1aa 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -230,15 +230,68 @@ int ida_get_new_above(struct ida *ida, int starting_id, int *p_id);
void ida_remove(struct ida *ida, int id);
void ida_destroy(struct ida *ida);
-int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
- gfp_t gfp_mask);
-void ida_simple_remove(struct ida *ida, unsigned int id);
+int ida_alloc_range(struct ida *, unsigned int min, unsigned int max, gfp_t);
+void ida_free(struct ida *, unsigned int id);
+
+/**
+ * ida_alloc() - Allocate an unused ID.
+ * @ida: IDA handle.
+ * @gfp: Memory allocation flags.
+ *
+ * Allocate an ID between 0 and %INT_MAX, inclusive.
+ *
+ * Context: Any context.
+ * Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
+ * or %-ENOSPC if there are no free IDs.
+ */
+static inline int ida_alloc(struct ida *ida, gfp_t gfp)
+{
+ return ida_alloc_range(ida, 0, ~0, gfp);
+}
+
+/**
+ * ida_alloc_min() - Allocate an unused ID.
+ * @ida: IDA handle.
+ * @min: Lowest ID to allocate.
+ * @gfp: Memory allocation flags.
+ *
+ * Allocate an ID between @min and %INT_MAX, inclusive.
+ *
+ * Context: Any context.
+ * Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
+ * or %-ENOSPC if there are no free IDs.
+ */
+static inline int ida_alloc_min(struct ida *ida, unsigned int min, gfp_t gfp)
+{
+ return ida_alloc_range(ida, min, ~0, gfp);
+}
+
+/**
+ * ida_alloc_max() - Allocate an unused ID.
+ * @ida: IDA handle.
+ * @max: Highest ID to allocate.
+ * @gfp: Memory allocation flags.
+ *
+ * Allocate an ID between 0 and @max, inclusive.
+ *
+ * Context: Any context.
+ * Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
+ * or %-ENOSPC if there are no free IDs.
+ */
+static inline int ida_alloc_max(struct ida *ida, unsigned int max, gfp_t gfp)
+{
+ return ida_alloc_range(ida, 0, max, gfp);
+}
static inline void ida_init(struct ida *ida)
{
INIT_RADIX_TREE(&ida->ida_rt, IDR_RT_MARKER | GFP_NOWAIT);
}
+#define ida_simple_get(ida, start, end, gfp) \
+ ida_alloc_range(ida, start, (end) - 1, gfp)
+#define ida_simple_remove(ida, id) ida_free(ida, id)
+
/**
* ida_get_new - allocate new ID
* @ida: idr handle
diff --git a/lib/idr.c b/lib/idr.c
index 352c6160e2e0..8b3a5e7eb734 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -317,7 +317,8 @@ EXPORT_SYMBOL(idr_replace);
* bit per ID, and so is more space efficient than an IDR. To use an IDA,
* define it using DEFINE_IDA() (or embed a &struct ida in a data structure,
* then initialise it using ida_init()). To allocate a new ID, call
- * ida_simple_get(). To free an ID, call ida_simple_remove().
+ * ida_alloc(), ida_alloc_max() or ida_alloc_range(). To free an ID, call
+ * ida_free().
*
* If you have more complex locking requirements, use a loop around
* ida_pre_get() and ida_get_new() to allocate a new ID. Then use
@@ -378,7 +379,7 @@ EXPORT_SYMBOL(idr_replace);
* Allocate new ID above or equal to @start. It should be called
* with any required locks to ensure that concurrent calls to
* ida_get_new_above() / ida_get_new() / ida_remove() are not allowed.
- * Consider using ida_simple_get() if you do not have complex locking
+ * Consider using ida_alloc_range() if you do not have complex locking
* requirements.
*
* If memory is required, it will return %-EAGAIN, you should unlock
@@ -551,43 +552,34 @@ void ida_destroy(struct ida *ida)
EXPORT_SYMBOL(ida_destroy);
/**
- * ida_simple_get - get a new id.
- * @ida: the (initialized) ida.
- * @start: the minimum id (inclusive, < 0x8000000)
- * @end: the maximum id (exclusive, < 0x8000000 or 0)
- * @gfp_mask: memory allocation flags
- *
- * Allocates an id in the range start <= id < end, or returns -ENOSPC.
- * On memory allocation failure, returns -ENOMEM.
+ * ida_alloc_range() - Allocate an unused ID.
+ * @ida: IDA handle.
+ * @min: Lowest ID to allocate.
+ * @max: Highest ID to allocate.
+ * @gfp: Memory allocation flags.
*
- * Compared to ida_get_new_above() this function does its own locking, and
- * should be used unless there are special requirements.
+ * Allocate an ID between @min and @max, inclusive. The allocated ID will
+ * not exceed %INT_MAX, even if @max is larger.
*
- * Use ida_simple_remove() to get rid of an id.
+ * Context: Any context.
+ * Return: The allocated ID, or %-ENOMEM if memory could not be allocated,
+ * or %-ENOSPC if there are no free IDs.
*/
-int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
- gfp_t gfp_mask)
+int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max,
+ gfp_t gfp)
{
int ret, id;
- unsigned int max;
unsigned long flags;
- BUG_ON((int)start < 0);
- BUG_ON((int)end < 0);
+ if ((int)min < 0)
+ return -ENOSPC;
- if (end == 0)
- max = 0x80000000;
- else {
- BUG_ON(end < start);
- max = end - 1;
- }
+ if ((int)max < 0)
+ max = INT_MAX;
again:
- if (!ida_pre_get(ida, gfp_mask))
- return -ENOMEM;
-
xa_lock_irqsave(&ida->ida_rt, flags);
- ret = ida_get_new_above(ida, start, &id);
+ ret = ida_get_new_above(ida, min, &id);
if (!ret) {
if (id > max) {
ida_remove(ida, id);
@@ -598,24 +590,24 @@ int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
}
xa_unlock_irqrestore(&ida->ida_rt, flags);
- if (unlikely(ret == -EAGAIN))
+ if (unlikely(ret == -EAGAIN)) {
+ if (!ida_pre_get(ida, gfp))
+ return -ENOMEM;
goto again;
+ }
return ret;
}
-EXPORT_SYMBOL(ida_simple_get);
+EXPORT_SYMBOL(ida_alloc_range);
/**
- * ida_simple_remove - remove an allocated id.
- * @ida: the (initialized) ida.
- * @id: the id returned by ida_simple_get.
- *
- * Use to release an id allocated with ida_simple_get().
+ * ida_free() - Release an allocated ID.
+ * @ida: IDA handle.
+ * @id: Previously allocated ID.
*
- * Compared to ida_remove() this function does its own locking, and should be
- * used unless there are special requirements.
+ * Context: Any context.
*/
-void ida_simple_remove(struct ida *ida, unsigned int id)
+void ida_free(struct ida *ida, unsigned int id)
{
unsigned long flags;
@@ -624,4 +616,4 @@ void ida_simple_remove(struct ida *ida, unsigned int id)
ida_remove(ida, id);
xa_unlock_irqrestore(&ida->ida_rt, flags);
}
-EXPORT_SYMBOL(ida_simple_remove);
+EXPORT_SYMBOL(ida_free);
--
2.17.1
Now that idr.c includes xarray.h, we need to have an xarray.h to include
in the test suite.
Signed-off-by: Matthew Wilcox <[email protected]>
---
tools/testing/radix-tree/linux/xarray.h | 2 ++
1 file changed, 2 insertions(+)
create mode 100644 tools/testing/radix-tree/linux/xarray.h
diff --git a/tools/testing/radix-tree/linux/xarray.h b/tools/testing/radix-tree/linux/xarray.h
new file mode 100644
index 000000000000..df3812cda376
--- /dev/null
+++ b/tools/testing/radix-tree/linux/xarray.h
@@ -0,0 +1,2 @@
+#include "generated/map-shift.h"
+#include "../../../../include/linux/xarray.h"
--
2.17.1
On Thu, Jun 21, 2018 at 02:28:20PM -0700, Matthew Wilcox wrote:
> Eliminates the custom spinlock and the call to ida_pre_get.
>
> Signed-off-by: Matthew Wilcox <[email protected]>
Acked-by: Micha? Miros?aw <[email protected]>
On Thu, 21 Jun 2018 14:28:22 -0700
Matthew Wilcox <[email protected]> wrote:
> ida_alloc_range is the perfect fit for this use case. Eliminates
> a custom spinlock, a call to ida_pre_get and a local check for the
> allocated ID exceeding a maximum.
>
> Signed-off-by: Matthew Wilcox <[email protected]>
> ---
> arch/powerpc/mm/mmu_context_book3s64.c | 44 +++-----------------------
> 1 file changed, 4 insertions(+), 40 deletions(-)
>
> diff --git a/arch/powerpc/mm/mmu_context_book3s64.c b/arch/powerpc/mm/mmu_context_book3s64.c
> index f3d4b4a0e561..5a0cf2cc8ba0 100644
> --- a/arch/powerpc/mm/mmu_context_book3s64.c
> +++ b/arch/powerpc/mm/mmu_context_book3s64.c
> @@ -26,48 +26,16 @@
> #include <asm/mmu_context.h>
> #include <asm/pgalloc.h>
>
> -static DEFINE_SPINLOCK(mmu_context_lock);
> static DEFINE_IDA(mmu_context_ida);
>
> static int alloc_context_id(int min_id, int max_id)
> {
> - int index, err;
> -
> -again:
> - if (!ida_pre_get(&mmu_context_ida, GFP_KERNEL))
> - return -ENOMEM;
> -
> - spin_lock(&mmu_context_lock);
> - err = ida_get_new_above(&mmu_context_ida, min_id, &index);
> - spin_unlock(&mmu_context_lock);
> -
> - if (err == -EAGAIN)
> - goto again;
> - else if (err)
> - return err;
> -
> - if (index > max_id) {
> - spin_lock(&mmu_context_lock);
> - ida_remove(&mmu_context_ida, index);
> - spin_unlock(&mmu_context_lock);
> - return -ENOMEM;
> - }
> -
> - return index;
> + return ida_alloc_range(&mmu_context_ida, min_id, max_id, GFP_KERNEL);
> }
>
> void hash__reserve_context_id(int id)
> {
> - int rc, result = 0;
> -
> - do {
> - if (!ida_pre_get(&mmu_context_ida, GFP_KERNEL))
> - break;
> -
> - spin_lock(&mmu_context_lock);
> - rc = ida_get_new_above(&mmu_context_ida, id, &result);
> - spin_unlock(&mmu_context_lock);
> - } while (rc == -EAGAIN);
> + int result = ida_alloc_range(&mmu_context_ida, id, id, GFP_KERNEL);
>
> WARN(result != id, "mmu: Failed to reserve context id %d (rc %d)\n", id, result);
> }
> @@ -172,9 +140,7 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
>
> void __destroy_context(int context_id)
> {
> - spin_lock(&mmu_context_lock);
> - ida_remove(&mmu_context_ida, context_id);
> - spin_unlock(&mmu_context_lock);
> + ida_free(&mmu_context_ida, context_id);
> }
> EXPORT_SYMBOL_GPL(__destroy_context);
>
> @@ -182,13 +148,11 @@ static void destroy_contexts(mm_context_t *ctx)
> {
> int index, context_id;
>
> - spin_lock(&mmu_context_lock);
> for (index = 0; index < ARRAY_SIZE(ctx->extended_id); index++) {
> context_id = ctx->extended_id[index];
> if (context_id)
> - ida_remove(&mmu_context_ida, context_id);
> + ida_free(&mmu_context_ida, context_id);
> }
> - spin_unlock(&mmu_context_lock);
> }
>
> static void pte_frag_destroy(void *pte_frag)
This hunk should be okay because the mmu_context_lock does not protect
the extended_id array, right Aneesh?
Reviewed-by: Nicholas Piggin <[email protected]>
Thanks,
Nick
On Fri, Jun 22, 2018 at 12:15:11PM +1000, Nicholas Piggin wrote:
> On Thu, 21 Jun 2018 14:28:22 -0700
> Matthew Wilcox <[email protected]> wrote:
> > static int alloc_context_id(int min_id, int max_id)
...
> > - spin_lock(&mmu_context_lock);
> > - err = ida_get_new_above(&mmu_context_ida, min_id, &index);
> > - spin_unlock(&mmu_context_lock);
...
> > @@ -182,13 +148,11 @@ static void destroy_contexts(mm_context_t *ctx)
> > {
> > int index, context_id;
> >
> > - spin_lock(&mmu_context_lock);
> > for (index = 0; index < ARRAY_SIZE(ctx->extended_id); index++) {
> > context_id = ctx->extended_id[index];
> > if (context_id)
> > - ida_remove(&mmu_context_ida, context_id);
> > + ida_free(&mmu_context_ida, context_id);
> > }
> > - spin_unlock(&mmu_context_lock);
> > }
> >
> > static void pte_frag_destroy(void *pte_frag)
>
> This hunk should be okay because the mmu_context_lock does not protect
> the extended_id array, right Aneesh?
That's my understanding. The code today does this:
static inline int alloc_extended_context(struct mm_struct *mm,
unsigned long ea)
{
int context_id;
int index = ea >> MAX_EA_BITS_PER_CONTEXT;
context_id = hash__alloc_context_id();
if (context_id < 0)
return context_id;
VM_WARN_ON(mm->context.extended_id[index]);
mm->context.extended_id[index] = context_id;
so it's not currently protected by this lock. I suppose we are currently
protected from destroy_contexts() being called twice simultaneously, but
you'll notice that we don't zero the array elements in destroy_contexts(),
so if we somehow had a code path which could call it concurrently, we'd
be seeing warnings when the second caller tried to remove the context
IDs from the IDA. I deduced that something else must be preventing
this situation from occurring (like, oh i don't know, this function only
being called on process exit, so implicitly only called once per context).
> Reviewed-by: Nicholas Piggin <[email protected]>
Thanks.
On Thu, 21 Jun 2018 21:38:15 -0700
Matthew Wilcox <[email protected]> wrote:
> On Fri, Jun 22, 2018 at 12:15:11PM +1000, Nicholas Piggin wrote:
> > On Thu, 21 Jun 2018 14:28:22 -0700
> > Matthew Wilcox <[email protected]> wrote:
> > > static int alloc_context_id(int min_id, int max_id)
> ...
> > > - spin_lock(&mmu_context_lock);
> > > - err = ida_get_new_above(&mmu_context_ida, min_id, &index);
> > > - spin_unlock(&mmu_context_lock);
> ...
> > > @@ -182,13 +148,11 @@ static void destroy_contexts(mm_context_t *ctx)
> > > {
> > > int index, context_id;
> > >
> > > - spin_lock(&mmu_context_lock);
> > > for (index = 0; index < ARRAY_SIZE(ctx->extended_id); index++) {
> > > context_id = ctx->extended_id[index];
> > > if (context_id)
> > > - ida_remove(&mmu_context_ida, context_id);
> > > + ida_free(&mmu_context_ida, context_id);
> > > }
> > > - spin_unlock(&mmu_context_lock);
> > > }
> > >
> > > static void pte_frag_destroy(void *pte_frag)
> >
> > This hunk should be okay because the mmu_context_lock does not protect
> > the extended_id array, right Aneesh?
>
> That's my understanding. The code today does this:
>
> static inline int alloc_extended_context(struct mm_struct *mm,
> unsigned long ea)
> {
> int context_id;
>
> int index = ea >> MAX_EA_BITS_PER_CONTEXT;
>
> context_id = hash__alloc_context_id();
> if (context_id < 0)
> return context_id;
>
> VM_WARN_ON(mm->context.extended_id[index]);
> mm->context.extended_id[index] = context_id;
>
> so it's not currently protected by this lock. I suppose we are currently
> protected from destroy_contexts() being called twice simultaneously, but
> you'll notice that we don't zero the array elements in destroy_contexts(),
> so if we somehow had a code path which could call it concurrently, we'd
> be seeing warnings when the second caller tried to remove the context
Yeah that'd be an existing bug.
> IDs from the IDA. I deduced that something else must be preventing
> this situation from occurring (like, oh i don't know, this function only
> being called on process exit, so implicitly only called once per context).
I think that's exactly right.
Thanks,
Nick
Nicholas Piggin <[email protected]> writes:
> On Thu, 21 Jun 2018 14:28:22 -0700
> Matthew Wilcox <[email protected]> wrote:
>
>> ida_alloc_range is the perfect fit for this use case. Eliminates
>> a custom spinlock, a call to ida_pre_get and a local check for the
>> allocated ID exceeding a maximum.
>>
>> Signed-off-by: Matthew Wilcox <[email protected]>
>> ---
>> arch/powerpc/mm/mmu_context_book3s64.c | 44 +++-----------------------
>> 1 file changed, 4 insertions(+), 40 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/mmu_context_book3s64.c b/arch/powerpc/mm/mmu_context_book3s64.c
>> index f3d4b4a0e561..5a0cf2cc8ba0 100644
>> --- a/arch/powerpc/mm/mmu_context_book3s64.c
>> +++ b/arch/powerpc/mm/mmu_context_book3s64.c
>> @@ -26,48 +26,16 @@
>> #include <asm/mmu_context.h>
>> #include <asm/pgalloc.h>
>>
>> -static DEFINE_SPINLOCK(mmu_context_lock);
>> static DEFINE_IDA(mmu_context_ida);
>>
>> static int alloc_context_id(int min_id, int max_id)
>> {
>> - int index, err;
>> -
>> -again:
>> - if (!ida_pre_get(&mmu_context_ida, GFP_KERNEL))
>> - return -ENOMEM;
>> -
>> - spin_lock(&mmu_context_lock);
>> - err = ida_get_new_above(&mmu_context_ida, min_id, &index);
>> - spin_unlock(&mmu_context_lock);
>> -
>> - if (err == -EAGAIN)
>> - goto again;
>> - else if (err)
>> - return err;
>> -
>> - if (index > max_id) {
>> - spin_lock(&mmu_context_lock);
>> - ida_remove(&mmu_context_ida, index);
>> - spin_unlock(&mmu_context_lock);
>> - return -ENOMEM;
>> - }
>> -
>> - return index;
>> + return ida_alloc_range(&mmu_context_ida, min_id, max_id, GFP_KERNEL);
>> }
>>
>> void hash__reserve_context_id(int id)
>> {
>> - int rc, result = 0;
>> -
>> - do {
>> - if (!ida_pre_get(&mmu_context_ida, GFP_KERNEL))
>> - break;
>> -
>> - spin_lock(&mmu_context_lock);
>> - rc = ida_get_new_above(&mmu_context_ida, id, &result);
>> - spin_unlock(&mmu_context_lock);
>> - } while (rc == -EAGAIN);
>> + int result = ida_alloc_range(&mmu_context_ida, id, id, GFP_KERNEL);
>>
>> WARN(result != id, "mmu: Failed to reserve context id %d (rc %d)\n", id, result);
>> }
>> @@ -172,9 +140,7 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
>>
>> void __destroy_context(int context_id)
>> {
>> - spin_lock(&mmu_context_lock);
>> - ida_remove(&mmu_context_ida, context_id);
>> - spin_unlock(&mmu_context_lock);
>> + ida_free(&mmu_context_ida, context_id);
>> }
>> EXPORT_SYMBOL_GPL(__destroy_context);
>>
>> @@ -182,13 +148,11 @@ static void destroy_contexts(mm_context_t *ctx)
>> {
>> int index, context_id;
>>
>> - spin_lock(&mmu_context_lock);
>> for (index = 0; index < ARRAY_SIZE(ctx->extended_id); index++) {
>> context_id = ctx->extended_id[index];
>> if (context_id)
>> - ida_remove(&mmu_context_ida, context_id);
>> + ida_free(&mmu_context_ida, context_id);
>> }
>> - spin_unlock(&mmu_context_lock);
>> }
>>
>> static void pte_frag_destroy(void *pte_frag)
>
> This hunk should be okay because the mmu_context_lock does not protect
> the extended_id array, right Aneesh?
Yes. This is called at process exit, so we should not find parallel
calls. On the allocation side, we are protected by mmap_sem. We do
allocate extended_id when doing mmap.
-aneesh
Matthew Wilcox <[email protected]> writes:
> this situation from occurring (like, oh i don't know, this function only
> being called on process exit, so implicitly only called once per context).
>
That is correct.
On 06/21/2018 02:28 PM, Matthew Wilcox wrote:
> The new API is much easier for this user. Also add kerneldoc for
> get_anon_bdev().
>
> Signed-off-by: Matthew Wilcox <[email protected]>
> ---
> fs/super.c | 63 +++++++++++++++++++-----------------------------------
> 1 file changed, 22 insertions(+), 41 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index 50728d9c1a05..3e7a0aea716a 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -978,58 +978,42 @@ void emergency_thaw_all(void)
> }
> }
>
> -/*
> - * Unnamed block devices are dummy devices used by virtual
> - * filesystems which don't use real block-devices. -- jrs
> - */
> -
> static DEFINE_IDA(unnamed_dev_ida);
> -static DEFINE_SPINLOCK(unnamed_dev_lock);/* protects the above */
> -/* Many userspace utilities consider an FSID of 0 invalid.
> - * Always return at least 1 from get_anon_bdev.
> - */
> -static int unnamed_dev_start = 1;
>
> +/**
> + * get_anon_bdev - Allocate a block device for filesystems which don't have one.
> + * @p: Pointer to a dev_t.
> + *
> + * Filesystems which don't use real block devices can call this function
> + * to allocate a virtual block device.
> + *
> + * Context: Any context. Frequently called while holding sb_lock.
> + * Return: 0 on success, -EMFILE if there are no anonymous bdevs left
> + * or -EAGAIN if memory allocation failed.
Looks to me like the code used to return -ENOMEM and used -EAGAIN as an
internal retry code.
confused? (/me)
> + */
> int get_anon_bdev(dev_t *p)
> {
> int dev;
> - int error;
>
> - retry:
> - if (ida_pre_get(&unnamed_dev_ida, GFP_ATOMIC) == 0)
> - return -ENOMEM;
> - spin_lock(&unnamed_dev_lock);
> - error = ida_get_new_above(&unnamed_dev_ida, unnamed_dev_start, &dev);
> - if (!error)
> - unnamed_dev_start = dev + 1;
> - spin_unlock(&unnamed_dev_lock);
> - if (error == -EAGAIN)
> - /* We raced and lost with another CPU. */
> - goto retry;
> - else if (error)
> + /*
> + * Many userspace utilities consider an FSID of 0 invalid.
> + * Always return at least 1 from get_anon_bdev.
> + */
> + dev = ida_alloc_range(&unnamed_dev_ida, 1, (1 << MINORBITS) - 1,
> + GFP_ATOMIC);
> + if (dev == -ENOSPC)
> + return -EMFILE;
> + if (dev < 0)
> return -EAGAIN;
>
> - if (dev >= (1 << MINORBITS)) {
> - spin_lock(&unnamed_dev_lock);
> - ida_remove(&unnamed_dev_ida, dev);
> - if (unnamed_dev_start > dev)
> - unnamed_dev_start = dev;
> - spin_unlock(&unnamed_dev_lock);
> - return -EMFILE;
> - }
> - *p = MKDEV(0, dev & MINORMASK);
> + *p = MKDEV(0, dev);
> return 0;
> }
> EXPORT_SYMBOL(get_anon_bdev);
--
~Randy
On Fri, Jun 22, 2018 at 12:45:10PM -0700, Randy Dunlap wrote:
> > + * Context: Any context. Frequently called while holding sb_lock.
> > + * Return: 0 on success, -EMFILE if there are no anonymous bdevs left
> > + * or -EAGAIN if memory allocation failed.
>
> Looks to me like the code used to return -ENOMEM and used -EAGAIN as an
> internal retry code.
>
> confused? (/me)
Quite right.
+++ b/fs/super.c
@@ -989,7 +989,7 @@ static DEFINE_IDA(unnamed_dev_ida);
*
* Context: Any context. Frequently called while holding sb_lock.
* Return: 0 on success, -EMFILE if there are no anonymous bdevs left
- * or -EAGAIN if memory allocation failed.
+ * or -ENOMEM if memory allocation failed.
*/
int get_anon_bdev(dev_t *p)
{
@@ -1002,9 +1002,9 @@ int get_anon_bdev(dev_t *p)
dev = ida_alloc_range(&unnamed_dev_ida, 1, (1 << MINORBITS) - 1,
GFP_ATOMIC);
if (dev == -ENOSPC)
- return -EMFILE;
+ dev = -EMFILE;
if (dev < 0)
- return -EAGAIN;
+ return dev;
*p = MKDEV(0, dev);
return 0;
On 21-06-18, 14:28, Matthew Wilcox wrote:
> Simpler and shorter code.
I couldn't find ida_alloc/ida_free in 4.18-rc1 so I assume this will go
thru tree adding this so:
Acked-by: Vinod Koul <[email protected]>
>
> Signed-off-by: Matthew Wilcox <[email protected]>
> ---
> drivers/dma/dmaengine.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 08ba8473a284..1e9bdadfc312 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -161,9 +161,7 @@ static void chan_dev_release(struct device *dev)
>
> chan_dev = container_of(dev, typeof(*chan_dev), device);
> if (atomic_dec_and_test(chan_dev->idr_ref)) {
> - mutex_lock(&dma_list_mutex);
> - ida_remove(&dma_ida, chan_dev->dev_id);
> - mutex_unlock(&dma_list_mutex);
> + ida_free(&dma_ida, chan_dev->dev_id);
> kfree(chan_dev->idr_ref);
> }
> kfree(chan_dev);
> @@ -896,16 +894,10 @@ static bool device_has_all_tx_types(struct dma_device *device)
>
> static int get_dma_id(struct dma_device *device)
> {
> - int rc;
> -
> - do {
> - if (!ida_pre_get(&dma_ida, GFP_KERNEL))
> - return -ENOMEM;
> - mutex_lock(&dma_list_mutex);
> - rc = ida_get_new(&dma_ida, &device->dev_id);
> - mutex_unlock(&dma_list_mutex);
> - } while (rc == -EAGAIN);
> + int rc = ida_alloc(&dma_ida, GFP_KERNEL);
>
> + if (rc >= 0)
> + device->dev_id = rc;
> return rc;
> }
>
> @@ -1090,9 +1082,7 @@ int dma_async_device_register(struct dma_device *device)
> err_out:
> /* if we never registered a channel just release the idr */
> if (atomic_read(idr_ref) == 0) {
> - mutex_lock(&dma_list_mutex);
> - ida_remove(&dma_ida, device->dev_id);
> - mutex_unlock(&dma_list_mutex);
> + ida_free(&dma_ida, device->dev_id);
> kfree(idr_ref);
> return rc;
> }
> --
> 2.17.1
--
~Vinod
On Sat, Jun 23, 2018 at 06:00:39PM +0530, Vinod wrote:
> On 21-06-18, 14:28, Matthew Wilcox wrote:
> > Simpler and shorter code.
>
> I couldn't find ida_alloc/ida_free in 4.18-rc1 so I assume this will go
> thru tree adding this so:
>
> Acked-by: Vinod Koul <[email protected]>
Yup, you were bcc'd on patch 3/26 which added the new API. I'll be sending a pull request for this mass conversion, and I'll add your ack. Thanks!
On 06/21/2018 11:28 PM, Matthew Wilcox wrote:
> Simpler and shorter code.
>
> Signed-off-by: Matthew Wilcox <[email protected]>
> ---
[...]> {
> - int rc;
> -
> - do {
> - if (!ida_pre_get(&dma_ida, GFP_KERNEL))
> - return -ENOMEM;
> - mutex_lock(&dma_list_mutex);
> - rc = ida_get_new(&dma_ida, &device->dev_id);
> - mutex_unlock(&dma_list_mutex);
> - } while (rc == -EAGAIN);
> + int rc = ida_alloc(&dma_ida, GFP_KERNEL);
>
> + if (rc >= 0)
> + device->dev_id = rc;
> return rc;
This used to return 0 on success, now it returns the ID. That wont work
considering that it is used like this
rc = get_dma_id(device);
if (rc != 0) ...
> }
>
> @@ -1090,9 +1082,7 @@ int dma_async_device_register(struct dma_device *device)
> err_out:
> /* if we never registered a channel just release the idr */
> if (atomic_read(idr_ref) == 0) {
> - mutex_lock(&dma_list_mutex);
> - ida_remove(&dma_ida, device->dev_id);
> - mutex_unlock(&dma_list_mutex);
> + ida_free(&dma_ida, device->dev_id);
> kfree(idr_ref);
> return rc;
> }
>
Looks good,
Reviewed-by: Johannes Thumshirn <[email protected]>
--
Johannes Thumshirn Storage
[email protected] +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
On Sun, Jun 24, 2018 at 09:57:45AM +0200, Lars-Peter Clausen wrote:
> > + int rc = ida_alloc(&dma_ida, GFP_KERNEL);
> >
> > + if (rc >= 0)
> > + device->dev_id = rc;
> > return rc;
>
> This used to return 0 on success, now it returns the ID. That wont work
> considering that it is used like this
>
> rc = get_dma_id(device);
> if (rc != 0) ...
Thanks! I changed it to this:
static int get_dma_id(struct dma_device *device)
{
int rc = ida_alloc(&dma_ida, GFP_KERNEL);
if (rc < 0)
return rc;
device->dev_id = rc;
return 0;
}
On Thu, Jun 21, 2018 at 02:28:24PM -0700, Matthew Wilcox wrote:
> Removes a custom spinlock and simplifies the code.
I took a closer look at this patch as part of fixing the typo *ahem*.
The original code is buggy at the limit:
- if (winid > VAS_WINDOWS_PER_CHIP) {
- pr_err("Too many (%d) open windows\n", winid);
- vas_release_window_id(ida, winid);
That permits winid to be == VAS_WINDOWS_PER_CHIP, which is 64 << 10.
Since you then go on to store:
int id = window->winid;
vinst->windows[id] = window;
and windows is defined as:
struct vas_window *windows[VAS_WINDOWS_PER_CHIP];
that's a buffer overflow.
Here's the current version of my patch which will be in linux-next tomorrow.
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index ff9f48812331..e59e0e60e5b5 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -515,35 +515,17 @@ int init_winctx_regs(struct vas_window *window, struct vas_winctx *winctx)
return 0;
}
-static DEFINE_SPINLOCK(vas_ida_lock);
-
static void vas_release_window_id(struct ida *ida, int winid)
{
- spin_lock(&vas_ida_lock);
- ida_remove(ida, winid);
- spin_unlock(&vas_ida_lock);
+ ida_free(ida, winid);
}
static int vas_assign_window_id(struct ida *ida)
{
- int rc, winid;
-
- do {
- rc = ida_pre_get(ida, GFP_KERNEL);
- if (!rc)
- return -EAGAIN;
-
- spin_lock(&vas_ida_lock);
- rc = ida_get_new(ida, &winid);
- spin_unlock(&vas_ida_lock);
- } while (rc == -EAGAIN);
-
- if (rc)
- return rc;
+ int winid = ida_alloc_max(ida, VAS_WINDOWS_PER_CHIP - 1, GFP_KERNEL);
- if (winid > VAS_WINDOWS_PER_CHIP) {
- pr_err("Too many (%d) open windows\n", winid);
- vas_release_window_id(ida, winid);
+ if (winid == -ENOSPC) {
+ pr_err("Too many (%d) open windows\n", VAS_WINDOWS_PER_CHIP);
return -EAGAIN;
}
On Thu, Jun 21, 2018 at 02:28:23PM -0700, Matthew Wilcox wrote:
> Removes a call to ida_pre_get().
>
> Signed-off-by: Matthew Wilcox <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
> ---
> drivers/media/media-device.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index ae59c3177555..d51088bcd735 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -575,18 +575,12 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> entity->num_links = 0;
> entity->num_backlinks = 0;
>
> - if (!ida_pre_get(&mdev->entity_internal_idx, GFP_KERNEL))
> - return -ENOMEM;
> -
> - mutex_lock(&mdev->graph_mutex);
> -
> - ret = ida_get_new_above(&mdev->entity_internal_idx, 1,
> - &entity->internal_idx);
> - if (ret < 0) {
> - mutex_unlock(&mdev->graph_mutex);
> + ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
> + if (ret < 0)
> return ret;
> - }
> + entity->internal_idx = ret;
>
> + mutex_lock(&mdev->graph_mutex);
> mdev->entity_internal_idx_max =
> max(mdev->entity_internal_idx_max, entity->internal_idx);
>
> @@ -632,7 +626,7 @@ static void __media_device_unregister_entity(struct media_entity *entity)
> struct media_interface *intf;
> unsigned int i;
>
> - ida_simple_remove(&mdev->entity_internal_idx, entity->internal_idx);
> + ida_free(&mdev->entity_internal_idx, entity->internal_idx);
>
> /* Remove all interface links pointing to this entity */
> list_for_each_entry(intf, &mdev->interfaces, graph_obj.list) {
--
Sakari Ailus
e-mail: [email protected]
On 06/21/2018 04:28 PM, Matthew Wilcox wrote:
> @@ -1163,11 +1157,9 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn,
> goto old_sess_out;
> if (conn->sess->se_sess)
> transport_free_session(conn->sess->se_sess);
> - if (conn->sess->session_index != 0) {
> - spin_lock_bh(&sess_idr_lock);
> - idr_remove(&sess_idr, conn->sess->session_index);
> - spin_unlock_bh(&sess_idr_lock);
This code looks buggy. We will probably NULL pointer oops before we hit it.
It looks like the session_index check was supposed to detect when login
fails in the middle of doing login, so that code probably wanted to do:
idr_alloc(&sess_idr, NULL, 1, 0, GFP_NOWAIT);
The problem is that iscsi_login_zero_tsih_s1 sets conn->sess early in
iscsi_login_set_conn_values. If the function fails later like when we
alloc the idr it does kfree(sess) and leaves the conn->sess pointer set.
iscsi_login_zero_tsih_s1 then returns -Exyz and we then call
iscsi_target_login_sess_out and access the freed memory above.
So I am not sure what we want to do here for your patch since it is not
adding any new bugs. Just merge your patch now and I can send a fix for
the above bug over it?
> - }
> + /* Um, 0 is a valid ID. I suppose we never free it? */
> + if (conn->sess->session_index != 0)
> + ida_free(&sess_ida, conn->sess->session_index);
> kfree(conn->sess->sess_ops);
> kfree(conn->sess);
> conn->sess = NULL;
>
On 07/26/2018 11:48 AM, Mike Christie wrote:
> On 06/21/2018 04:28 PM, Matthew Wilcox wrote:
>
>> @@ -1163,11 +1157,9 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn,
>> goto old_sess_out;
>> if (conn->sess->se_sess)
>> transport_free_session(conn->sess->se_sess);
>> - if (conn->sess->session_index != 0) {
>> - spin_lock_bh(&sess_idr_lock);
>> - idr_remove(&sess_idr, conn->sess->session_index);
>> - spin_unlock_bh(&sess_idr_lock);
>
> This code looks buggy. We will probably NULL pointer oops before we hit it.
Sorry did not mean null pointer, but some crash due to accessing memory
that was freed.
>
> It looks like the session_index check was supposed to detect when login
> fails in the middle of doing login, so that code probably wanted to do:
>
> idr_alloc(&sess_idr, NULL, 1, 0, GFP_NOWAIT);
>
> The problem is that iscsi_login_zero_tsih_s1 sets conn->sess early in
> iscsi_login_set_conn_values. If the function fails later like when we
> alloc the idr it does kfree(sess) and leaves the conn->sess pointer set.
> iscsi_login_zero_tsih_s1 then returns -Exyz and we then call
> iscsi_target_login_sess_out and access the freed memory above.
>
> So I am not sure what we want to do here for your patch since it is not
> adding any new bugs. Just merge your patch now and I can send a fix for
> the above bug over it?
>
>
>> - }
>> + /* Um, 0 is a valid ID. I suppose we never free it? */
>> + if (conn->sess->session_index != 0)
>> + ida_free(&sess_ida, conn->sess->session_index);
>> kfree(conn->sess->sess_ops);
>> kfree(conn->sess);
>> conn->sess = NULL;
>>
>
On 07/26/2018 11:48 AM, Mike Christie wrote:
> So I am not sure what we want to do here for your patch since it is not
> adding any new bugs. Just merge your patch now and I can send a fix for
> the above bug over it?
If we want to fix the bug first, then I made the attached patch and I
can submit it.
On Thu, Jul 26, 2018 at 12:13:49PM -0500, Mike Christie wrote:
> If we want to fix the bug first, then I made the attached patch and I
> can submit it.
How about I take it through my tree to minimise the amount of rebasing
I'll need to do? I'm already dependent on the nvdimm tree and I'd rather
not depend on the scsi tree as well. I'll queue it up in front of my
IDA change to maximise its backportability.
> >From 80c495c3d7f4487c1b6f52f70e8ddc74dcb70794 Mon Sep 17 00:00:00 2001
> From: Mike Christie <[email protected]>
> Date: Thu, 26 Jul 2018 12:06:07 -0500
> Subject: [PATCH] iscsi target: fix session creation failure handling
>
> The problem is that iscsi_login_zero_tsih_s1 sets conn->sess early in
> iscsi_login_set_conn_values. If the function fails later like when we
> alloc the idr it does kfree(sess) and leaves the conn->sess pointer set.
> iscsi_login_zero_tsih_s1 then returns -Exyz and we then call
> iscsi_target_login_sess_out and access the freed memory.
>
> This patch has iscsi_login_zero_tsih_s1 either completely setup the
> session or completely tear it down, so later in
> iscsi_target_login_sess_out we can just check for it being set to the
> connection.
> ---
> drivers/target/iscsi/iscsi_target_login.c | 35 ++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index 9950178..e20d811 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -348,8 +348,7 @@ static int iscsi_login_zero_tsih_s1(
> pr_err("idr_alloc() for sess_idr failed\n");
> iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
> ISCSI_LOGIN_STATUS_NO_RESOURCES);
> - kfree(sess);
> - return -ENOMEM;
> + goto free_sess;
> }
>
> sess->creation_time = get_jiffies_64();
> @@ -365,20 +364,28 @@ static int iscsi_login_zero_tsih_s1(
> ISCSI_LOGIN_STATUS_NO_RESOURCES);
> pr_err("Unable to allocate memory for"
> " struct iscsi_sess_ops.\n");
> - kfree(sess);
> - return -ENOMEM;
> + goto remove_idr;
> }
>
> sess->se_sess = transport_init_session(TARGET_PROT_NORMAL);
> if (IS_ERR(sess->se_sess)) {
> iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
> ISCSI_LOGIN_STATUS_NO_RESOURCES);
> - kfree(sess->sess_ops);
> - kfree(sess);
> - return -ENOMEM;
> + goto free_ops;
> }
>
> return 0;
> +
> +free_ops:
> + kfree(sess->sess_ops);
> +remove_idr:
> + spin_lock_bh(&sess_idr_lock);
> + idr_remove(&sess_idr, sess->session_index);
> + spin_unlock_bh(&sess_idr_lock);
> +free_sess:
> + kfree(sess);
> + conn->sess = NULL;
> + return -ENOMEM;
> }
>
> static int iscsi_login_zero_tsih_s2(
> @@ -1161,13 +1168,13 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn,
> ISCSI_LOGIN_STATUS_INIT_ERR);
> if (!zero_tsih || !conn->sess)
> goto old_sess_out;
> - if (conn->sess->se_sess)
> - transport_free_session(conn->sess->se_sess);
> - if (conn->sess->session_index != 0) {
> - spin_lock_bh(&sess_idr_lock);
> - idr_remove(&sess_idr, conn->sess->session_index);
> - spin_unlock_bh(&sess_idr_lock);
> - }
> +
> + transport_free_session(conn->sess->se_sess);
> +
> + spin_lock_bh(&sess_idr_lock);
> + idr_remove(&sess_idr, conn->sess->session_index);
> + spin_unlock_bh(&sess_idr_lock);
> +
> kfree(conn->sess->sess_ops);
> kfree(conn->sess);
> conn->sess = NULL;
> --
> 1.8.3.1
>
On 07/27/2018 02:38 PM, Matthew Wilcox wrote:
> On Thu, Jul 26, 2018 at 12:13:49PM -0500, Mike Christie wrote:
>> If we want to fix the bug first, then I made the attached patch and I
>> can submit it.
>
> How about I take it through my tree to minimise the amount of rebasing
> I'll need to do? I'm already dependent on the nvdimm tree and I'd rather
> not depend on the scsi tree as well. I'll queue it up in front of my
> IDA change to maximise its backportability.
Ccing Martin, because he has been handling target patches.
The above plan sounds ok to me.
Em Tue, 24 Jul 2018 14:05:07 +0300
Sakari Ailus <[email protected]> escreveu:
> On Thu, Jun 21, 2018 at 02:28:23PM -0700, Matthew Wilcox wrote:
> > Removes a call to ida_pre_get().
> >
> > Signed-off-by: Matthew Wilcox <[email protected]>
>
> Reviewed-by: Sakari Ailus <[email protected]>
I'm assuming that the entire series will be applied together via some
other tree. So:
Acked-by: Mauro Carvalho Chehab <[email protected]>
>
> > ---
> > drivers/media/media-device.c | 16 +++++-----------
> > 1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index ae59c3177555..d51088bcd735 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -575,18 +575,12 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> > entity->num_links = 0;
> > entity->num_backlinks = 0;
> >
> > - if (!ida_pre_get(&mdev->entity_internal_idx, GFP_KERNEL))
> > - return -ENOMEM;
> > -
> > - mutex_lock(&mdev->graph_mutex);
> > -
> > - ret = ida_get_new_above(&mdev->entity_internal_idx, 1,
> > - &entity->internal_idx);
> > - if (ret < 0) {
> > - mutex_unlock(&mdev->graph_mutex);
> > + ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
> > + if (ret < 0)
> > return ret;
> > - }
> > + entity->internal_idx = ret;
> >
> > + mutex_lock(&mdev->graph_mutex);
> > mdev->entity_internal_idx_max =
> > max(mdev->entity_internal_idx_max, entity->internal_idx);
> >
> > @@ -632,7 +626,7 @@ static void __media_device_unregister_entity(struct media_entity *entity)
> > struct media_interface *intf;
> > unsigned int i;
> >
> > - ida_simple_remove(&mdev->entity_internal_idx, entity->internal_idx);
> > + ida_free(&mdev->entity_internal_idx, entity->internal_idx);
> >
> > /* Remove all interface links pointing to this entity */
> > list_for_each_entry(intf, &mdev->interfaces, graph_obj.list) {
>
Thanks,
Mauro
Mike,
>> How about I take it through my tree to minimise the amount of rebasing
>> I'll need to do? I'm already dependent on the nvdimm tree and I'd rather
>> not depend on the scsi tree as well. I'll queue it up in front of my
>> IDA change to maximise its backportability.
>
> Ccing Martin, because he has been handling target patches.
That's fine with me.
Acked-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
On Mon, Jul 30, 2018 at 10:03:21PM -0400, Martin K. Petersen wrote:
>
> Mike,
>
> >> How about I take it through my tree to minimise the amount of rebasing
> >> I'll need to do? I'm already dependent on the nvdimm tree and I'd rather
> >> not depend on the scsi tree as well. I'll queue it up in front of my
> >> IDA change to maximise its backportability.
> >
> > Ccing Martin, because he has been handling target patches.
>
> That's fine with me.
>
> Acked-by: Martin K. Petersen <[email protected]>
Thanks, Martin. Mike, can I have your Signed-off-by: on the original patch?
What Fixes: line should this have? As far as I can tell, the bug is present
all the way back to its introduction, so:
Fixes: e48354ce078c ("iscsi-target: Add iSCSI fabric support for target v4.1")
yes?
On Mon, Jul 30, 2018 at 11:55:21AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 24 Jul 2018 14:05:07 +0300
> Sakari Ailus <[email protected]> escreveu:
>
> > On Thu, Jun 21, 2018 at 02:28:23PM -0700, Matthew Wilcox wrote:
> > > Removes a call to ida_pre_get().
> > >
> > > Signed-off-by: Matthew Wilcox <[email protected]>
> >
> > Reviewed-by: Sakari Ailus <[email protected]>
>
> I'm assuming that the entire series will be applied together via some
> other tree. So:
>
> Acked-by: Mauro Carvalho Chehab <[email protected]>
Yep, thanks. It's in linux-next and it's all going in via my 'ida'
branch.
On 07/31/2018 01:15 PM, Matthew Wilcox wrote:
> On Mon, Jul 30, 2018 at 10:03:21PM -0400, Martin K. Petersen wrote:
>>
>> Mike,
>>
>>>> How about I take it through my tree to minimise the amount of rebasing
>>>> I'll need to do? I'm already dependent on the nvdimm tree and I'd rather
>>>> not depend on the scsi tree as well. I'll queue it up in front of my
>>>> IDA change to maximise its backportability.
>>>
>>> Ccing Martin, because he has been handling target patches.
>>
>> That's fine with me.
>>
>> Acked-by: Martin K. Petersen <[email protected]>
>
> Thanks, Martin. Mike, can I have your Signed-off-by: on the original patch?
Yes,
Signed-off-by: Mike Christie <[email protected]>
> What Fixes: line should this have? As far as I can tell, the bug is present
> all the way back to its introduction, so:
>
> Fixes: e48354ce078c ("iscsi-target: Add iSCSI fabric support for target v4.1")
>
> yes?
The session_index bug was in that patch, but I think it correctly kfreed
the session in __iscsi_target_login_thread.
I think it was this one that added the early kfree bug:
commit 0957627a99604f379d35831897a2aa15272528fc
Author: Nicholas Bellinger <[email protected]>
Date: Fri Nov 4 11:36:38 2011 -0700
iscsi-target: Fix sess allocation leak in iscsi_login_zero_tsih_s1
Dan's tools probably just did not catch the iscsi_login_zero_tsih_s1 ->
iscsi_login_set_conn_values call that sets conn->sess then later
kfree(conn->sess) call in the error handling in __iscsi_target_login_thread.