From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754124AbdJSSSJ (ORCPT ); Thu, 19 Oct 2017 14:18:09 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:33046 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752022AbdJSSSE (ORCPT ); Thu, 19 Oct 2017 14:18:04 -0400 X-AuditID: b6c32a38-d89ff70000001124-3f-59e8ec5949b4 Subject: Re: [PATCH 2/4][PoC][RFC] Add rlimit-events framework To: Greg KH Cc: viro@zeniv.linux.org.uk, arnd@arndb.de, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-arch@vger.kernel.org, k.lewandowsk@samsung.com, l.stelmach@samsung.com, p.szewczyk@samsung.com, b.zolnierkie@samsung.com, andrzej.p@samsung.com, kopasiak90@gmail.com From: Krzysztof Opasiak Message-id: <71b58311-5f64-5ece-c2eb-d891e373c307@samsung.com> Date: Thu, 19 Oct 2017 20:17:55 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-version: 1.0 In-reply-to: <20171019074118.GD20787@kroah.com> Content-type: text/plain; charset="utf-8"; format="flowed" Content-language: en-US Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrPKsWRmVeSWpSXmKPExsWy7bCmrm7kmxeRBu1P9S1mvWxnsfg76Ri7 xcYZ61ktmhevZ7No/DSX2eLZ6TyLm4dWMFp07PrKYrFn70kWi8u75rBZ/JoP1HD+73FWBx6P 378mMXrsnHWX3WP/3DXsHn1bVjF6fN4k57HpyVumALYoLpuU1JzMstQifbsEroyuE2vZCo5r VazsO83cwDhXsYuRk0NCwERi5+E17F2MXBxCAjsYJZ4s2c4C4XxnlHi67Dw7TNXe37OhqnYz SvS0P2OGcO4zSmz8sIAFpEpYwE5i3o17bCC2iICGxMujt8BGMQvMYpJY1fcSyOHgYBPQl5i3 SxSkhheo/vGCe2BhFgFVia/tbiBhUYEIiQubfjJBlAhK/Jh8D2w8J1DnhJnrwcYzC1hJPPvX ygphi0s0t95kgbDlJTaveQt2m4TAdzaJh4+XMYHMlxBwkehsYYJ4Rlji1fEt7BBhaYlLR20h ytcxSlzY+oANogboyZan0RC2tcSfVROh9vJJvPvawwrRyyvR0SYEUeIhsWPFYhYI21Hi5NIG JkjwPGGUmPZ0LeMERrlZSN6ZheSFWUhemIXkhQWMLKsYxVILinPTU4sNC0z0ihNzi0vz0vWS 83M3MYKTkZbFDsY953wOMQpwMCrx8G648CJSiDWxrLgy9xCjBAezkgjvsptAId6UxMqq1KL8 +KLSnNTiQ4zSHCxK4ryi669FCAmkJ5akZqemFqQWwWSZODilGhh1ubrfqvLnuFhMdDjA7bOf nS+V/4S+f0dD0LqZztKrFlatv+ymLVy6bd//DN8S5eLsuXYP1gS7u4c8Yb/B6vpgqR/bnEK5 g2F7J15ZV7KhOmai8yw7148XjLsXFmSf2XhK0/jToS09n0R9J2bN7/NwPByzaV7f6/kvTXVf rJmUyRd06WFW6A4lluKMREMt5qLiRAA/tsmGQgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupgkeLIzCtJLcpLzFFi42I5/e+xgG7kmxeRBvt+C1rMetnOYvF30jF2 i40z1rNaNC9ez2bR+Gkus8Wz03kWNw+tYLTo2PWVxWLP3pMsFpd3zWGz+DUfqOH83+OsDjwe v39NYvTYOesuu8f+uWvYPfq2rGL0+LxJzmPTk7dMAWxRXDYpqTmZZalF+nYJXBldJ9ayFRzX qljZd5q5gXGuYhcjJ4eEgInE3t+z2bsYuTiEBHYyStxePIMZwnnIKPFk3lpGkCphATuJeTfu sYHYIgIaEi+P3mIBKWIWmMUk0TO/C6r9CaPEnoV/WbsYOTjYBPQl5u0SBWngBWp+vOAeC0iY RUBV4mu7G4gpKhAhsWEjP0SFoMSPySAVnBycQI0TZq4HW8UsYCbx5eVhVghbXKK59SYLhC0v sXnNW+YJjAKzkLTPQtIyC0nLLCQtCxhZVjFKphYU56bnFhsVGOallusVJ+YWl+al6yXn525i BMbPtsNafTsY7y+JP8QowMGoxMMbce5FpBBrYllxZe4hRgkOZiUR3mU3gUK8KYmVValF+fFF pTmpxYcYpTlYlMR5b+cdixQSSE8sSc1OTS1ILYLJMnFwSjUwGq8LajasC7q5JvNdYLqngZrN 28luyz2XS56IYngm9qR/+9238vVXQi78ixB7qzQ1t3pl8Uv1pqke1ZNfrGVZ+VH4x8nmL6fl K2f4Rsg7z+yQF5B6b9Qt8MPOYdt/PVWmjAitHq+/m6UMknvZj9669/m0/NW+HUaJRY8eb+0+ 62KpNNWMnUNDiaU4I9FQi7moOBEAlTKN6JsCAAA= X-CMS-MailID: 20171019181801epcas1p49c9ac3778c28ae468fc603019d7be61e X-Msg-Generator: CA X-Sender-IP: 182.195.42.142 X-Local-Sender: =?UTF-8?B?S3J6eXN6dG9mIE9wYXNpYWsbU1JQT0wtU3lzdGVtIChUUCkb?= =?UTF-8?B?7IK87ISx7KCE7J6QG1NvZnR3YXJlIEVuZ2luZWVyIC8gRXhwZXJ0IFByb2dy?= =?UTF-8?B?YW1tZXI=?= X-Global-Sender: =?UTF-8?B?S3J6eXN6dG9mIE9wYXNpYWsbU1JQT0wtU3lzdGVtIChUUCkb?= =?UTF-8?B?U2Ftc3VuZ8KgRWxlY3Ryb25pY3MbU29mdHdhcmUgRW5naW5lZXIgLyBFeHBl?= =?UTF-8?B?cnQgUHJvZ3JhbW1lcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTY=?= CMS-TYPE: 101P X-CMS-RootMailID: 20171018203333epcas1p3d8dd8f3a755cb6ee8e9dde63cb91851b X-RootMTR: 20171018203333epcas1p3d8dd8f3a755cb6ee8e9dde63cb91851b References: <20171018203230.29871-1-k.opasiak@samsung.com> <20171018203230.29871-3-k.opasiak@samsung.com> <20171019074118.GD20787@kroah.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 10/19/2017 09:41 AM, Greg KH wrote: > Meta-comments on the code, I'm not commenting on the content, just > normal code review things that I always see in kernel code... > > On Wed, Oct 18, 2017 at 10:32:28PM +0200, Krzysztof Opasiak wrote: >> diff --git a/include/linux/rlimit_noti_kern.h b/include/linux/rlimit_noti_kern.h >> new file mode 100644 >> index 000000000000..e49fddaa21c0 >> --- /dev/null >> +++ b/include/linux/rlimit_noti_kern.h >> @@ -0,0 +1,54 @@ >> +/* >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. > > I have to ask, do you really mean "any later version" for this, and the > other new files you created? > If it's about me then I have not problems with "any later version" of GPL but there is not only me but also my company;) To be honest, I copied this from a file created some time ago by one of my coworkers assuming that he fallowed the company procedures, but maybe he didn't as it's causing so much interest;) I'll double check the company procedure and update this before sending v2. Thanks. > And, it is nice to use SPDX for new files to identify their license. > It's not that prevelant, but is getting there... Ok I'll fix this using SPDX. > >> --- a/include/uapi/linux/netlink.h >> +++ b/include/uapi/linux/netlink.h >> @@ -28,6 +28,7 @@ >> #define NETLINK_RDMA 20 >> #define NETLINK_CRYPTO 21 /* Crypto layer */ >> #define NETLINK_SMC 22 /* SMC monitoring */ >> +#define NETLINK_RLIMIT_EVENTS 23 /* rlimit notification */ > > No tabs? ahhh, my emacs is getting crazy after my last customization experiments. I'll fix this. It's weird that checkpatch didn't complain about this one. > >> --- /dev/null >> +++ b/include/uapi/linux/rlimit_noti.h >> @@ -0,0 +1,71 @@ >> +/* >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. > > GPLv2+ in a user api header file? You are really brave :) Like above > >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#ifndef _UAPI_LINUX_RLIMIT_NOTI_H_ >> +#define _UAPI_LINUX_RLIMIT_NOTI_H_ >> + >> +#ifdef __KERNEL__ >> +#include >> +#include >> +#else >> +#include >> +#endif >> + >> +#define RLIMIT_GET_NOTI_FD 1000 >> + >> +/* ioctl's */ >> +#define RLIMIT_ADD_NOTI_LVL 1 >> +#define RLIMIT_RM_NOTI_LVL 2 >> + >> +#define RLIMIT_SET_NOTI_ALL 3 >> +#define RLIMIT_CLEAR_NOTI_ALL 4 > > No tabs? > >> + >> +/* >> + * For future (notify every 5, 10 units change): >> + * #define RLIMIT_SET_NOTI_STEP 5 >> + */ >> + >> +#define RLIMIT_GET_NOTI_LVLS 6 >> +#define RLIMIT_GET_NOTI_LVL_COUNT 7 >> + >> +/* Flags for ioctl's */ >> +#define RLIMIT_FLAG_NO_INHERIT (1u << 0) >> + >> +/* Event types */ >> +enum { >> + RLIMIT_EVENT_TYPE_RES_CHANGED, >> + RLIMIT_EVENT_TYPE_MAX >> +}; >> + >> +/* TODO take care of padding (packed) */ >> +struct rlimit_noti_subject { >> + pid_t pid; >> + uint32_t resource; >> +}; > > For structures that cross the user/kernel boundry, you have to use the > correct variable types. And that is never "unit32_t" and such, use > "__u32" and the other "__" types. > > And are you _sure_ that pid_t is able to be exported to userspace > correctly? Hmmm it's used in kernel headers alongside with __kernel_pid_t, but the later one is just a typedef from include/linux/types.h: typedef __kernel_pid_t pid_t; but if you think I should use __kernel_pid_t then I'll fix this. > >> + >> +struct rlimit_noti_level { >> + struct rlimit_noti_subject subj; >> + uint64_t value; > > __u64 > >> + uint32_t flags; > > __u32 > > And so on for all others. I'll fix this for v2. > > You don't seem to describe an ioctl here in the "normal" method, but > only use vague numbers up above, that's odd, why? Sorry, there is no real reason. Just started with numbers to prepare some working prototype to show the concept before doing whole implementation and forgot to fix this. > >> diff --git a/init/Kconfig b/init/Kconfig >> index 1d3475fc9496..4bc44fa86640 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -332,6 +332,12 @@ config AUDIT_TREE >> depends on AUDITSYSCALL >> select FSNOTIFY >> >> +config RLIMIT_NOTIFICATION >> + bool "Support fd notifications on given resource usage" >> + depends on NET >> + help >> + Enable this to monitor process resource changes usage via fd. > > Mix of tab and spaces :( > Sorry, I'll fix this. I'm curious why checkpatch didn't catch this. It reported some whitespace errors and I fixed all of them but they are still in there:( Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics