lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Jul 2022 20:19:19 +0800 From: Guo Zhi <qtxuning1999@...u.edu.cn> To: "Michael S. Tsirkin" <mst@...hat.com> Cc: jasowang@...hat.com, eperezma@...hat.com, virtualization@...ts.linux-foundation.org, linux-kernel@...r.kernel.org, sgarzare@...hat.com Subject: Re: [PATCH v2 2/4] virtio_test: move magic number in code as defined constant On 2022/7/7 15:36, Michael S. Tsirkin wrote: > On Thu, Jul 07, 2022 at 02:17:19PM +0800, Guo Zhi wrote: >> On 2022/7/7 13:09, Michael S. Tsirkin wrote: >>> On Thu, Jul 07, 2022 at 10:44:07AM +0800, Guo Zhi wrote: >>>> We should avoid using magic numbers directly. >>> Not necessarily. For repeated values - I guess. >>> But this kind of thing: >>> >>> #define BUF_SIZE 1024 >>> int buf_size = BUF_SIZE; >>> >>> brings no benefit IMHO. >>> >>> And this has cost - values are now removed from code. >> Firstly, as a test, user will have to change the config to test virtio and >> vhost frequently. If these magic number are put together like these, change >> will be more easier. > If tweaking these is useful for users we should add a command line flags > as opposed to asking users to tweak code. I think you suggestions are correct, I should not change that magic numbers which doesn't have relationship with my work. So in next version patch, I will stop changing these magic number to defined values. > >> Secondly, many configs will be repeated, such as MAX_SG_FRAGS, even if some >> varibale only appear once, subsequent upgrade of virtio_test will use these >> variable in other place. For example, in this series of patch, patch 4/4 >> will use RINGSIZE to set INDIRECTS_BUF_SIZE. >> >>>> Signed-off-by: Guo Zhi <qtxuning1999@...u.edu.cn> > Isn't there some way to use the size we get as 1st parameter of kmalloc? > >>>> --- >>>> tools/virtio/virtio_test.c | 18 +++++++++++------- >>>> 1 file changed, 11 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c >>>> index 95f78b311..1ecd64271 100644 >>>> --- a/tools/virtio/virtio_test.c >>>> +++ b/tools/virtio/virtio_test.c >>>> @@ -20,7 +20,10 @@ >>>> #include "../../drivers/vhost/test.h" >>>> #define RANDOM_BATCH -1 >>>> - >>>> +#define ALIGN 4096 >>>> +#define RINGSIZE 256 >>>> +#define TEST_BUF_NUM 0x100000 >>>> +#define BUF_SIZE 1024 >>>> /* Unused */ >>>> void *__kmalloc_fake, *__kfree_ignore_start, *__kfree_ignore_end; >>>> @@ -100,8 +103,8 @@ static void vq_reset(struct vq_info *info, int num, struct virtio_device *vdev) >>>> if (info->vq) >>>> vring_del_virtqueue(info->vq); >>>> - memset(info->ring, 0, vring_size(num, 4096)); >>>> - vring_init(&info->vring, num, info->ring, 4096); >>>> + memset(info->ring, 0, vring_size(num, ALIGN)); >>>> + vring_init(&info->vring, num, info->ring, ALIGN); >>>> info->vq = __vring_new_virtqueue(info->idx, info->vring, vdev, true, >>>> false, vq_notify, vq_callback, "test"); >>>> assert(info->vq); >>>> @@ -115,7 +118,7 @@ static void vq_info_add(struct vdev_info *dev, int num) >>>> info->idx = dev->nvqs; >>>> info->kick = eventfd(0, EFD_NONBLOCK); >>>> info->call = eventfd(0, EFD_NONBLOCK); >>>> - r = posix_memalign(&info->ring, 4096, vring_size(num, 4096)); >>>> + r = posix_memalign(&info->ring, PAGE_SIZE, vring_size(num, ALIGN)); >>>> assert(r >= 0); >>>> vq_reset(info, num, &dev->vdev); >>>> vhost_vq_setup(dev, info); >>> This is actually doing more than what commit log says. >>> >>>> @@ -131,7 +134,7 @@ static void vdev_info_init(struct vdev_info* dev, unsigned long long features) >>>> dev->vdev.features = features; >>>> INIT_LIST_HEAD(&dev->vdev.vqs); >>>> spin_lock_init(&dev->vdev.vqs_list_lock); >>>> - dev->buf_size = 1024; >>>> + dev->buf_size = BUF_SIZE; >>>> >>>> >>>> This seems to have zero added value. >>>> >> Since the reason above and other magic number have been set as a defined >> constant, change here for simplicity and consistency. >>>> dev->buf = malloc(dev->buf_size); >>>> assert(dev->buf); >>>> dev->control = open("/dev/vhost-test", O_RDWR); >>>> @@ -396,7 +399,8 @@ int main(int argc, char **argv) >>>> done: >>>> vdev_info_init(&dev, features); >>>> - vq_info_add(&dev, 256); >>>> - run_test(&dev, &dev.vqs[0], delayed, batch, reset, 0x100000); >>>> + vq_info_add(&dev, RINGSIZE); >>>> + >>>> + run_test(&dev, &dev.vqs[0], delayed, batch, reset, TEST_BUF_NUM); >>>> return 0; >>> This replacement does not buy us anything either. >> RINGSIZE will be used in other place in subsequent modification. >>>> } >>>> -- >>>> 2.17.1
Powered by blists - more mailing lists