On 11/25/22 18:19, Adrian Moreno wrote:
> Hi Mike,
> 
> Sorry it took that long to review this patch.
> 
> On 3/25/22 23:17, Mike Pattrick wrote:
>> Add new option --dump-hugepages option in ovs-ctl to enable the addition
>> of hugepages in the core dump filter.
>>
>> Signed-off-by: Mike Pattrick <[email protected]>
>> ---
>>   NEWS                 |  4 ++++
>>   utilities/ovs-ctl.in | 15 +++++++++++----
>>   2 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 8fa57836a..7af60dce3 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -3,6 +3,10 @@ Post-v2.17.0
>>      - OVSDB:
>>        * 'relay' service model now supports transaction history, i.e. honors 
>> the
>>          'last-txn-id' field in 'monitor_cond_since' requests from clients.
>> +   - ovs-ctl:
>> +     * New option '--dump-hugepages' to include hugepages in core dumps. 
>> This
>> +       can assist with postmortem analysis involving DPDK, but may also 
>> produce
>> +       significantly larger core dump files.
>>   
> 
> I'm afraid this part needs rebasing.
> 
>>     v2.17.0 - 17 Feb 2022
>> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
>> index e6e07f476..8f900314b 100644
>> --- a/utilities/ovs-ctl.in
>> +++ b/utilities/ovs-ctl.in
>> @@ -103,8 +103,13 @@ set_system_ids () {
>>       action "Configuring Open vSwitch system IDs" "$@" $extra_ids
>>   }
>>   -check_force_cores () {
>> -    if test X"$FORCE_COREFILES" = Xyes; then
>> +check_core_config () {
>> +    if test X"$DUMP_HUGEPAGES" = Xyes; then
>> +        echo 0x3f > /proc/self/coredump_filter

Shouldn't this be 0x7f instead?
0x3f doesn't enable bit #6, which is responsible for dumping
shared huge pages.  Or am I missing something?

Best regards, Ilya Maximets.

>> +        if test X"$FORCE_COREFILES" = Xyes; then
>> +            ulimit -c unlimited
>> +        fi
>> +    elif test X"$FORCE_COREFILES" = Xyes; then
>>           ulimit -c 67108864
>>       fi
>>   }
>> @@ -116,7 +121,7 @@ del_transient_ports () {
>>   }
>>     do_start_ovsdb () {
>> -    check_force_cores
>> +    check_core_config
>>         if daemon_is_running ovsdb-server; then
>>           log_success_msg "ovsdb-server is already running"
>> @@ -193,7 +198,7 @@ add_managers () {
>>   }
>>     do_start_forwarding () {
>> -    check_force_cores
>> +    check_core_config
>>         insert_mod_if_required || return 1
>>   @@ -330,6 +335,7 @@ set_defaults () {
>>         DAEMON_CWD=/
>>       FORCE_COREFILES=yes
>> +    DUMP_HUGEPAGES=no
>>       MLOCKALL=yes
>>       SELF_CONFINEMENT=yes
>>       MONITOR=yes
>> @@ -419,6 +425,7 @@ Other important options for "start", "restart" and 
>> "force-reload-kmod":
>>   Less important options for "start", "restart" and "force-reload-kmod":
>>     --daemon-cwd=DIR               set working dir for OVS daemons (default: 
>> $DAEMON_CWD)
>>     --no-force-corefiles           do not force on core dumps for OVS daemons
>> +  --dump-hugepages               include hugepages in coredumps
>>     --no-mlockall                  do not lock all of ovs-vswitchd into 
>> memory
>>     --ovsdb-server-priority=NICE   set ovsdb-server's niceness (default: 
>> $OVSDB_SERVER_PRIORITY)
>>     --ovsdb-server-options=OPTIONS additional options for ovsdb-server 
>> (example: '-vconsole:dbg -vfile:dbg')
>>
> 
> Tested locally and verified that with the option hugepages appear in 
> coredumps.
> Apart from the need to rebase the NEWS, the patch looks good to me.
> 
> Acked-by: Adrian Moreno <[email protected]>
> 
> -- 
> Adrián Moreno

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to