Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <d9b4a501-1827-1c5d-0e14-3957edf7b538@nvidia.com>
Date:   Wed, 25 Oct 2023 09:10:46 +0300
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v2 iproute2-next 3/3] rdma: Adjust man page for rdma
 system set privileged_qkey command
To:     Petr Machata <petrm@nvidia.com>
Cc:     jgg@ziepe.ca, leon@kernel.org, dsahern@gmail.com,
        stephen@networkplumber.org, netdev@vger.kernel.org,
        linux-rdma@vger.kernel.org, linuxarm@huawei.com,
        linux-kernel@vger.kernel.org, huangjunxian6@hisilicon.com,
        michaelgur@nvidia.com
References: <20231023112217.3439-1-phaddad@nvidia.com>
 <20231023112217.3439-4-phaddad@nvidia.com> <87zg0856io.fsf@nvidia.com>
Content-Language: en-US
From:   Patrisious Haddad <phaddad@nvidia.com>
In-Reply-To: <87zg0856io.fsf@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bWVad3Qwa1pvWlliUWhyLzBpVGZYOFlWdGFrMDVsWnQwQkZLTUEwR05QMVU0?=
 =?utf-8?B?NkU0WU5Vc2U0YjBiSUtQbk1zWHRCV3IyOTE4d1VteXJZRzUxSWNvZGw3QlJm?=
 =?utf-8?B?cDN1T2s0NVNDV2hieUNLbW5ycGFPTmhlYjZpVW9KWm5yRkNONHZaMDg3Zjgv?=
 =?utf-8?B?aFQ1K29ZMzltTmJhNW0yWXlhRG5XM1FkZkdOT0NrRXNKSmJiNlVxclBEL1hL?=
 =?utf-8?B?ZWlwd2d2WTEzK01VYzJyVmhNc1V2R2JMQWp2RkVRMHBwK3F2YXRNWDhlMWNI?=
 =?utf-8?B?aVVxOHdiSkJRUkRpbVBUbm1EZCtvSXBwQ3I2MExWbDRlUEhsSUgxSmxRM1Bj?=
 =?utf-8?B?OE44RVpRRy9QeVpTdUZQbWJ6SGl5Tjg4WFQvaXZtMDJLZkF0ZjJiOS9sR0NL?=
 =?utf-8?B?WXV1eFgvYkNiaWJqeHpkN2J1SUxoREVMUGwvVWNGd1JieVpTK29hNG9hQUNG?=
 =?utf-8?B?ZWtYN0NaNUIzSW1zWGRTWWh1K1JjOGRtZGZhK0lLVU13S3BDMit0aWw3WWFa?=
 =?utf-8?B?bjBGbURjTjMvUDNZbzZLSlZSdUpoOHB3cU9uK2pwN21DWks4Vlhsb1grNWFu?=
 =?utf-8?B?RXRhZ0NDSjVLTGN3Z080VTFjaXRYdTlpaFZjQlZQRnArWGhvQ09ka2pHcDlW?=
 =?utf-8?B?Mnoxa3NPSmVvVkN4STlod28wMVVUNDhic3pmb2FxbG1RbXVkMmRkanNjY1pM?=
 =?utf-8?B?V05oY0VCMHc0T0t0bFJLeUVsY0RtOWF1UGZCc0hPSzhoeHR5OHViVVczTnJq?=
 =?utf-8?B?ZC90cjdYQ0ZhUGYvNkx5QWY0TUwvK0tiZUtLSzA0WC9nVGtCV2ZuME5tK1Vq?=
 =?utf-8?B?dlFGT2JjbjUrWEorVlNTbE1jTDBUazdsd0tPYUllbVkvYlhlYnY3WG5ZaFZL?=
 =?utf-8?B?d3czZEw4OEJKQ29LR0xnRnFOdGQrc1JNeGJYMnQyWitUZlkwVExneE93Z0xw?=
 =?utf-8?B?SCtJMjMrMUZqK2QvMDE4WmpaQnU2VnRLN2lzVGxDWFJrdE45ZmJRQjIySWtU?=
 =?utf-8?B?RG13VktBbjhzL0Vjc3NiNWtMS0x0eCt5MmgwMXFDME9kemlleTdHVnV1UGlE?=
 =?utf-8?B?WWZUbDBzdjdvUkhPS1d5TWpQU0tONVhpSWZOcUJDMlJUa3c5NFZkZ2tSZWNW?=
 =?utf-8?B?UlgxVXlYVFlaUXpPN0pMODhHRmpFRXdVa2xPRWdrQ0Z2aGcwN0RmQk9zZVV4?=
 =?utf-8?B?OTVnQVdPcFpxYU5KeGxib21nYUtpc0pmZFl1Z1o0Y1ljTmVCYWpjdVA4ZUNW?=
 =?utf-8?B?YW1CV3h1SjcrZ01ERXM4S2d4L29YS3h4VzM5Q1ZxeXllU3Y2Zk5EcWZuK05P?=
 =?utf-8?B?cVFYdDdLYmtlRk9GMi9FeHJRaDZoUXhjVDlzVTBTa1hoVG1qY2RUR285Y3RQ?=
 =?utf-8?B?cTZmUTZqMWZXMWtYQzAzY2lYMmwrL1c3VnoxdC9heGQ3cm44bE8wc3dzQ3dj?=
 =?utf-8?B?VFBwUi9EbjJYSWVHV2N4ckg3WnlsVk9uNW9DSDJjeFlRdmV5b1hWOFE1bGtl?=
 =?utf-8?B?bE5kRmlmcGFVVE9TdUdFS3FhOEF4em14Uks2aU9iTmlGbTR1ZjNRQVprUE9z?=
 =?utf-8?B?QkdRNXRrVDVuekpRa1JqV01hRE85NTN6OFVnaGpBR3B0WkJhV3NRRHlGeWlo?=
 =?utf-8?B?MjdQMHlCSXFtWXowaWw0QjhaTkp6TUR3djljeFk1OVRZZFJ1c1M4VFZGamND?=
 =?utf-8?B?dmxhWmE2QVJWR2pSTnM5Z1dKOFVJa01iUHdzbkpwM0s4NTlnUWNpWWFzUDl5?=
 =?utf-8?B?a1h3Nzd2N0pNQjMxc1BNZ2swZ0FFMS9ETVRPUklTUnJiZnFTcjBldzUwdUZX?=
 =?utf-8?B?SFlMWmhXamRTMTlOMDI3MDlRZGI2ekhwZ0QyRkhzaWZsRmpVWW01bjVPV2pv?=
 =?utf-8?B?OEZ3a3p2ZjUvU2I5TkkxamVXQWdLaVFUNzkxb2liOHczcytpb096RWlvcUNl?=
 =?utf-8?B?RGwvY3JrWlhkaVFWMExQUDc1Zy9ScjBIOW9jK3VJeVZWM2NGUEJ2bHgxN2dv?=
 =?utf-8?B?WVB6ZktJci9NQ2dtdlZtZHZXNW9Ka2RKRk9VU3VLZ2VJV3BMeEZWNE1CdWpI?=
 =?utf-8?B?QnZoc204aXZuUDVTZlZXQ1d3YkZJbzB0YU02RysrU1lHSjdkeFZLbXJpS1U3?=
 =?utf-8?Q?rh4iGzucM4f969BxlNWhmedUQ?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e4e9bed5-d23f-4462-926a-08dbd52125ce
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6660.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Oct 2023 06:10:55.0762
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: uhiKfQ7CdO2UBituD8QjAARwQWmawg8lbjbcsYHtpsPyUbPzdeItIu8BrtcBtNdYPuboN9Xk3ueLANPwcWGK3w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB7283
X-Spam-Status: No, score=-4.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 24 Oct 2023 23:11:12 -0700 (PDT)


On 10/24/2023 7:09 PM, Petr Machata wrote:
> Patrisious Haddad <phaddad@nvidia.com> writes:
>
>> Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
>> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>> ---
>>   man/man8/rdma-system.8 | 32 +++++++++++++++++++++++++++-----
>>   1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/man/man8/rdma-system.8 b/man/man8/rdma-system.8
>> index ab1d89fd..a2914eb8 100644
>> --- a/man/man8/rdma-system.8
>> +++ b/man/man8/rdma-system.8
>> @@ -23,16 +23,16 @@ rdma-system \- RDMA subsystem configuration
>>   
>>   .ti -8
>>   .B rdma system set
>> -.BR netns
>> -.BR NEWMODE
>> +.BR netns/privileged_qkey
>> +.BR NEWMODE/NEWSTATE
> What is this netns/priveleged_qkey syntax? I thought they are
> independent options. If so, the way to express it is:
>
> 	rdma system set [netns NEWMODE] [privileged_qkey NEWSTATE]
>
> Also, your option is not actually privileged_qkey, but privileged-qkey.

I'll fix the typo in the argument name, but about them being independent 
while that is true I used or,

since they can't be configured using the same command whereas your 
proposed syntax gives the feeling that you can

configure both in the same rdma system set command.

>
>>   .ti -8
>>   .B rdma system help
>>   
>>   .SH "DESCRIPTION"
>> -.SS rdma system set - set RDMA subsystem network namespace mode
>> +.SS rdma system set - set RDMA subsystem network namespace mode or privileged qkey mode
>>   
>> -.SS rdma system show - display RDMA subsystem network namespace mode
>> +.SS rdma system show - display RDMA subsystem network namespace mode and privileged qkey state
> Maybe make it just something like "configure RDMA system settings" or
> whatever the umbrella term is? The next option will certainly have to do
> something, this doesn't scale.
>
> Plus the lines are waaay over 80, even over 90 that I think I've seen
> Stephen or David mention as OK for iproute2 code.

Will fix all line lengths, thanks for noting.

True about scaling, but isn't the idea behind man-page to be 
comprehensive and list all options explicitly ?

>
>>   .PP
>>   .I "NEWMODE"
>> @@ -49,12 +49,21 @@ network namespaces is not needed, shared mode can be used.
>>   
>>   It is preferred to not change the subsystem mode when there is active
>>   RDMA traffic running, even though it is supported.
>> +.PP
>> +.I "NEWSTATE"
>> +- specifies the new state of the privileged_qkey parameter. Either enabled or disabled.
>> +Whereas this decides whether a non-privileged user is allowed to specify a controlled
>> +QKEY or not, since such QKEYS are considered privileged.
>> +
>> +When this parameter is enabled, non-privileged users will be allowed to
>> +specify a controlled QKEY.
> This is missing syntax notes. One might think that to enable it they
> need to say "enable", but in fact it's "on", and "off" for disabled.
> There should be an "{on | off}" somewhere in there.
>
> Also, line length.
>
> Also, the paragraph is imho a bit long-winded. Maybe make it just this?
>
> 	determines whether a non-privileged user is allowed to specify a
>          controlled QKEY or not.
will rewrite.
>
>>   .SH "EXAMPLES"
>>   .PP
>>   rdma system show
>>   .RS 4
>> -Shows the state of RDMA subsystem network namespace mode on the system.
>> +Shows the state of RDMA subsystem network namespace mode on the system and
>> +the state of privileged qkey parameter.
>>   .RE
>>   .PP
>>   rdma system set netns exclusive
>> @@ -69,6 +78,19 @@ Sets the RDMA subsystem in network namespace shared mode. In this mode RDMA devi
>>   are shared among network namespaces.
>>   .RE
>>   .PP
>> +.PP
>> +rdma system set privileged_qkey enabled
>> +.RS 4
>> +Sets the privileged_qkey parameter to enabled. In this state non-privileged user
>> +is allowed to specify a controlled QKEY.
>> +.RE
>> +.PP
>> +rdma system set privileged_qkey disabled
>> +.RS 4
>> +Sets the privileged_qkey parameter to disabled. In this state non-privileged user
>> +is *not* allowed to specify a controlled QKEY.
>> +.RE
>> +.PP
> on | off, not enabled | disabled.
yeah I don't know how I missed that will fix in all instances of man-page.
>
>>   .SH SEE ALSO
>>   .BR rdma (8),