Quoting Christian König (2017-08-16 08:49:07)
> Am 16.08.2017 um 04:12 schrieb Chris Mi:
> > Using current TC code, it is very slow to insert a lot of rules.
> >
> > In order to improve the rules update rate in TC,
> > we introduced the following two changes:
> > 1) changed cls_flower to use IDR to manage the filters.
> > 2) changed all act_xxx modules to use IDR instead of
> > a small hash table
> >
> > But IDR has a limitation that it uses int. TC handle uses u32.
> > To make sure there is no regression, we also changed IDR to use
> > unsigned long. All clients of IDR are changed to use new IDR API.
>
> WOW, wait a second. The idr change is touching a lot of drivers and to
> be honest doesn't looks correct at all.
>
> Just look at the first chunk of your modification:
> > @@ -998,8 +999,9 @@ int bsg_register_queue(struct request_queue *q, struct device *parent,
> >
> > mutex_lock(&bsg_mutex);
> >
> > - ret = idr_alloc(&bsg_minor_idr, bcd, 0, BSG_MAX_DEVS, GFP_KERNEL);
> > - if (ret < 0) {
> > + ret = idr_alloc(&bsg_minor_idr, bcd, &idr_index, 0, BSG_MAX_DEVS,
> > + GFP_KERNEL);
> > + if (ret) {
> > if (ret == -ENOSPC) {
> > printk(KERN_ERR "bsg: too many bsg devices\n");
> > ret = -EINVAL;
> The condition "if (ret)" will now always be true after the first
> allocation and so we always run into the error handling after that.
ret is now purely the error code, so it doesn't look that suspicious.
> I've never read the bsg code before, but that's certainly not correct.
> And that incorrect pattern repeats over and over again in this code.
>
> Apart from that why the heck do you want to allocate more than 1<<31
> handles?
And more to the point, arbitrarily changing the maximum to ULONG_MAX
where the ABI only supports U32_MAX is dangerous. Unless you do the
analysis otherwise, you have to replace all the end=0 with end=INT_MAX
to maintain existing behaviour.
-Chris
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
