Re: [patch net-next 0/3] net/sched: Improve getting objects by indexes

2017-10-26 Thread Christian König

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(_mutex);
  
-	ret = idr_alloc(_minor_idr, bcd, 0, BSG_MAX_DEVS, GFP_KERNEL);

-   if (ret < 0) {
+   ret = idr_alloc(_minor_idr, bcd, _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.


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?


Regards,
Christian.



Chris Mi (3):
   idr: Use unsigned long instead of int
   net/sched: Change cls_flower to use IDR
   net/sched: Change act_api and act_xxx modules to use IDR

  block/bsg.c |   8 +-
  block/genhd.c   |  12 +-
  drivers/atm/nicstar.c   |  11 +-
  drivers/block/drbd/drbd_main.c  |  31 +--
  drivers/block/drbd/drbd_nl.c|  22 ++-
  drivers/block/drbd/drbd_proc.c  |   3 +-
  drivers/block/drbd/drbd_receiver.c  |  15 +-
  drivers/block/drbd/drbd_state.c |  34 ++--
  drivers/block/drbd/drbd_worker.c|   6 +-
  drivers/block/loop.c|  17 +-
  drivers/block/nbd.c |  20 +-
  drivers/block/zram/zram_drv.c   |   9 +-
  drivers/char/tpm/tpm-chip.c |  10 +-
  drivers/char/tpm/tpm.h  |   2 +-
  drivers/dca/dca-sysfs.c |   9 +-
  drivers/firewire/core-cdev.c|  18 +-
  drivers/firewire/core-device.c  |  15 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c |   8 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c |   9 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |   6 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |   2 +-
  drivers/gpu/drm/drm_auth.c  |   9 +-
  drivers/gpu/drm/drm_connector.c |  10 +-
  drivers/gpu/drm/drm_context.c   |  20 +-
  drivers/gpu/drm/drm_dp_aux_dev.c|  11 +-
  drivers/gpu/drm/drm_drv.c   |   6 +-
  drivers/gpu/drm/drm_gem.c   |  19 +-
  drivers/gpu/drm/drm_info.c  |   2 +-
  drivers/gpu/drm/drm_mode_object.c   |  11 +-
  drivers/gpu/drm/drm_syncobj.c   |  18 +-
  drivers/gpu/drm/exynos/exynos_drm_ipp.c |  25 ++-
  drivers/gpu/drm/i915/gvt/display.c  |   2 +-
  drivers/gpu/drm/i915/gvt/kvmgt.c|   2 +-
  drivers/gpu/drm/i915/gvt/vgpu.c |   9 +-
  drivers/gpu/drm/i915/i915_debugfs.c |   6 +-
  drivers/gpu/drm/i915/i915_gem_context.c |   9 +-
  drivers/gpu/drm/qxl/qxl_cmd.c   |   8 +-
  drivers/gpu/drm/qxl/qxl_release.c   |  14 +-
  drivers/gpu/drm/sis/sis_mm.c|   8 +-
  drivers/gpu/drm/tegra/drm.c |  10 +-
  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c|   3 +-
  drivers/gpu/drm/vgem/vgem_fence.c   |  12 +-
  drivers/gpu/drm/via/via_mm.c|   8 +-
  drivers/gpu/drm/virtio/virtgpu_kms.c|   5 +-
  drivers/gpu/drm/virtio/virtgpu_vq.c |   5 +-
  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c|   9 +-
  drivers/i2c/i2c-core-base.c |  19 +-
  drivers/infiniband/core/cm.c|   8 +-
  drivers/infiniband/core/cma.c   |  12 +-
  drivers/infiniband/core/rdma_core.c |   9 +-
  drivers/infiniband/core/sa_query.c  |  23 +--
  drivers/infiniband/core/ucm.c   |   7 +-
  drivers/infiniband/core/ucma.c  |  14 +-
  drivers/infiniband/hw/cxgb3/iwch.c  |   4 +-
  drivers/infiniband/hw/cxgb3/iwch.h  |   4 +-
  

[patch net-next 0/3] net/sched: Improve getting objects by indexes

2017-10-26 Thread 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.

Chris Mi (3):
  idr: Use unsigned long instead of int
  net/sched: Change cls_flower to use IDR
  net/sched: Change act_api and act_xxx modules to use IDR

 block/bsg.c |   8 +-
 block/genhd.c   |  12 +-
 drivers/atm/nicstar.c   |  11 +-
 drivers/block/drbd/drbd_main.c  |  31 +--
 drivers/block/drbd/drbd_nl.c|  22 ++-
 drivers/block/drbd/drbd_proc.c  |   3 +-
 drivers/block/drbd/drbd_receiver.c  |  15 +-
 drivers/block/drbd/drbd_state.c |  34 ++--
 drivers/block/drbd/drbd_worker.c|   6 +-
 drivers/block/loop.c|  17 +-
 drivers/block/nbd.c |  20 +-
 drivers/block/zram/zram_drv.c   |   9 +-
 drivers/char/tpm/tpm-chip.c |  10 +-
 drivers/char/tpm/tpm.h  |   2 +-
 drivers/dca/dca-sysfs.c |   9 +-
 drivers/firewire/core-cdev.c|  18 +-
 drivers/firewire/core-device.c  |  15 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c |   8 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c |   9 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |   6 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |   2 +-
 drivers/gpu/drm/drm_auth.c  |   9 +-
 drivers/gpu/drm/drm_connector.c |  10 +-
 drivers/gpu/drm/drm_context.c   |  20 +-
 drivers/gpu/drm/drm_dp_aux_dev.c|  11 +-
 drivers/gpu/drm/drm_drv.c   |   6 +-
 drivers/gpu/drm/drm_gem.c   |  19 +-
 drivers/gpu/drm/drm_info.c  |   2 +-
 drivers/gpu/drm/drm_mode_object.c   |  11 +-
 drivers/gpu/drm/drm_syncobj.c   |  18 +-
 drivers/gpu/drm/exynos/exynos_drm_ipp.c |  25 ++-
 drivers/gpu/drm/i915/gvt/display.c  |   2 +-
 drivers/gpu/drm/i915/gvt/kvmgt.c|   2 +-
 drivers/gpu/drm/i915/gvt/vgpu.c |   9 +-
 drivers/gpu/drm/i915/i915_debugfs.c |   6 +-
 drivers/gpu/drm/i915/i915_gem_context.c |   9 +-
 drivers/gpu/drm/qxl/qxl_cmd.c   |   8 +-
 drivers/gpu/drm/qxl/qxl_release.c   |  14 +-
 drivers/gpu/drm/sis/sis_mm.c|   8 +-
 drivers/gpu/drm/tegra/drm.c |  10 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c|   3 +-
 drivers/gpu/drm/vgem/vgem_fence.c   |  12 +-
 drivers/gpu/drm/via/via_mm.c|   8 +-
 drivers/gpu/drm/virtio/virtgpu_kms.c|   5 +-
 drivers/gpu/drm/virtio/virtgpu_vq.c |   5 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c|   9 +-
 drivers/i2c/i2c-core-base.c |  19 +-
 drivers/infiniband/core/cm.c|   8 +-
 drivers/infiniband/core/cma.c   |  12 +-
 drivers/infiniband/core/rdma_core.c |   9 +-
 drivers/infiniband/core/sa_query.c  |  23 +--
 drivers/infiniband/core/ucm.c   |   7 +-
 drivers/infiniband/core/ucma.c  |  14 +-
 drivers/infiniband/hw/cxgb3/iwch.c  |   4 +-
 drivers/infiniband/hw/cxgb3/iwch.h  |   4 +-
 drivers/infiniband/hw/cxgb4/device.c|  18 +-
 drivers/infiniband/hw/cxgb4/iw_cxgb4.h  |   4 +-
 drivers/infiniband/hw/hfi1/init.c   |   9 +-
 drivers/infiniband/hw/hfi1/vnic_main.c  |   6 +-
 drivers/infiniband/hw/mlx4/cm.c |  13 +-
 drivers/infiniband/hw/ocrdma/ocrdma_main.c  |   7 +-
 drivers/infiniband/hw/qib/qib_init.c|   9 +-
 drivers/infiniband/ulp/opa_vnic/opa_vnic_vema.c |  10 +-
 drivers/iommu/intel-svm.c   |   9 +-
 drivers/md/dm.c |  13 +-
 drivers/memstick/core/memstick.c|  10 +-
 drivers/memstick/core/ms_block.c|   9 +-
 drivers/memstick/core/mspro_block.c |  12 +-
 drivers/mfd/rtsx_pcr.c  |   9 +-
 drivers/misc/c2port/core.c  |   7 +-
 drivers/misc/cxl/context.c  |   8 +-
 drivers/misc/cxl/main.c |  15 +-
 drivers/misc/mei/main.c |   8 +-
 drivers/misc/mic/scif/scif_api.c|  11 +-
 drivers/misc/mic/scif/scif_ports.c

Re: [patch net-next 0/3] net/sched: Improve getting objects by indexes

2017-08-16 Thread Chris Wilson
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(_mutex);
> >   
> > - ret = idr_alloc(_minor_idr, bcd, 0, BSG_MAX_DEVS, GFP_KERNEL);
> > - if (ret < 0) {
> > + ret = idr_alloc(_minor_idr, bcd, _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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization